-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Change ChildOf to Childof { parent: Entity} and support deriving Relationship and RelationshipTarget with named structs #17905
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
Conversation
589c7cc to
2fe51a0
Compare
No, that's unpleasant, and trivially worked around. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work :) Thanks a ton for finishing this up; you're much more adept with the macros than I am!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! Thanks for putting this together!
#17905 replaced `ChildOf(entity)` with `ChildOf { parent: entity }`, but some deprecation advice was overlooked. Also corrected formatting in documentation. ## Testing Added a `set_parent` to a random example. Confirmed that the deprecation warning shows and the advice can be pasted in.
# Objective In #17905 we swapped to a named field on `ChildOf` to help resolve variable naming ambiguity of child vs parent (ex: `child_of.parent` clearly reads as "I am accessing the parent of the child_of relationship", whereas `child_of.0` is less clear). Unfortunately this has the side effect of making initialization less ideal. `ChildOf { parent }` reads just as well as `ChildOf(parent)`, but `ChildOf { parent: root }` doesn't read nearly as well as `ChildOf(root)`. ## Solution Move back to `ChildOf(pub Entity)` but add a `child_of.parent()` function and use it for all accesses. The downside here is that users are no longer "forced" to access the parent field with `parent` nomenclature, but I think this strikes the right balance. Take a look at the diff. I think the results provide strong evidence for this change. Initialization has the benefit of reading much better _and_ of taking up significantly less space, as many lines go from 3 to 1, and we're cutting out a bunch of syntax in some cases. Sadly I do think this should land in 0.16 as the cost of doing this _after_ the relationships migration is high.
# Objective In #17905 we swapped to a named field on `ChildOf` to help resolve variable naming ambiguity of child vs parent (ex: `child_of.parent` clearly reads as "I am accessing the parent of the child_of relationship", whereas `child_of.0` is less clear). Unfortunately this has the side effect of making initialization less ideal. `ChildOf { parent }` reads just as well as `ChildOf(parent)`, but `ChildOf { parent: root }` doesn't read nearly as well as `ChildOf(root)`. ## Solution Move back to `ChildOf(pub Entity)` but add a `child_of.parent()` function and use it for all accesses. The downside here is that users are no longer "forced" to access the parent field with `parent` nomenclature, but I think this strikes the right balance. Take a look at the diff. I think the results provide strong evidence for this change. Initialization has the benefit of reading much better _and_ of taking up significantly less space, as many lines go from 3 to 1, and we're cutting out a bunch of syntax in some cases. Sadly I do think this should land in 0.16 as the cost of doing this _after_ the relationships migration is high.
Objective
fixes #17896
Solution
Change ChildOf ( Entity ) to ChildOf { parent: Entity }
by doing this we also allow users to use named structs for relationship derives, When you have more than 1 field in a struct with named fields the macro will look for a field with the attribute #[relationship] and all of the other fields should implement the Default trait. Unnamed fields are still supported.
When u have a unnamed struct with more than one field the macro will fail.
Do we want to support something like this ?
I could add this, it but doesn't seem nice.
Testing
crates/bevy_ecs - cargo test
Showcase