Skip to content

Conversation

@Ralith
Copy link
Owner

@Ralith Ralith commented Feb 4, 2024

Much like CommandBuffer and serialization support, I think this is non-obvious, useful, and self-contained enough to merit inclusion even though it's implemented purely in terms of the public API. I'm interested in feedback on whether this would indeed be useful to people, and on the API design, including the detailed semantics.

@kanerogers
Copy link

My 2 cents

What I like:

  • It's opt-in
  • Self-contained
  • Documentation on track is excellent
  • Conceptually simple

What I don't like:

  • Some module-level documentation would help with visibility (happy to volunteer for this)

@Ralith
Copy link
Owner Author

Ralith commented Feb 4, 2024

module-level documentation

The module isn't public; I'm just reexporting ChangeTracker and Changes at the top level. I'd welcome a succinct doctest for ChangeTracker to illustrate its use, though.

@h3r2tic
Copy link

h3r2tic commented Feb 26, 2024

Looks quite handy, and I like how small and clear an implementation this is! FWIW our project enjoys change tracking in Bevy via DerefMut marking components dirty (as opposed to running PartialEq). It most cases it wouldn't matter much, but at least in one one case the component involved is a chonky one, and I suspect that with this implementation, we'd need to layer an additional dirtiness tracking thing on top in order to avoid the cost of PartialEq.

I suspect there could be a simple workaround; we stash a whole CPU-side collision mesh into a component, and use change detection to trigger its upload to the physics engine. We could probably keep a change counter or a content hash in a separate component, and make the PartialEq run quickly anyway. For hecs you could probably just say in a doc "yo don't do change detection on fat structs or else", and recommend a silly workaround like this.

@Ralith Ralith enabled auto-merge (rebase) February 27, 2024 01:03
@Ralith Ralith merged commit e2e9499 into master Feb 27, 2024
@Ralith Ralith deleted the change-tracker branch February 27, 2024 01:03
@Ralith Ralith mentioned this pull request Mar 16, 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.

5 participants