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

Use component lifecycle hooks to make Parent + Children hierarchy code simpler and more robust #12235

Open
alice-i-cecile opened this issue Mar 1, 2024 · 10 comments
Labels
A-ECS Entities, components, systems, and events A-Hierarchy Parent-child entity hierarchies C-Code-Quality A section of code that is hard to understand or change D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Milestone

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

bevy_hierarchy's internals are complex, performance-critical, and have panicking unsafe code in them.
The core reason for this is that we cannot let the graph become invalid: this leads to wasted work, subtle bugs and huge amounts of user frustration. And because we use unsafe code internally that relies on the assumption of validity for soundness, this can become a soundness problem.

They're painful to maintain, and nearly impossible to replicate externally for other hierarchy-like code that wants to uphold these same variants.

What solution would you like?

#10756 adds component lifecycle hooks: operations that take effect when components are added or removed.

We should replace the existing complex code in bevy_hierarchy with this idiom, being sure to benchmark and optimize performance.

What alternative(s) have you considered?

We could wait until the fabled day that relations (#3742) finally gets shipped.

Additional context

This has been widely anticipated as one of the key benefits of that PR: I'm recording it here to make sure we don't forget (and can coordinate work).

While the user-facing API is also pretty gnarly and inconsistent, I'd like to leave that for future work to make sure that the correctness-sensitive internals get the review attention it needs.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Complex Quite challenging from either a design or technical perspective. Ask for help! A-Hierarchy Parent-child entity hierarchies labels Mar 1, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Mar 1, 2024
@iiYese
Copy link
Contributor

iiYese commented Mar 1, 2024

Is this just using hooks for cleanup? Like removing dangling, despawn recursive, etc?

@alice-i-cecile
Copy link
Member Author

Yeah, I think that's about all we'll be able to do currently.

@james-j-obrien
Copy link
Contributor

If we don't allow users to mutably access the components directly we can enforce an always up to date hierarchy by only allowing commands to modify it. Although this would be an access pattern that I don't believe bevy currently uses anywhere.

@iiYese
Copy link
Contributor

iiYese commented Mar 1, 2024

That's what I do for aery. Private Component with public QueryData. I can't think of any use cases where people would be modifying hierarchy edges manually but if there are it should be a straight forward migration because automatically inserted sync points exist now.

@tim-blackbird
Copy link
Contributor

I did most of the work for this a while ago as I was testing the hooks PR.
The current state of hooks makes it a little unfortunate but we can discuss that when I have the PR ready

@alice-i-cecile
Copy link
Member Author

Awesome, thank you @irate-devil :) And yeah, ironing out the kinks of the component hooks API is a big part of why I'm keen to do this.

@iiYese
Copy link
Contributor

iiYese commented May 16, 2024

Doing this is going to require making Children & Parent private and exposing QueryData structs for them. Since entity.get::<Children>()/entity.get::<Parent>() will no longer be possible with this change #13375 should be merged first otherwise this causes breakage that's annoying to work around.

@ItsDoot
Copy link
Contributor

ItsDoot commented Jul 27, 2024

EDIT: Tabbed over to the wrong issue!

@NthTensor
Copy link
Contributor

What is the point of using hooks if we restrict access to commands? Can't we just do the cleanup in the commands?

@bushrat011899
Copy link
Contributor

The fix for this has been marked for 0.16, probably should do the same here.

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 C-Code-Quality A section of code that is hard to understand or change D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
Archived in project
Status: Todo
Development

Successfully merging a pull request may close this issue.

8 participants