Skip to content

Update propogate_transforms to only visit the transforms of entities that have changed #17181

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

Open
BenjaminBrienen opened this issue Jan 6, 2025 · 2 comments
Labels
A-ECS Entities, components, systems, and events A-Transform Translations, rotations and scales D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@BenjaminBrienen
Copy link
Contributor

As far as I know yes, the transform propagation algorithm hasn't been changed since posting this. In order to alleviate the performance concerns we'd need two changes:

  1. Update propogate_transforms to only visit the transforms of entities that have changed. Currently we visit the entire hierarchy on every run of the system which is very inefficient (this is sub-optimal regardless of whether we update transforms in FixedUpdate, but this issue would exacerbate it)
  2. We'd need a way of sharing change ticks between multiple instances of propogate_transforms. Currently, if you were to add propogate_transforms to the schedule multiple times, each instance of the system would have its own change ticks, which means each instance would redundantly propagate transforms for the same entities/components

Originally posted by @JoJoJet in #7836 (comment)

item 2 can be a follow-up after this issue is handled

@BenjaminBrienen BenjaminBrienen added A-ECS Entities, components, systems, and events A-Transform Translations, rotations and scales D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Jan 6, 2025
@joseph-gio
Copy link
Member

Reposting my comment from the original issue for visibility (pun not intended)

See also #9497, which updated the visibility propagation algorithm to only visit entities that were changed. That exact approach may not be ideal for transforms though, since it can result in redundant propagations for a single entity within the same frame. For visibility, we figured that the tradeoff was worth it since propagating visibility is relatively cheap, and the cost of visiting the entire hierarchy tends to be more expensive than propagating changes entities. Transform propagation is computationally more expensive so it's not clear if that same tradeoff would be worth it.

@NthTensor
Copy link
Contributor

NthTensor commented Jan 11, 2025

Yeah, the naive approach used in #9497 is going to cause problems for transform propagation. It's not just non-ideal to propagate multiple times, I believe it would be potential UB. Since transform propagation is highly parallel, double propagation could result in conflicting mutable access.

The method used by big_space should be adapted here. I've been planning to do this for a while, and will probably do so shortly after cart's relations stuff merges.

@NthTensor NthTensor added S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Transform Translations, rotations and scales D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

3 participants