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

Removing Parent / Children from the world and then reinserting them can break hierarchies #15575

Open
alice-i-cecile opened this issue Oct 1, 2024 · 1 comment
Labels
A-ECS Entities, components, systems, and events A-Hierarchy Parent-child entity hierarchies A-Transform Translations, rotations and scales C-Bug An unexpected or incorrect behavior D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR

Comments

@alice-i-cecile
Copy link
Member

Problem

Within the internals of the transform propagation systems, we rely on hierarchies being well-formed (via #4197) for soundness, and panic if they are not in this line. This was added in #4775.

This is why we haven't added simple constructors or mutable access to Parent or Children (like Clone or Copy), like in #15574. If we can ever swap or set the value of Parent or Children directly, it becomes very easy to break the hierarchy. Unfortunately, this isn't the only way to get that problem.

Path 1: get mutable access to two different components via Query::get_many_mut or the like, and then mem_swap them.

Path 2: remove Parent / Children from an entity via a method on World that returns the removed value

Potential solutions

  1. Wait for Entity-entity relations 🌈  #3742.
  2. Implement Use component lifecycle hooks to make Parent + Children hierarchy code simpler and more robust #12235 in an airtight way.
  3. Make bevy_transform fully safe and eat the performance hit and correctness problems if people break their hierarchy by mistake.
  4. Add more, niche, high complexity features to absolutely forbid mutable access to some component types. Somehow.

I prefer 2, and would be opposed to both 3 and 4.

Additional context

Because this is actually guarded against by an assert, I think that merging #15574's successor is actually fine (cc @atornity, sorry).

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events A-Transform Translations, rotations and scales D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Design This issue requires design work to think about how it would best be accomplished A-Hierarchy Parent-child entity hierarchies X-Controversial There is active debate or serious implications around merging this PR labels Oct 1, 2024
@bushrat011899
Copy link
Contributor

#15635 can fully address invalidation caused by path 2 (adding or removing Parent/Children) but path 1 (mem::swap) will require something beyond what ComponentHooks currently offer, since there is no hook for mutable access.

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-Hierarchy Parent-child entity hierarchies A-Transform Translations, rotations and scales C-Bug An unexpected or incorrect behavior D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

2 participants