search

Dungeon-CampusMinden / Dungeon

The "Dungeon" is a tool to gamify classroom content and integrate it into a 2D Rogue-Like role-playing game.
MIT License
15 stars 33 forks source link

Dungeon: add filter-stream operations to System #1565

Open cagix opened 1 week ago

cagix commented 1 week ago

aus https://github.com/Dungeon-CampusMinden/Dungeon/pull/1564#discussion_r1661030484:

Ein häufiges Pattern ist das Aufrufen der System#entityStream-Methode (oder nach dem Mergen von #1564 der System#filteredEntityStream-Methode) und Filtern aller Entitäten nach dem Vorhandensein einer bestimmten Component, um danach dann über alle Entitäten mit einer bestimmten Component zu itererieren. Dabei wird exakt diese Component anschließend extrahiert und damit dann irgendwas gemacht, ohne dass die Entität noch benötigt wird:

  public void execute() {
    filteredEntityStream(SpikyComponent.class)
        .forEach(e -> e.fetch(SpikyComponent.class).orElseThrow().reduceCoolDown());
  }

Es wäre einfacher, wenn in System auch ein Filter für Stream<T extends Component> angeboten würde für einfache Filter-Operationen.

Bei komplexeren Filtern (mehrere Components gesucht) müsste ein Stream über passenden Tuples gebildet werden.

