Änderung / Erweiterung an Entity, Komponenten und Extensions #249
Conversation
* Removed UniverseId * Static Settings * Add SimulationTest * Bugfix assembly loading
* Add Design document * Add Multiplayer button * Change ResourceManager ctor
some trys
This reverts commit 6c8b5b4.
component system wächst
Extended IDefinitionManager Splitted Extension Cleanup, added Interfaces,
| //TODO More Generic, überdenken der Planetgeneration im allgemeinen (Heapmap + Highmap + Biome + Modding) | ||
| // +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ | ||
|
|
||
| ushort sandIndex = definitionManager.GetDefinitionIndexByName(Languages.OctoBasics.Sand); |
There was a problem hiding this comment.
Das ist meiner Meinung nach nicht so gut, den lokalisieten Namen zu verwenden. Verleitet einen ENtwickler, der nur in Sprache X testet deren hardcoded Wert zu verwenden, und in Sprache Y gehts dann nicht mehr. rweiterungsübergreifend ist auch per se kein gegenseitiger Zugriff auf die Lokalisierung möglich.
Wichtig: Diese A´nmerkung gilt gleich für alle anderen Stellen, an denen dies so verwendet wird.
There was a problem hiding this comment.
Ja das ist mir bewusst. Ich plädiere für ein festes Token auch Server übergreifend, Aber nebenbei gesagt das ist Modder Code, wer kann den Modder schon zwingen in verschiedenen sprachen zu Coden :D
aber hast recht! das wäre ohnehin nur vorübergehend so geblieben
| { | ||
| base.Deserialize(reader, definitionManager); | ||
| base.Deserialize(reader, definition); |
There was a problem hiding this comment.
Hier finde ich den alten Namen schöner, aber über Namensgebung lässt sich streiten... Vor allem weiter unten bräcuhte man den Namen "definition" für eine Definition, da muss auf "def" ausgewichen werden.
There was a problem hiding this comment.
Wollte die Signatur ändern zum IGameService. Dass dann so hängen geblieben ;O aber hast auch recht.
| @@ -90,11 +88,14 @@ public bool RemoveUnit(InventorySlot slot) | |||
| if (slot.Amount >= definition.VolumePerUnit) // Wir können noch einen Block setzen | |||
| { | |||
| slot.Amount -= definition.VolumePerUnit; | |||
| if (slot.Amount <= 0) | |||
| Inventory.Remove(slot); | |||
| if (slot.Amount <= 0) Inventory.Remove(slot); | |||
There was a problem hiding this comment.
Coding styles... Aber es werden weniger Verstöße ;-)
| } | ||
| else if (service.TakeBlock(controller, Entity.Cache, out IInventoryableDefinition item)) | ||
| { | ||
| // TODO: und jetzt ? |
There was a problem hiding this comment.
Ich finde es etwas komisch, dass die ToolbarComponent direkt mit der Welt interagiert.
There was a problem hiding this comment.
Warum nicht ? Direkt ist es eben nicht, sondern über den Service. Der Cache liegt eh in der Enttiy... da könnte sonst was mit gemacht werden. Ob es diese Komponente macht oder eine andere ist für mich kein Unterschied. Die Komponenten berechnen sich selbst wenn es geht. Dann braucht man kein typeof(T) gesuche mehr in irgendwelchen Liste.
| { | ||
| if (Tools.Any(s => s != null && s.Definition.Name == slot.Definition.Name)) | ||
| continue; | ||
| else AddNewSlot(slot); |
There was a problem hiding this comment.
Coding styles oder sich das else sparen.
| { | ||
| player.Components.AddComponent(new GroundPhysicComponent(60f, 400f, 0.8f, 3.5f)); | ||
| player.Components.AddComponent(new InventoryComponent()); | ||
| player.Components.AddComponent(new ToolBarComponent()); |
There was a problem hiding this comment.
Was hat das Inventar-Zeug mit Physik zu tun?
There was a problem hiding this comment.
Nichts. Kann dir eine dritte schreiben ^^
| namespace OctoAwesome.Basics.SimulationComponents | ||
| { | ||
| class PhysicEngine | ||
| { |
There was a problem hiding this comment.
Was macht diese leere Klasse? Außerdem nach Coding Styles bitte immer die Zugriffsmodifikatoren angeben...
| private IDefinitionManager definitionmanager; | ||
| /// <summary> | ||
| /// Manager for persistance at lokal Disk. | ||
| /// </summary> |
There was a problem hiding this comment.
Hast du die Doku-Kommentare gemacht? Wir hatten uns darauf geeinigt, dass diese in Deutsch auszuführen sind. Bitte keine Diskussion - hatten wir über das Thema schon genug!
| IDefinitionManager DefinitionManager { get; } | ||
| /// <summary> | ||
| /// Take a Block from the World. | ||
| /// </summary> |
There was a problem hiding this comment.
Doku-Kommentare bitte auf Deutsch.
|
Ich habe jetzt mal grob drübergeschaut, meine ersten Eindrücke: Allgemein:
In deiner PDF, was meinst du mit "Das Selektieren von Blöcken im Inventar wurde jedoch nicht wieder umgesetzt."? Wenn Funktionalität verloren geht, ist das nie gut. UI Bezüglich der Branches Zu den Komponenten / Rendering sollen bitte andere etwas sagen. Physik habe ich mir auch nicht angeschaut, aber warum hats du denn überhaupt die Herangehensweise geändert, wenn du sie selbst als schlecht handhabbar (vgl. PDF S. 13) bezeichnest? |
|
Meine Antworten nicht als Diskussionsversuch sehen :) zumindest nicht immer. Ein bisschen rechter fertigen werde ich mich ja dürfen.
Es konnte früher mit den Tasten 1-9 und Maus über einem Block im Inventar, Blöcke in die Toolbar geschoben werden. Das konnte ich auf die schnelle nicht fixen :( Sie können nur mit der Maus reingezogen werden.
Zur UI muss ich sagen, dass ich es in zwei Teile aufgeteilt habe, weil durch die allgemeinere Umsetzung konnte ich es nicht wieder so verbinden wie es war. Dafür landen gerade alle Möglichen UI Controls im "Inventar" und liegen in den ComboBox(en) oben sodass man wechseln kann. Vllt kommt ja später auch noch ein Skill-Tree oder so ka. Wenn das wirklich so schlimm ist (ja hübsch ist es nicht) kann man auch eine Komponenten basteln die beides ist. So Enorm ist der Aufwand nicht. Außerdem müsste dann die Registrierung auf Tasten möglich sein, also fände ich Vorteilhafter.
Die Redundanz war auch schon vor meiner Änderung gegeben ^^ sieht man jetzt vllt. stärker aber eigentlich ist das Toolbar-Inventar Control genauso wie das alte.
WAS ist denn da passiert... hab es nochmal angeschaut, also da sollte eigentlich nichts mehr im Code sein. Falls doch macht es die Develope auch nicht kaputt, aber eure Entscheidung.
Die Physik wollte ich so mal Testen weil mir die Rechnung Kraft * Kraft nicht so gefallen hat xD. Es ist im Moment auch nicht wirklich ausbalanciert. Vllt. finden sich ein paar Fürsprecher wenn nicht ändere ich es beim nächsten Push, versprochen!
Alle ? ;O... würd das dann nebenbei machen... Ein Kommentar am Tag ... |
Paar Kommentare von en auf de geändert. Leere Klasse entsorgt. Definition in definitionmanager umbenannant.
|
So erstmal danke für deinen erneuten Pull Request. Vorne weg etwas lob: Der Pull Request an sich sieht sehr ordentlich aus mit kurzer Beschreibung und Überschrift, wirklich kein vergleich zum letzen mal top. Habe auch gesehen das du mir eine E-Mail geschickt hast, ich denke das die Mail mit dem Pull Request zu tun hatt, lese ich mir heute abend durch. Wir gucken uns deinen Pull Request an und bitten um etwas geduld :) |
There was a problem hiding this comment.
Ok habe das erstmal ganz oberflächlich gecheckt. Mal wieder wären ein paar stylguide Geschichten zu beheben.
Der GameService ist soweit eine gute Idee.
Leider ist sowohl kollission und Physik komplett kaputt so das das spiel nicht spielbar wird.
Es wäre von vorteil deine Eingefügten Todo's bitte noch in deinem Kommentar zu erwähnen so kann man nach einem Merge gleich Issues draus machen.
Zu deiner PDF und dem Komponenten Thema schreibe ich in einem gesonderten Kommentar etwas dazu.
Inhaltlich wären die Änderungen noch zu prüfen.
| //TODO More Generic, überdenken der Planetgeneration im allgemeinen (Heapmap + Highmap + Biome + Modding) | ||
| // +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ | ||
|
|
||
| ushort sandIndex = definitionManager.GetDefinitionIndex<SandBlockDefinition>().FirstOrDefault(); |
There was a problem hiding this comment.
Warum kommt von GetDefinitionIndex eine IList zurück? Rein von namen her würde ich bereits einen Index erwarten und keine ganze liste.
There was a problem hiding this comment.
Weil es eine Definition theoretisch zweimal oder mehrere male geben kann :P
| @@ -0,0 +1,56 @@ | |||
| # Network / Multiplayer | |||
|
|
|||
There was a problem hiding this comment.
Das ist Teil des Multiplayers branches
| Value = 40; | ||
| Margin = Border.All(20, 30); | ||
| } | ||
| public override string ToString() |
There was a problem hiding this comment.
Verwende hier am besten den Pfeiloperator
| { | ||
| // TODO: resx -> Languages.OctoBasics.Inventory or what ever... | ||
| return "Inventar"; | ||
| } |
There was a problem hiding this comment.
Hier am besten auch den Pfeilchenoperator verwenden
| grid.AddControl(buttons[i], i, 1); | ||
| } | ||
| } | ||
| protected override void OnUpdate(GameTime gameTime) |
There was a problem hiding this comment.
Mach hier bitte noch eine Leerzeile rein, also zwischen die klassen ^^
| /// <returns></returns> | ||
| public static bool IsNullOrEmpty(this string input) | ||
| { | ||
| return string.IsNullOrEmpty(input); |
There was a problem hiding this comment.
Das ist unnötig das kann .NET nämlich von Haus aus von daher sehe ich die CodeExtension nicht als sinnvoll an. Vorallem da Extensions der grauen sind was Performance angeht.
There was a problem hiding this comment.
Da muss man aber immer string.IsNullOrEmpty(string) schreiben.
Echt hätte erwartet, dass der Jitter das inline macht...
There was a problem hiding this comment.
Nope die mergen das recht umständlich während der Laufzeit. Da ist zwar ne änderung geplant in C# 8 was ein extension keyword einfügt aber leider noch keine perfomance verbesserung. Um das performanter zu machen bedarf die .NET runtime in dem punkt ein redesign.
Außerdem ist sehr häufig string.IsNullOrWhitespace besser.
There was a problem hiding this comment.
Ich würde es durchgehen lassen wenn die anderen ihr grünes licht dazu geben.
| IDefinitionManager DefinitionManager { get; } | ||
| /// <summary> | ||
| /// Take a Block from the World. | ||
| /// </summary> |
| /// </summary> | ||
| public Entity Entity { get; set; } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hier ist der Dateiname wirklich irreführend. Ich bin sowieso nicht so der Freund davon mehrere Klassen in einer Datei zu haben. Aber wenn man das macht dann sollte doch der Name entsprechend sein.
There was a problem hiding this comment.
Das nicht von mir hab es nur verschoben ^^
| @@ -10,6 +10,7 @@ public interface IMapPopulator | |||
| /// </summary> | |||
| int Order { get; } | |||
|
|
|||
| //TODO: Kommentieren xD | |||
|
|
||
| namespace OctoAwesome | ||
| { | ||
| // TODO: implementieren -> egal was oder wie xD |
There was a problem hiding this comment.
Das Todo entfernen und lieber später in ein Issue packen
Kurzes Fazit
Zu deinen Interfaces:Du gehts hier eigentlich den entgegengesetzten weg als wir bisher in Octo gegangen sind. Wir haben uns ganz bewusst dazu entschieden auf Komponenten zu setzen und nicht wie bei Unity auf ein Interface, Koponenten misch masch oder nur Interfaces. Alle drei seiten haben natürlich vor- und nachteile. Sowas sehe ich aber fast schon als generelle Design entscheidung. Es ist etwas doof wenn ein Teil des Projektes es so macht und der andere teil anderst. Zur Phsysik:Die Physik logik ist in deinem Code komplett zerschossen. Von daher würde ich das wenn du das ändern möchtest separiert sehen. Es wäre gut die alte Physik wieder herzustellen. Zusammenfassend:Eigentlich versuchst du hier drei Teilbereiche einzuführen wenn ich dich richtig verstehen: neue Physik, neues Komponentensystem, einführung des Gameservices. Oder ? Aktuell, und ich muss es wirklich mir noch inhaltlich genauer ansehen, sind das eigentlich irgendwie 3 Pull Requests.
Außerdem fehlt für einen Merge eine entsprechende veröffentlichung, somit würde das maximal in den Stream fallen und das würde frühenstens nach dem Multiplayer stattfinden. |
Gallimathias
added
input needed
|
Also ich bin noch nicht dazu gekommen, darüber zuschauen und ich weiß nicht ob das gleich wird. |
|
Also es ist wieder kompliziert jetzt. 1)Ich denke die neuste Version von Engenious sollte schon laufen, weil die 1.12 ist schon relativ alt. Mein Problem ist gerade das es immer mehr ein Monster wird, der Pullrequest und es vermischen sich viele Dinge. Mein Problem ist immer noch, das das System für Multiplayer nicht ausgereift genug ist, aber es gibt viele kleine und gute Ansätze die man Umsetzen kann und darauf Aufbauen sollte. |
|
Ja das mit Mutliplayer Basis war ein fail. Aber der Stand war noch nicht allzu weit von der Develop entfernt. Bis auf das eine File das ihr beide angemerkt hab ist sonst eigentlich nichts mehr drin und das sind Notizen. Es tut mir schrecklich leid :( Zum Rest muss ich sagen: Ich hab eigentlich versucht eine Struktur zu implementieren (bzw. den Weg dafür einzuschlagen) die mehr Leisten kann als das gegenwärtige Komponenten System. Dafür musste ich eben mehr ändern auch die UI. Es muss nicht über Interfaces gehen, ist jetzt eine persönliche vorlebe, können auch Basisklassen sein. Interessant ist, dass alle das womit ich wenigsten zeit verbracht hab am besten finden... xD |
|
Eigentlich wurde bisher nur ein einzelnes Issue von dir Geschlossen alle anderen sind noch aktiv. Weist du das Problem mit den Issues ist das selbe wie mit den Pull Requests, du willst zu viel auf einmal. Um so größer die sachen werden um so eher bloppen natürlich sachen auf oder können andere nichts damit anfangen weil sie zu allgemein gehalten sind. Hättest du z.b. tatsächlich erst den GameService gemacht dann hätte der warscheinlich sogar schon recht früh implementiert werden können. So aber machst du gleich 3 verschiedene Sachen in einem aufwasch. Auch in den Issues, du machst ein Issue Refactorisierung auf was absolut alles sein kann und willst gleich zich sachen in einem Issue abarbeiten. Das gleiche gilt für das Issue Vererbung, vererbung ist ein weites feld. Wenn man die Beschreibung liest willst du gleich zwei Dinge ansprechen (Basisklasse für Komponenten und Blöcke mit Funktionen) und im Gesprächsverlauf gehts dann auf einmal um Todo: unschön, typeof() methoden, um extensions, um ui. Ich hatte das issue kurzzeitig gelockt weil es zu sehr vom Thema abwich. Besser wäre es wenn du spezifischer und dedizierter vorgehst. z.b. ein Issue auf machst Titel: IEntityComponent, Text: hallo leute ich habe mir vorgestellt das wir eine IEntityComponent brauchen weil, und das soll folgendes können, und damit ist uns das und das möglich. Oder Titel: KomponentenSystem Redesign. Text: Ich finde wir müssen das Komponentensystem folgender maßen ändern weil..... Das gleiche gilt für einen Pull Request, wenn du kleinerere Einheiten Einreichst die spezifischer sind z.b. der GameService oder reaktion auf Http request 418 oder was auch immer dann ist zum einen für alle beteiligten die änderung überschaubarer und man kann dedizierter dinge anehmen oder ablehenen. Am besten vom develop branch über einen feature branch in den develop branch. So blockiert sich auch nichts. Aktuell blockieren dein Pull Request vorallem änderungen die vllt. nur minimal etwas miteinander zu tun haben. Zu aller letzt, wir schätzen dein Engangment, das ist wirklich toll, bringt ein wenig leben in die Bude so soll es sein aber wir haben hier bei Octo definitiv kein druck und auch kein enges Zeitfenster. Octo wird niemals ein Endstadium erreichen, so wie du es sagst eine Never Ending Story und zwar ganz absichtlich. Nimm ein wenig druck aus der sache raus und lasse dir doch zeit mit den Dingen es muss nicht an einem Wochenende das Komplette EntitySytem Redesignd werden. Und du darfst nicht vergessen das wir eine Gemeinschaft sind, man will miteinander Entwickeln und voneinander lernen, von niemandem ist verlangt das er ganze Funktionen des Spiels (UI, Multiplayer, Extensions etc...) fertig und perfekt abliefert, Und das ist auch der Punkt, so etwas wie ein neues Komponentensystem betrifft eben alle, also gibt es auch viele Meinungen zu einem Thema. Wenn du dich ein bisschen umguckst oder dir die Videos anschaust dann siehst du ja auch wie ähnliche Themen abgehandelt wurden (Definitions, Multiplayer, Universum, Sprache im Projekt) die leute möchten dazu ihre Meinung abgeben und ihre Ideen mit einbringen. Vllt. möchte das gerne jemand mit dir gemeinsam entwickeln, wer weis das schon, alles ist möglich wenn man bei Octo mitmacht. Also genug der vielen Worte wir machen das jetzt mal im laufe der woche so wie der @CsharpLassi gesagt hatt und dann gucken wir weiter. ^^ |
|
Was ist hier jetzt mit dem Pullrequest? |
|
Wir sind die Woche nicht dazu gekommen |
|
Zu diesem PR wurde eigentlich das wesentlichste gesagt. Wir lehnen den PR aus oben genannten Gründen ab. Was die Vorschläge zum Komponentensystem angeht werde ich für dich ein Issue aufmachen. Der Gameservice wird sehr gerne angenommen. Aber dann bitte diesen als einzelnen PR aufmachen. Der Pullrequest sollte in einem Branch sein der von develop stammt. |
Docu.pdf
und noch die alten Komponenten
OldComponents.zip
Also bei mir läuft es wenn ihr es ausprobiert und es nicht geht, tja was kann ich da sagen.
Bin auf engenious 1.12, bin nicht dazu gekommen engenious zu debuggen.
Ich ändere im Endeffekt nicht viel, es kommen neue dinge dazu aber ich hab versucht so wenig wie möglich umzuschreiben. Allerdings kann ich, sofern meine Änderungen euch zusagen, nicht weiter machen weil es sonst wirklich zu viel auf einmal wird. Das hier soll den den neuen Weg einleiten, es ist aber nicht mal im Ansatz vollständig - so wie OctoAwesome - eine Never Ending Story halt.
Da ich die feature/multiplayer als Basis benutzte... hab ich aus Faulheit das in meine Develope reingemerged. Ich hab hoffentlich alles was nicht rein gehört gelöscht. Versprechen kann ich aber
nichts :(