Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bevy_reactor_signals open issues #16

Open
viridia opened this issue May 22, 2024 · 0 comments
Open

bevy_reactor_signals open issues #16

viridia opened this issue May 22, 2024 · 0 comments

Comments

@viridia
Copy link
Owner

viridia commented May 22, 2024

Although the bevy_reactor_signals crate is shaping up very nicely overall, there are some critical areas where I think that the design is less than optimal, and where I have not yet been able to come up with a suitable solution. This ticket will enumerate these areas.

  • A lot of the design choices around reactions are based on intuitive notions of what is performant in ECS - such as whether it's faster to lookup an entity in a query by id vs. caching the results in a separate list, whether a single query with optional components is faster or slower than multiple separate queries and so on. Unfortunately, I have not attempted to measure any of this, and so my guesses are most likely wrong in a lot of cases.

  • TrackingScope has too many public members. The reason for opening up access to the public is to allow external crates to define their own reaction traits, and reactions need access to the internals of TrackingScope. In particular, View is a type of reaction which is different from Reaction, but needs to be able to do the kinds of things a reaction does: run cleanup handlers, add and remove dependencies, and so on. We need to move some of this logic from the reaction handlers into methods of TrackingScope itself.

  • By the same token, there's a lot of common code between run_reactions and run_view_reactions which ought to be combined. Unfortunately one of the problems with Rust is that one is often forced to refactor - or forced to refrain from refactoring - in order to satisfy the borrow checker. This seems like one of those cases.

  • The reaction handlers for TrackingScope do far too many query lookups. In run_reactions and run_view_reactions, each time through the inner loop, we look up the same entity in the query three separate times. The reason for this is to satisfy the borrow checker: each time we get a copy of the data from the query, and then use that data along with the reference to the World to do come computation. Because the query also contains a reference to the world, and because this reference is mutable, we can't hold on to the query data and use the World reference at the same time. So to insure that the query data has a short lifetime, we have to look up the query multiple times.

    The flow of code looks roughly like this (pseudo-code):

    - for each tracking scope that has changed dependencies:
      - use the entity to lookup the tracking scope, and get the list of cleanup handlers.
      - run the cleanup handlers using the `World`.
      - use the entity to lookup the reaction trait.
      - run the reaction using the `World`.
      - use the entity to lookup the tracking scope again.
      - replace the dependencies in the tracking scope (using mem::take) with the dependencies generated while running the reaction.
    

    Unfortunately, the various Bevy solutions for gaining multiple references to the world won't work here, since those don't help when you have references to multiple components on the same entity while at the same time holding a reference to World. The only thing I can think of that would work is some kind of deferred world mutation solution, but that doesn't exist yet, and I'm not sure what the side-effects would be.

  • The system of dependency checking relies entirely on Bevy's change detection system. This has three drawbacks:

    • It's not clear how to depend on changing data that lives outside of Bevy's World. While we could add additional dependency tracking to every TrackingScope, such dependencies would need a clear understanding of the lifetimes of such dependencies. With Bevy components and resources, we have a clear way of knowing whether a given entity, component or resource still exists and we don't have to worry terribly much about reference lifetimes, assumptions which probably won't be true for data outside the world.
    • The change detection is relatively coarse-grained. This means that when working with resources or components that have a large, complex structure, we have to do a lot of memoization in order to prevent a storm of spurious reactions. Memos of course have their own costs, every memo generates a new Entity and a new Component to store the memoized data.
    • There's a long-standing user request to integrate query results into the reaction system. Unfortunately, queries don't have change detection because they are not long-lived. You can query for components that have changed, but you can't detect when the query results themselves have changed. Probably what we'll need to do in the long run is invent some kind of persistent "query-like" action which is not strictly a Bevy query, but which has some of the same abilities.
  • The reaction debug tracing system as currently formulated is both incorrect and fragile - if systems are run in the wrong order it will break. We should construct the entire debug list immediately before running any reaction systems, because reactions might want to reference the debug list (such as a debug display UI). The problem here, unfortunately, is that checking for changed dependencies is expensive (as it involves iterating through resource and component ids stored in the tracking scope) and should only be done once per tracking scope, which is why we currently combine the logic for recording debug trace information and handling reactions in the same system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant