bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.09k stars 3.56k forks source link

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

Open alice-i-cecile opened 8 months ago

alice-i-cecile commented 8 months ago

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.

iiYese commented 8 months ago

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

alice-i-cecile commented 8 months ago

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

james-j-obrien commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 5 months ago

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 commented 3 months ago

EDIT: Tabbed over to the wrong issue!

NthTensor commented 1 month ago

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

bushrat011899 commented 4 weeks ago

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