Überarbeitung von Entities, Komponenten und Extensions #245

HierGibtEsDrachen · 2018-02-27T05:41:44Z

Ein bisschen was überarbeitet.
Neue Interfaces für Extensions und grundlegende Mechanismen.
Simulation hoffentlich nicht verschlechtert. Wobei sie jetzt anders ist aber kann ich notfalls wieder ändern.
UI dynamischer gestaltet.
Entity angepasst.

component system wächst

Gallimathias

Also erstmal danke für deine Mitarbeit.

Dann ein paar dinge.

Bitte wähle einen aussagekräftigeren Titel
Werde bitte in deinem Kommentar deutlicher was du gemacht hast und welche konsequenzen das hat
Erläutere bitte die Todo's die du im Code hinzugefügt hast. Es wäre auch schön zusammenfassend in deiner Beschreibung zu lesen was noch getan werden muss
Bitte beachte unsere Coding Richtlinien CodeStyles

Da das eine größere Änderung ist, kann man die nicht einfach so mergen. Es wäre schön wenn du ein Video oder Blogeintrag oder ähnliches verlinkst, gemäß unserer Code of Conducts

Andernfalls wird es wohl noch sehr lange dauern bis wir das im Stream ausführlich bearbeiten können.

Gallimathias · 2018-02-27T06:24:30Z

OctoAwesome/OctoAwesome.Basics/ComplexPlanetGenerator.cs

@@ -11,10 +11,8 @@ public class ComplexPlanetGenerator : IMapGenerator
        public IPlanet GeneratePlanet(Guid universe, int id, int seed)
        {
            Index3 size = new Index3(12, 12, 3);
-            ComplexPlanet planet = new ComplexPlanet(id, universe, size, this, seed)
-            {
-                Generator = this


Es steht zwar nicht explizit in den Richtlinien aber ich würde die Schreibweise mit dem Initialisier bevorzugen.

Gallimathias · 2018-02-27T06:26:41Z

OctoAwesome/OctoAwesome.Basics/Controls/InventoryControl.cs

+            };
+            for (int i = 0; i < COLUMNS; i++)
+                grid.Columns.Add(new ColumnDefinition() { ResizeMode = ResizeMode.Parts, Width = 1 });
+            // TODO: wieder einfügen 


Bitte verwende aussagekräftigere Todo's wenn du sie im Code machst. Was möchtest du hier wieder einfügen

Gallimathias · 2018-02-27T06:27:13Z

OctoAwesome/OctoAwesome.Basics/Controls/InventoryControl.cs

+        public override string ToString()
+        {
+            // TODO: resx
+            return "Inventar";


Auch hier bitte ein Aussagekräftigeres Todo verwenden.

Gallimathias · 2018-02-27T06:29:13Z

OctoAwesome/OctoAwesome.Basics/Controls/ToolbarControl.cs

+
+        private ToolBarComponent toolbar;
+
+        public Label activeToolLabel;


Es steht auch nicht explizit in den Richtlinien wird aber so gehandhabt. Bitte Namen von Öffentlichen Variablen und Prop's groß schreiben

Gallimathias · 2018-02-27T06:31:48Z

OctoAwesome/OctoAwesome.Basics/Controls/ToolbarControl.cs

+        {
+            if (!Visible || !Enabled)
+                return;
+            if(!(toolbar.Entity is IControllable con) || con.Controller == null)


Laut unseren Codingrichtlinen sind out Variablen nur innerhalb des if's scopes zu verwenden

Das ist im selben Scope. Da C# bei If und || operatoren abbricht bzw. nicht mehr weiter prüft sobald die Bedingung erfühlt ist.
Zweitens das ist kein out... das ist immer noch ein is. Selbst wenn C#7 das aufgemotzt hat.

Ja ok das ist kein out habe ich übersehen. Aber nein das ist nicht der selbe Scope ^^ du returnst direkt unter dem if und verwendest somit das con außerhalb des if's s.h. zeile 101 oder 122. Das ist etwas was auch ich mir abgewöhnen muss wenn ich in octo programmiere.

Gallimathias · 2018-02-27T07:00:58Z

OctoAwesome/OctoAwesome/Entities/IEntityController.cs

+    /// </summary>
+    public interface IEntityController
+    {
+        // TODO: more buttons ... ?


Bitte ausführlicher werden

Gallimathias · 2018-02-27T07:02:04Z

OctoAwesome/OctoAwesome/IExtension.cs

@@ -25,6 +25,6 @@ public interface IExtension
        /// Register the Components in the ExtensionsLoader
        /// </summary>
        /// <param name="extensionLoader">ExtensionsLoader</param>
-        void Register(IExtensionLoader extensionLoader);
+        void Register(IExtensionLoader extensionLoadere);


Rechtschreibfehler?

Und das von dir :(

Hatte die Signatur erweitert und es mir dann doch anders überlegt. Aber ja Rechtschreibfehler!

Ok war mir unsicher, hätte auch absichtlich sein können. Ok, dann einfach das e wegnehmen ^^.

Gallimathias · 2018-02-27T07:02:49Z

OctoAwesome/OctoAwesome/IMapPopulator.cs

@@ -10,6 +10,7 @@ public interface IMapPopulator
        /// </summary>
        int Order { get; }

+        //TODO: Kommentieren xD


Bitte machen und nicht auffordern

Schau mal wer (und zwar nicht ich) die XML Comments für den Populator gemacht hat... ich hab wenigstens den Anstand es oben oben drüber zu schreiben.

/// <param name="column00">TODO: Kommentieren</param> /// <param name="column01">TODO: Kommentieren</param> /// <param name="column10">TODO: Kommentieren</param> /// <param name="column11">TODO: Kommentieren</param>

Wir hatten "vor deiner zeit" auch eine seeeeeeehr geladene und heftige Diskussion zu dem Thema Todos direkt im code. An sich sind sie wenn die nachwelt was damit anfangen kann ok. Grundsätzlich stehe ich aber auf dem Standpunkt das man die Todo's so stark wie möglich reduzieren sollte bis spätestens zu einem pull request. Des weiteren meine Ich wenn du den Code an der stelle eh schon geschrieben hast, warum hast du nicht auch gleich kommentiert?

Der ist ja nicht von mir "(und zwar nicht ich) " weiß ja nicht mal was ich da sinnvolles kommentieren soll :P
aus column 00 ...

Gallimathias · 2018-02-27T07:04:38Z

OctoAwesome/OctoAwesome/Simulation.cs

@@ -155,26 +160,26 @@ public void ExitGame()
        /// <param name="entity">Neue Entity</param>
        public void AddEntity(Entity entity)
        {
-            if (entity == null)
-                throw new ArgumentNullException(nameof(entity));
+            if (entity == null) throw new ArgumentNullException(nameof(entity));


Der Coding Style tut mir in den Augen weh.
Aber bitte. Ich versuch mich dran zu halten.

Wir haben ihn in der Gemeinschaft mit Abstimmung entschieden. Auch ich, auch lassi, Jvbsl und wer nicht alles müssen sich daran halten selbst wenn jeder von uns eigene coding style vorlieben hatt

Gallimathias · 2018-02-27T07:04:52Z

OctoAwesome/OctoAwesome/Simulation.cs


-            if (entity == null)
-                throw new ArgumentNullException(nameof(entity));
+            if (entity == null) throw new ArgumentNullException(nameof(entity));


XYZLassi · 2018-02-27T07:14:47Z

Puuh sowas zum Frühen morgen. Ich sag erstmal vielen Dank.
Ich bin die Änderungen mal kurz überflogen.

Aber ich hab leider ein paar Punkte anzumerken.
-Der Title ist nichts sagend und würde ich deshalb auch nicht mergen ^^
-Ich glaub nicht das Multiplayer das richtig ist zum mergen, aber das ist wohl Geschmackssache
-Es ist ein bissel viel

Mein Problem ist, es gibt viele Dinge die mir gefallen z.B.
-WorldService
-IUserInterfaceExtention
-... (noch einige andere finde die Stellen aber nicht aufanhibe mehr)

Und dann gibt es leider auch die, wo ich sage so lieber nicht z.B:
-Das zusammen Legen von Physik-Komponenten (Also die BewegungsComponenten)
-GetBlock im IChunk hat ein bool remove ????
-Das Zusammenlegen von Componenten direkt in Entities

Aber ich würd mir das heute Abend noch mal ein bissel besser Anschauen.

viele grüße
Lassi

Gallimathias

Hab es mal zusätzlich auf die schnelle Technisch auf meinem Toaster geprüft. Mir scheint das beim Merge ein wenig schief gelaufen ist. OctoAwesome ist in dem Eingereichten Stand weder Kompilier- noch ausführbar.

Gallimathias · 2018-02-27T09:59:55Z

OctoAwesome/OctoAwesome.Network/packages.config

@@ -1,6 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
 <packages>
-  <package id="engenious" version="0.1.17" targetFramework="net461" />


engenious wird in der Network.dll benötigt

Es ist doch der Sinn von einem Merge zu prüfen was geändert werden soll und was bleiben kann oder nicht ?

Develop wäre deshalb als stand etwas geschickter gewesen, im stream läuft nicht immer alles perfekt :D

Develop lief bei mir nicht :D

Was lief den im Develop nicht?

Nein ein Merge hat den zweck zwei Dinge zu einem zu vereinen. Der jenige der den merge ausführt muss, zumindestens wenn es quellcode betrifft, überprüfen oder wissen welches von welcher seite das gültige bzw. richtige ist.

Andere Wortwahl, naja egal jetzt.
In der SceneControl, Zeile 80.

simpleShader = manager.Game.Content.Load<Effect>("simple");

Die Exceptions sind auch sehr aufschlussreich:
Message: ""
Stacktrace:
bei engenious.Graphics.Shader.Compile()
bei engenious.Content.Serialization.EffectTypeReader.Read(ContentManager manager, ContentReader reader)
bei engenious.Content.Serialization.ContentTypeReader`1.engenious.Content.Serialization.IContentTypeReader.Read(ContentManager manager, ContentReader reader)
bei engenious.Content.ContentManager.ReadAsset[T](String assetName)
bei engenious.Content.ContentManager.Load[T](String assetName)
bei OctoAwesome.Client.Controls.SceneControl..ctor(ScreenComponent manager, String style) in C:\Users\Klaus A\Source\Repos\octoawesome\OctoAwesome\OctoAwesome.Client\Controls\SceneControl.cs:Zeile 80.

Wenn du genaueres wissen willst führ das ContentTool manuell aus und schau die Fehlermeldung dort an, wenn es dort keine gibt, läuft irgendetwas mächtig schief.
Welche OpenGL Version hast du denn? OpenGL 1.1 ist zu alt

Dieser Kommentar hängt mit #248 zusammen. Bitte schreibe deinen Kommentar dort nochmal @jvbsl der Pull Request ist ja schon zu.

Gallimathias · 2018-02-27T10:00:44Z

OctoAwesome/OctoAwesome.Client/OctoGame.cs

+            extensionLoader.LoadExtensions();
+
+            DefinitionManager = new DefinitionManager(extensionLoader);
+            ResourceManager = new ResourceManager(extensionLoader, DefinitionManager, Settings);


Der ctor aufruf ist nicht korrekt. Der ResourceManager möchte vier Parameter haben.

Gallimathias · 2018-02-27T10:01:20Z

OctoAwesome/OctoAwesome.Network/SimulationManager.cs

-
-            extensionLoader = new ExtensionLoader(settings);
+
+            ExtensionLoader extensionLoader = new ExtensionLoader(settings);


ExtensionLoader gibt es nicht mehr als Prop

Gallimathias · 2018-02-27T10:03:44Z

OctoAwesome/OctoAwesome.Client/OctoGame.cs

@@ -84,8 +76,23 @@ public OctoGame() : base()

            Assets = new AssetComponent(this);
            Components.Add(Assets);


Hier kommt es zur NullReferenceException

Das ist ein Property aus dem Basis Klasse. Was habt ihr da bitte geändert?
Oder was hab ich falsch gemacht ?

Die Property wird local in diesem Stand ausgeblendet also nein das ist nicht die Prop aus der Basisklasse ;)

ManuelHu · 2018-02-27T10:23:13Z

OctoAwesome/OctoAwesome.Client/Controls/SceneControl.cs

@@ -106,10 +106,10 @@ internal sealed class SceneControl : Control
                }
            }

+            // TODO: valid planet id should be > 0


Warum sollte 0 keine valide ID sein? Zählen ab 0 ist doch normal. Wir brauchen sowieso immer minimal einen Planeten und der ist halt Nummer 0.

XYZLassi · 2018-02-27T10:26:46Z

Es kompiliert nicht bei mir,
Viele Datein sind nicht im Projekt mehr verlinkt, aber existieren

Siehe Video später

XYZLassi · 2018-02-27T10:07:28Z

OctoAwesome/OctoAwesome.Basics/ComplexPlanet.cs

-        }
-
-        private void Initalize()
-        {


Warum Initalize entfernen ???

Hast recht der muss wieder rein!

XYZLassi · 2018-02-27T10:13:50Z

OctoAwesome/OctoAwesome.Basics/Entities/WauziEntity.cs

+
+            public WauziTestController()
+            {
+            }


Zu heavy, sollte mehr aufgesplittet sein. Zum Beispiel Slected Sides

Verstehe zwar nicht was du mir sagen willst, aber wenn es das ist was ich denke das es ist.

Nenene, das Interace bietet eine generalisiert Möglichkeit eine Entität mit dem Spieler als auch einer KI zu steuern. Hallte es nicht für Sinnvoll für jeden möglichen UserInput eine eigene Klassen oder Interface zu schreiben.
Ich sage, es ist nicht Heavy genug...

XYZLassi · 2018-02-27T10:15:26Z

OctoAwesome/OctoAwesome.Basics/Entities/WauziEntity.cs

+        public float Radius => 1;
+        private IEntityController currentcontroller;
+        private float jumptime;
+        public WauziEntity() : base(true)


Eigenschaften nicht änderbar

Das ist die konkrete Implementierung einer einer Entity für Testzwecke die außerdem in Entwicklung ist.

XYZLassi · 2018-02-27T10:16:19Z

OctoAwesome/OctoAwesome.Basics/Entities/WauziEntity.cs

-            Components.AddComponent(new BoxCollisionComponent());
-            Components.AddComponent(new ControllableComponent());
-            Components.AddComponent(new RenderComponent() { Name = "Wauzi", ModelName = "dog", TextureName = "texdog", BaseZRotation = -90 }, true);
+            this.currentcontroller = controller;


Bitte Componenten lassen

XYZLassi · 2018-02-27T10:17:49Z

OctoAwesome/OctoAwesome.Basics/EntityComponents/BoxCollisionComponent.cs

+
+                        var blockplane = CollisionPlane.GetBlockCollisionPlanes(pos, movecomp.Velocity);
+
+                        var planes = from pp in playerplanes


Das kommt noch von mir und hat Bestandschutz, ist nur kopiert

XYZLassi · 2018-02-27T10:33:02Z

OctoAwesome/OctoAwesome/Chunk.cs

+            int flatindex = GetFlatIndex(x, y, z);
+            ushort block = Blocks[flatindex];
+            if (removeblock) SetBlock(flatindex, 0);
+            return block;


Warum remove

Immer noch der Rattenschwanz von oben ^^

XYZLassi · 2018-02-27T10:33:20Z

OctoAwesome/OctoAwesome/CodeExtensions/Vector2Extension.cs

+            return new Vector3(vector.X, vector.Y);
+        }
+    }
+}


Gut gefällt

XYZLassi · 2018-02-27T10:35:11Z

OctoAwesome/OctoAwesome/Entities/IEntityDefinition.cs

+        /// </summary>
+        Vector3 Body { get; }
+    }
+}


IEntity Definition ist zu speziell

Die ist noch nicht fertig ...

XYZLassi · 2018-02-27T10:36:26Z

OctoAwesome/OctoAwesome/ILocalChunkCache.cs

+        /// <param name="removeblock">Entfernt den Block.</param>
+        /// <returns>Die Block-ID an der angegebenen Koordinate</returns>
+        ushort GetBlock(int x, int y, int z, bool removeblock);
+


Warum remove

XYZLassi · 2018-02-27T10:37:59Z

OctoAwesome/OctoAwesome/OctoAwesome.csproj

-    <Compile Include="EntityComponents\PositionComponent.cs" />
-    <Compile Include="EntityComponents\ToolBarComponent.cs" />
-    <Compile Include="EntityFilterAttribute.cs" />
-    <Compile Include="Extension.cs" />


Sind immer noch im Projekt, aber nicht verlinkt

Also bei mir sind sie weg :)

XYZLassi · 2018-02-27T12:22:26Z

Hier noch das entsprechende Review-Video:https://www.youtube.com/watch?v=bRm7qTHiKqA

TODOs entfernt.

HierGibtEsDrachen · 2018-02-27T17:59:44Z

Also nach dem sehr guten Feedback überarbeite ich das jetzt nochmal und Merge es vorher lokal in die Develope.

ManuelHu · 2018-02-28T17:03:54Z

In diesem Pull Request sind einige Fragen zu den Conding Styles aufgetaucht. Ich habe gerade diese Fälle, jeweils wie @Gallimathias geschrieben hatte, in das Wiki eingepflegt.

Klaus A and others added 8 commits February 25, 2018 03:40

Refactoring, Entity- and Componentsystem

7d7b05e

some litle cleanups

3ae73a3

designe entscheidung wurde getroffen

5b902a0

component system wächst

UI interfaces

c680bd6

CleanUp

fe47832

Extensions and Entity refactored

63f6ee8

smal clean up

e49eeba

Merge branch 'feature/multiplayer' into feature/multiplayer

e415df5

Gallimathias requested changes Feb 27, 2018

View reviewed changes

Gallimathias self-assigned this Feb 27, 2018

Gallimathias added the input needed It waits for the answer to queries. label Feb 27, 2018

XYZLassi self-requested a review February 27, 2018 07:16

Gallimathias requested changes Feb 27, 2018

View reviewed changes

ManuelHu requested changes Feb 27, 2018

View reviewed changes

Gallimathias assigned XYZLassi Feb 27, 2018

XYZLassi requested changes Feb 27, 2018

View reviewed changes

Clean up

901527d

TODOs entfernt.

HierGibtEsDrachen changed the title :D Überarbeitung von Entities, Komponenten und Extensions Feb 27, 2018

Klaus A added 2 commits February 27, 2018 17:22

Merged base + clean up

d67aa39

Stabel ~

2f602b7

HierGibtEsDrachen closed this Feb 27, 2018

Gallimathias removed the input needed It waits for the answer to queries. label Feb 27, 2018

HierGibtEsDrachen deleted the feature/multiplayer branch March 5, 2018 21:45


		private ToolBarComponent toolbar;

		public Label activeToolLabel;


		extensionLoader = new ExtensionLoader(settings);

		ExtensionLoader extensionLoader = new ExtensionLoader(settings);

		@@ -84,8 +76,23 @@ public OctoGame() : base()

		Assets = new AssetComponent(this);
		Components.Add(Assets);


		var blockplane = CollisionPlane.GetBlockCollisionPlanes(pos, movecomp.Velocity);

		var planes = from pp in playerplanes

Überarbeitung von Entities, Komponenten und Extensions #245

Überarbeitung von Entities, Komponenten und Extensions #245

Conversation

HierGibtEsDrachen commented Feb 27, 2018

Gallimathias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HierGibtEsDrachen Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gallimathias Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HierGibtEsDrachen Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XYZLassi commented Feb 27, 2018

Gallimathias left a comment

Choose a reason for hiding this comment

Gallimathias Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

HierGibtEsDrachen Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HierGibtEsDrachen Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XYZLassi commented Feb 27, 2018

XYZLassi Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HierGibtEsDrachen Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XYZLassi commented Feb 27, 2018

HierGibtEsDrachen commented Feb 27, 2018

ManuelHu commented Feb 28, 2018

HierGibtEsDrachen Feb 27, 2018 •

edited

Loading

Gallimathias Feb 27, 2018 •

edited

Loading

HierGibtEsDrachen Feb 27, 2018 •

edited

Loading

Gallimathias Feb 27, 2018 •

edited

Loading

HierGibtEsDrachen Feb 27, 2018 •

edited

Loading

HierGibtEsDrachen Feb 27, 2018 •

edited

Loading

XYZLassi Feb 27, 2018 •

edited

Loading

HierGibtEsDrachen Feb 27, 2018 •

edited

Loading