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

SmartComponent interface for change-tracking functionality #83

Closed
wants to merge 10 commits into from

Conversation

sdleffler
Copy link
Contributor

This PR adds a SmartComponent trait which allows a user to implement their own callbacks for mutable/immutable component access. See https://github.com/sdleffler/sludge/blob/master/src/ecs.rs for a WIP example of its usage.

@sdleffler
Copy link
Contributor Author

With a bit of screwery, I've isolated a couple of changes from this PR which should be their own separate PRs (Archetype::component_types and World::resolve_unknown_gen.)

@Ralith
Copy link
Owner

Ralith commented Oct 17, 2020

@cart I'm interested in your thoughts on this, re: the path towards convergence for bevy_ecs and hecs. Would this be a suitable foundation for bevy's change detection feature?

@sdleffler
Copy link
Contributor Author

I'd honestly like to add some more stuff to this, as well - specifically it would also be nice to have on_insert and on_remove callbacks. I've spent a little time thinking about how that could be done lately, but not enough to try implementing it.

@cart
Copy link

cart commented Oct 17, 2020

@Ralith: at first glance I think it could probably work (functionally), but I have a few thoughts:

Bevy stores tracking state directly in each archetype. This has a few advantages:

  1. We don't need to "re-correlate" to entity tracking state when DerefMut is called on a pointer. The approach in this pr requires correlating an Entity+ComponentType to tracking state on each DerefMut, which would be decently expensive (especially if something is mutated multiple times).
  2. State is stored in contiguous arrays in the same order as the archetype's components. this means iteration is cache friendly. Doing the same in a separate Context would require a lot of "syncing" with the World's archetype state.
  3. Each "tracking pointer" directly stores a pointer to the tracking state. This means mutating the state is basically free. The approach in this pr involves an additional function call and the extra lookups mentioned in (1)

I'm a bit worried that the flexibility/agnosticism that this pr adds comes at the cost of performance. I'd want to see benchmarks of the following before converting to this approach:

  1. hecs (current master) vs hecs (this pr without any tracking context implemented)
  • this would check for significant regressions in baseline performance
  1. hecs (current master) vs hecs (this pr w/ tracking state implemented).
  • this would illustrate how expensive "real" tracking is relative to the results in (1)
  1. hecs (this pr w/ tracking state and queries implemented AND Bevy perf improvements upstreamed) vs bevy_ecs
  • this would roughly illustrate how the two implementations compare

I think Bevy's tracking approach is close to optimal (perf-wise) and I think it will be hard to reach that performance without being more opinionated + inflexible.

But its also possible that these concerns won't hold water in practice. Thats why we should measure.

@sdleffler
Copy link
Contributor Author

sdleffler commented Oct 17, 2020

Anecdotally we've already checked hecs (current master before a few pull requests I implemented which did not touch queries) against this PR; it's found that the IDs and extra data in each reference is inlined/eliminated by optimizations, and the benchmark times were identical. In addition, the "extra" function call is monomorphized by the compiler due to being non-dynamically dispatched and suitably inlined, so I don't see how an extra function call would really get in there.

As for tracking state itself, it's undoubtedly more expensive than bevy's approach, but when it comes to most usage of an ECS, the vast majority of components will be untracked anyways.[citation needed] Bevy's approach certainly has an advantage in being able to directly manipulate the tracking state without having to worry about sharing/concurrency - in my use case I have to use an atomic update to safely track a change...

@sdleffler
Copy link
Contributor Author

It could be interesting to allow per-archetype stored state in hecs...

@cart
Copy link

cart commented Oct 17, 2020

but when it comes to most usage of an ECS, the vast majority of components will be untracked anyways

I think this depends on perspective. In Bevy, we want all changes to be tracked because we can't know ahead of time what a user will be interested in. Because the tracking is so cheap, we were comfortable enabling it by default everywhere. The biggest cost here is the initial allocation of tracking state for each entity, which again, was worth it to me.

That being said, the needs of a general purpose engine are different than the needs of a game where the user owns the full stack / prefers to optimize based on their own use cases. I'm sure some people won't want to pay a cheaper, but universal cost for every component. They might prefer a slightly higher cost for the few components that need it (or no cost + no tracking).

Bevy's approach is only optimal for bevy's use cases + constraints 😄

@sdleffler
Copy link
Contributor Author

In Bevy, we want all changes to be tracked because we can't know ahead of time what a user will be interested in.

I see, in that case your implementation makes perfect sense! I currently use a wrapper around hecs, in its entirety, for my project, and one of the things I do is supply a change-tracking interface that can be selectively implemented for a component, in that any given component may or may not register modifications but can still receive insert/remove events. This rather modular approach also allows that in a way I think is interesting.

@Ralith
Copy link
Owner

Ralith commented Oct 17, 2020

Interesting thoughts, thanks!

If I understand correctly, bevy's approach is to more or less treat tracked state as a component, with all the associated benefits. I agree that's probably optimal assuming a sufficient proportion of states are touched. Decoupled tracking state, as currently reflected in this PR, could work better for very sparse access patterns (for both read and write), but is indeed probably the wrong default when tracking literally everything.

That said, the insight that bevy's state is functionally just another component, gives me an idea. What if a smart pointer with dense per-entity state is just a special type of Query? Queries can already be freely nested, and a smart pointer could be expressed as a query that borrows the actual T component along with something like a struct Modified<T>(bool, PhantomData<T>) component for the state.

@cart
Copy link

cart commented Oct 17, 2020

Ooh thats very interesting. It does have some cons:

  1. puts the burden on the user to add the relevant tracking components at insertion time. bevy could abstract this out, but that leads me to (2)
  2. something like a Bundle would either need to include the tracking in the bundle (manual + needs to anticipate the users needs ahead of time) or have an archetype change immediately after the initial insertion.

But the pros are definitely there too:

  1. significantly reduces the amount of extra "special case" tracker book-keeping code in hecs (which really is identical to components)
  2. opens up the possibility of basically any custom "tracking" type

@Ralith
Copy link
Owner

Ralith commented Oct 17, 2020

I believe adapter types could be (with some effort) defined that transform bundles and queries to versions where each component involved is annotated with tracking data, which would make hiding that in an abstraction practical.

@Ralith Ralith mentioned this pull request Jul 19, 2021
@Ralith
Copy link
Owner

Ralith commented Feb 4, 2024

Closing in favor of #366, which is more ergonomic, lower impact, and should perform better in most cases.

@Ralith Ralith closed this Feb 4, 2024
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

Successfully merging this pull request may close these issues.

3 participants