Edit: Das wird in dem folgenden Beispiel (aus #1567) besonders deutlich:

    filteredEntityStream(SpikyComponent.class)
        .map((e -> e.fetch(SpikyComponent.class)))
        .flatMap((Optional::stream))
        .forEach(SpikyComponent::reduceCoolDown);

In diesem Fall ist offensichtlich, dass die Abstraktionen noch nicht passen: Es wird ja nicht nach den Entitäten gesucht, sondern ihren Components. Es sollte also ein Stream mit den richtigen Objekten, also den gewünschten Components geliefert werden, anstatt einen Stream mit den Container-Objekten zu erzeugen, aus dem man die Dinge dann nochmal separat extrahieren muss.

Edit: Analyse der Stream-Methoden in Game und System (warum sind die eigentlich nicht alle in einer Klasse?!):

Methode in wird aufgerufen in dort tatsächlich benötigt Bemerkung
Game: Stream entityStream() contrib.entities.AIFactory Entity, (HealthComponent, AIComponent) Eigentlich wird nur die Entität benötigt (welche die beiden Components haben muss) => Einsatz der falschen Filter-Methode!
contrib.entities.HeroFactory Entity, UIComponent
contrib.utils.components.interaction.InteractionTool InteractionComponent, PositionComponent
Game: Stream entityStream(final System system) YAGNI
Game: Stream entityStream(final Set<Class<? extends Component>> filter) dojo.rooms.rooms.riddle.QuaderRoom (PlayerComponent), HealthComponent Eigentlich wird nach dem Hero gesucht, um dessen Health verändern zu können => Einsatz der falschen Methode!
dojo.rooms.rooms.search.KillMonsterRoom (PlayerComponent), HealthComponent Eigentlich wird nach dem Hero gesucht, um dessen Health verändern zu können => Einsatz der falschen Methode! (2x)
System: Stream filteredEntityStream(final Set<Class<? extends Component>> filterRules) Weiterleitung
System: Stream filteredEntityStream(final Set<Class<? extends Component>> filterRules) System: Stream filteredEntityStream() Weiterleitung
System: Stream filteredEntityStream(final Class<? extends Component>... filterRules) Weiterleitung
System: Stream filteredEntityStream() contrib.systems.CollisionSystem Entity, CollideComponent
core.systems.LevelSystem PlayerComponent, PositionComponent Eigentlich wird nach dem Hero gesucht, um dessen PositionComponent abzufragen => Einsatz der falschen Methode!
core.systems.PositionSystem Entity, PositionComponent Eigentlich wird nur die PositionCompoment benötigt, um Felder mit anderen Entitäten zu erkennen
System: Stream filteredEntityStream(final Class<? extends Component>... filterRules) contrib.systems.AISystem Entity, AIComponent
contrib.systems.CollisionSystem Entity, CollideComponent
contrib.systems.HealthBarSystem (Entity?), HealthComponent, PositionComponent Wird die Entity wirklich benötigt?
contrib.systems.HealthSystem (Entity?), HealthComponent, DrawComponent Wird die Entity wirklich benötigt?
contrib.systems.HudSystem UIComponent
contrib.systems.IdleSoundSystem IdleSoundComponent, (Entity, PositionComponent) Eigentlich wird die PositionComponent gebraucht zur Bestimmung der Nähe zum Hero, und dann die IdleSoundComponent, ob hier ein IdleSound gespielt werden soll.
contrib.systems.ProjectileSystem Entity, ProjectileComponent, PositionComponent, VelocityComponent
contrib.systems.SpikeSystem SpikyComponent
core.systems.CameraSystem CameraComponent, PositionComponent
core.systems.DrawSystem DrawComponent, PositionComponent, (PlayerComponent)
core.systems.LevelSystem PlayerComponent, PositionComponent Hier wird eigentlich nach dem Hero gesucht => Falsche Methode.
core.systems.PlayerSystem Entity, PlayerComponent
core.systems.PositionSystem PositionComponent
core.systems.VelocitySystem Entity, VelocityComponent, PositionComponent, DrawComponent
AMatutat commented 1 week ago

Bei komplexeren Filtern (mehrere Components gesucht) müsste ein Stream über passenden Tuples gebildet werden.

Ich würde tatsächlich die Entität als diesen Tupel sehen. Für komplexere Konstrukte kann man dann einen Builder verwenden

Aus DrawSystem


  private DSData buildDataObject(final Entity entity) {
    DrawComponent dc =
        entity
            .fetch(DrawComponent.class)
            .orElseThrow(() -> MissingComponentException.build(entity, DrawComponent.class));
    PositionComponent pc =
        entity
            .fetch(PositionComponent.class)
            .orElseThrow(() -> MissingComponentException.build(entity, PositionComponent.class));
    return new DSData(entity, dc, pc);
  }

  private record DSData(Entity e, DrawComponent dc, PositionComponent pc) {}
cagix commented 1 week ago

im prinzip bin ich da bei dir.

aber bei entities bin ich prinzipiell unsicher, ob eine component vorhanden ist oder nicht, deshalb ja auch das optional in der rückgabe. beim filtern nun die gesuchten components in eine neue entität zu packen löst uns leider nicht von dem sperrigen umgang mit optional in java, auch wenn wir hier in diesem konkreten fall eigentlich wissen, dass die component/s da ist/sind ...

und man könnte sich auch fragen, warum man hier in diesem fall nicht einfach die ursprüngliche entity zurück liefert...

vielleicht macht das spezielle filtern nach components nur sinn für den fall, dass man nur nach einer component sucht? selbst wenn man eine einfache tupel- oder pair-klasse hätte, das problem mit der reihenfolge bliebe bei komplexeren abfragen. (obwohl man die gefundenen components darin so anordnen könnte wie im filteraufruf übergeben.)

AMatutat commented 1 week ago

dem sperrigen umgang mit optional in java, auch wenn wir hier in diesem konkreten fall eigentlich wissen, dass die component/s da ist/sind

Eigentlich müsste man hier Ansetzen, oder nicht? Entity#forceFetch(Component.class): Component (also ohne Optional) und dann den Aufrufer in die Pflicht nehmen. Aber in der Praxis wird das dann auch verwendet, wenn eigentlich die Optional Variante richtig wäre, weil Entwickler==Faul.

cagix commented 1 week ago

dem sperrigen umgang mit optional in java, auch wenn wir hier in diesem konkreten fall eigentlich wissen, dass die component/s da ist/sind

Eigentlich müsste man hier Ansetzen, oder nicht? Entity#forceFetch(Component.class): Component (also ohne Optional) und dann den Aufrufer in die Pflicht nehmen. Aber in der Praxis wird das dann auch verwendet, wenn eigentlich die Optional Variante richtig wäre, weil Entwickler==Faul.

Aus meiner Sicht ist das nicht wirklich der richtige Schritt, da wir hier verschiedene "Kreise" haben:

  1. Entity als solche ("Entität im Spiel"): Es ist unsicher, ob diese Entity eine bestimmte Component besitzt. In diesem Use-Case ist es sinnvoll, beim fetch() mit Optional zu arbeiten (statt in alter schlechter Java-Manier ggf. stumpf null zu liefern oder (noch schlimmer) mit Exceptions um sich zu werfen).

  2. Filtern nach Eigenschaften im System: Hier kommt ein Stream zurück, der bestimmte Eigenschaften der Stream-Elemente "garantiert" (naja, so lange das korrekt implementiert wurde).

    Wenn ich einen Stream<Entity> liefere, dann habe ich an der Stelle die Zusage über die Typen, dass hier nur Entity-Objekte drin sind.

    Wenn ich aber nach Components suche, sollte ich

    • entweder (wenn ich nur nach einer Component suche) einen Stream<ComponentXYZ> liefern - dann kann ich als Kunde direkt damit arbeiten, oder
    • alternativ (wenn ich nach mehreren Components in Kombination suche) eine passende Komposition dieser Components in den Stream packen.

    Entity klingt dabei zunächst irgendwie naheliegend, da hier genau diese Gruppierung bereits gemacht wird, ist aber für mich die falsche Abstraktion: Ich möchte einen Container, in dem ich verlässlich auf Dinge zugreifen kann, weil ich bereits weiss, dass sie da sind. Bei Entity weiss ich das eben nicht. Und hier einen vermeintlichen Shortcut über ein forceFetch einzubauen führt nur (früher oder später) zu neuen Problemen und grässlichem Code - die Leute werden das dann nämlich nutzen und die ganzen Vorteile von Optional gehen flöten.

cagix commented 1 week ago

Nach der Analyse der Verwendung brauchen wir aktuell für die Filter drei verschiedene parametrische (innere) Records zum Halten der gewünschten Daten:

record A(Entity entity, T component)
record B(Entity entity, T1 component1, T2 component2)
record C(Entity entity, T1 component1, T2 component2, T3 component3)

(mit passend beschränkten Typ-Variablen)

Diese Record-Klassen könnten als innere statische Klassen in System definiert werden und dann in drei unterschiedlichen Filtermethoden zurückgeliefert werden:

Stream<A<T>> filteredEntityStream(T Component)
Stream<B<T1, T2>> filteredEntityStream(T1 Component, T2 Component)
Stream<C<T1, T2, T3>> filteredEntityStream(T1 Component, T2 Component, T3 Component)
cagix commented 6 days ago

Hmmm. Lustig: In HealthSystem modifizieren wir im Stream den Stream selbst:

  public void execute() {
    filteredEntityStream(HealthComponent.class, DrawComponent.class)
        ...
        .forEach(this::removeDeadEntities);
  }
  private void removeDeadEntities(final HSData hsd) {
      ...
      Game.remove(hsd.e);
  }

Der Stream geht in ECSManagment in der passenden activeEntityStorage los, und das remove ändert die Einträge in der activeEntityStorage... 😱

Eigentlich möchte man den Stream nach "lebendigen" und "toten" Entitäten partitionieren => zwei Mengen:

Das sollte aber dann auf diesen beiden Mengen jeweils erfolgen, nicht auf dem Stream aus filteredEntityStream ...

=> #1578

cagix commented 6 days ago

@AMatutat ich habe in #1579 mal "laut überlegt" und ein paar optionen durchgespielt.

option (1) ist die "traditionelle" variante, d.h. filteredEntityStream liefert einen Stream<Entity> zurück und man muss dann aus diesen entitäten selbst die nötigen components nochmal rausfischen (obwohl man eben erst danach gefiltert hat).

option (2) ist eine variante, die für eine, zwei oder drei components im filtervorgang in filteredEntityStreamX einen stream mit entsprechenden tupeln dieser components (plus der entität) zurückliefert. bei mehr als drei gesuchten components würde dann einfach wieder ein Stream<Entity> kommen und der kunde müsste sich das data object selbst bauen (oder eine neue überladung der filter-methode erstellen). im grunde greift das dem bauen der data objects in den ganzen systemen vor - das bräuchte man dort dann nicht mehr machen.

option (2a) ist eine variante von (2), bei der das neue pattern matching in java verwendet wird. leider geht das nur mit switch, d.h. beim "auspacken" braucht man eine eigene methode. und der switch möchte gern "exchaustive" sein, d.h. es sollte eine default-option dabei sein - und hier kommt dann wieder eine hässliche exception mit ins spiel.

option (3) ist eine variante, wo nur eine component gesucht und benötigt wird - hier wird dann direkt ein Stream<T extends Component> gebaut und die kunden können damit direkt arbeiten. aktuell trifft das aber nur auf wenige systeme zu, wobei man sich alle systeme eh nochmal gründlich anschauen sollte, was dort passiert (s.o. mit HealthSystem). vermutlich stellt sich dann heraus, dass man doch öfter als gedacht nur mit einer component hinkommt?

die spielereien sind alle in system - dort gehören sie aber vermutlich nicht hin und es sollte (wenn überhaupt) nach game umgezogen werden.

cagix commented 6 days ago

@AMatutat Ich bin grad am überlegen, ob die Rückgabe von Streams in Game/System wirklich eine gute Idee war. Aus Game/System müsste man eigentlich nur ein Set.of() herausreichen (dieses ist dann immutable und zudem eine Kopie der internen Datenstruktur), dann könnte man auf diesem Set als Kunde selbst streamen und hätte dann nicht das Problem wie im HealthSystem, dass man sich derweil der Stream noch über die Entitäten läuft diese bereits teilweise aus der Datenstruktur (aka Basis des Streams) löscht ...

AMatutat commented 6 days ago

Ich hinterlass hier mal ein "Ich habs gesehen" Kommentar. Hier muss ich mich mit ner dicken Tasse Kaffe nochmal reinwurschteln.