[Merged by Bors] - Split bevy_hierarchy out from bevy_transform #4168

alice-i-cecile · 2022-03-10T04:15:29Z

Objective

Hierarchy tools are not just used for Transform: they are also used for scenes.
In the future there's interest in using them for other features, such as visiibility inheritance.
The fact that these tools are found in bevy_transform causes a great deal of user and developer confusion
Fixes Should bevy-transform be two crates? #2758.

Solution

Split bevy_transform into two!
Make everything work again.

Note that this is a very tightly scoped PR: I know there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate.

Frustrations

The API around GlobalTransform is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance.

In the medium-term, I feel pretty strongly that GlobalTransform should be replaced by something like Inherited<Transform>, which lives in bevy_hierarchy:

avoids code duplication
makes the inheritance pattern extensible
links the types at the type-level
allows us to remove all references to inheritance from bevy_transform, making it more useful as a standalone crate and cleaning up its docs

Additional context

double-blessed by @cart in Hierarchy optimized systems and storage #4141 (comment) and Should bevy-transform be two crates? #2758 (comment)
preparation for more advanced / cleaner hierarchy tools: go read Consistent Hierarchies rfcs#53 !
originally attempted by @finegeometer in Transform hierarchy split #2789. It was a great idea, just needed more discussion!

alice-i-cecile · 2022-03-10T04:16:19Z

@cart as part of this PR's merge process, can we get a A - Hierarchy label?

crates/bevy_hierarchy/.gitignore

crates/bevy_hierarchy/src/hierarchy/hierarchy_maintenance_system.rs

crates/bevy_ui/src/lib.rs

alice-i-cecile · 2022-03-12T03:39:07Z

bors try

bors · 2022-03-12T03:47:26Z

try

Build failed:

check-doc

Cannot link fully because bevy_hierarchy is not a dependency :(

Reduces pointless importing boilerplate

This reverts commit 013836a.

IceSentry · 2022-03-12T20:45:35Z

btw, bevy_hierarchy is not reserved on crates.io. Even if this PR doesn't get merged, we should probably reserve it.

https://crates.io/crates/bevy_hierarchy

I made a PR for that bevyengine/bevy-crate-reservations#15

cart · 2022-03-15T01:39:53Z

The API around GlobalTransform is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance. In the medium-term, I feel pretty strongly that GlobalTransform should be replaced by something like Inherited, which lives in bevy_hierarchy

I object to the framing that a fully separate GlobalTransform component is a "mess". I personally think a named (and fully separate) GlobalTransform type is preferable to a more abstracted "component inheritance system". GlobalTransform having a separate type (and name) makes the context and "space" (as in "world space") of the component clear. It also leaves the door open to things like "adding a GlobalTransform::ignore_parent field".

On the other hand, Inherited<Transform> could literally mean anything (and would mean different things, depending on the context of Inherited + ComponentType). Of course, the value of this design is up for debate (and should be discussed in a different place), but by including "highly opinionated and negatively weighted" thoughts on what we already have, I feel compelled to reply here.

cart · 2022-03-15T01:41:10Z

(but I do agree that it would be nice to cut down on code duplication ... impl macros or Deref are also options)

cart

Looks good to me. Just one baby nit.

crates/bevy_transform/src/lib.rs

cart · 2022-03-15T01:53:50Z

bors r+

@cart

# Objective - Hierarchy tools are not just used for `Transform`: they are also used for scenes. - In the future there's interest in using them for other features, such as visiibility inheritance. - The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion - Fixes #2758. ## Solution - Split `bevy_transform` into two! - Make everything work again. Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate. ## Frustrations The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance. In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`: - avoids code duplication - makes the inheritance pattern extensible - links the types at the type-level - allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs ## Additional context - double-blessed by @cart in #4141 (comment) and #2758 (comment) - preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 ! - originally attempted by @finegeometer in #2789. It was a great idea, just needed more discussion! Co-authored-by: Carter Anderson <mcanders1@gmail.com>

alice-i-cecile · 2022-03-15T02:15:50Z

but by including "highly opinionated and negatively weighted" thoughts on what we already have, I feel compelled to reply here.

Fair; this wasn't really the place or tone for it. I'm sorry for that @cart. I was frustrated about the code duplication, but we can debate the relative merits of that design somewhere else.

bors · 2022-03-15T02:16:48Z

Pull request successfully merged into main.

Build succeeded:

cart · 2022-03-15T02:28:24Z

Fair; this wasn't really the place or tone for it. I'm sorry for that @cart. I was frustrated about the code duplication, but we can debate the relative merits of that design somewhere else.

No worries. I almost didn't say anything because its definitely ok to think something is a mess (although I try to avoid using that language to avoid hurting peoples' feelings ... and to account for the fact that I might find out I'm wrong about that assessment. When trying to get that point across, I think its better to just outline the specific facts of the situation rather than ascribe generic negative labels to things). I mainly brought it up because I'd rather leave "opinionated discussions and pronouncements about the value of a thing" to issues scoped to that topic. I selfishly don't want to defend my design choices in 10 different places :)

@cart

# Objective - Hierarchy tools are not just used for `Transform`: they are also used for scenes. - In the future there's interest in using them for other features, such as visiibility inheritance. - The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion - Fixes bevyengine#2758. ## Solution - Split `bevy_transform` into two! - Make everything work again. Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate. ## Frustrations The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance. In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`: - avoids code duplication - makes the inheritance pattern extensible - links the types at the type-level - allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs ## Additional context - double-blessed by @cart in bevyengine#4141 (comment) and bevyengine#2758 (comment) - preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 ! - originally attempted by @finegeometer in bevyengine#2789. It was a great idea, just needed more discussion! Co-authored-by: Carter Anderson <mcanders1@gmail.com>

…for hierarchies. Probably since bevyengine/bevy#4168

@cart

# Objective - Hierarchy tools are not just used for `Transform`: they are also used for scenes. - In the future there's interest in using them for other features, such as visiibility inheritance. - The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion - Fixes bevyengine#2758. ## Solution - Split `bevy_transform` into two! - Make everything work again. Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate. ## Frustrations The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance. In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`: - avoids code duplication - makes the inheritance pattern extensible - links the types at the type-level - allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs ## Additional context - double-blessed by @cart in bevyengine#4141 (comment) and bevyengine#2758 (comment) - preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 ! - originally attempted by @finegeometer in bevyengine#2789. It was a great idea, just needed more discussion! Co-authored-by: Carter Anderson <mcanders1@gmail.com>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 10, 2022

alice-i-cecile added A-Transform Translations, rotations and scales C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Mar 10, 2022

mockersf requested changes Mar 10, 2022

View reviewed changes

crates/bevy_hierarchy/.gitignore Outdated Show resolved Hide resolved

james7132 requested changes Mar 11, 2022

View reviewed changes

crates/bevy_hierarchy/src/hierarchy/hierarchy_maintenance_system.rs Outdated Show resolved Hide resolved

crates/bevy_ui/src/lib.rs Outdated Show resolved Hide resolved

bors bot added a commit that referenced this pull request Mar 12, 2022

Try #4168:

2a22f02

alice-i-cecile added 20 commits March 12, 2022 12:19

Clone bevy_transform contents into bevy_hierarchy

d9ed018

Remove Transform types from bevy_hierarchy

d843703

Remove hierarchy code from bevy_transform

54e43f0

Fix imports

b1c51a5

Add HierarchyPlugin to DefaultPlugins

a258ef9

Fix missing import

58de51d

Export bevy_hierarchy prelude in bevy::prelude

7138216

System labels belong to bevy_hierarchy

de8786d

Fix broken doc links

9eba280

Cannot link fully because bevy_hierarchy is not a dependency :(

Fix label imports

8559e58

Remove broken doc links

0484273

Fix broken doc link

c1f279b

Remove redundant .gitignore files

e65f14f

Move transform_propagate_system back to bevy_transform

9db9e08

Expose Transform and GlobalTransform at root of bevy_transform

78e2327

Reduces pointless importing boilerplate

Re-add doc links

a4fd724

Expose ChildBuilder traits in prelude

41bcf61

Revert "Expose Transform and GlobalTransform at root of bevy_transform"

5fb826c

This reverts commit 013836a.

Remove dead doc link

21849e1

Fix doc link

ffcea84

Remove unused dependencies

0deeede

alice-i-cecile force-pushed the bevy_hierarchy branch from cb027d7 to 0deeede Compare March 12, 2022 17:20

IceSentry mentioned this pull request Mar 12, 2022

add bevy_hierarchy bevyengine/bevy-crate-reservations#15

Closed

alice-i-cecile mentioned this pull request Mar 13, 2022

[Merged by Bors] - Hierarchy commandization #4197

Closed

2 tasks

james7132 approved these changes Mar 13, 2022

View reviewed changes

mockersf approved these changes Mar 14, 2022

View reviewed changes

mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 14, 2022

alice-i-cecile mentioned this pull request Mar 14, 2022

Reserve bevy_hierarchy bevyengine/bevy-crate-reservations#16

Closed

cart approved these changes Mar 15, 2022

View reviewed changes

crates/bevy_transform/src/lib.rs Outdated Show resolved Hide resolved

Update crates/bevy_transform/src/lib.rs

c0db93c

bors bot changed the title ~~Split bevy_hierarchy out from bevy_transform~~ [Merged by Bors] - Split bevy_hierarchy out from bevy_transform Mar 15, 2022

bors bot closed this Mar 15, 2022

zkat added a commit to zkat/big-brain that referenced this pull request Sep 24, 2022

fix(actions,scorers): Transform/GlobalTransform are no longer needed …

df10f03

…for hierarchies. Probably since bevyengine/bevy#4168

Kanabenki mentioned this pull request Sep 28, 2023

Tracking issue: deny missing docs, one crate at a time #3492

Open

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Split bevy_hierarchy out from bevy_transform #4168

[Merged by Bors] - Split bevy_hierarchy out from bevy_transform #4168

alice-i-cecile commented Mar 10, 2022 •

edited

Loading

alice-i-cecile commented Mar 10, 2022

alice-i-cecile commented Mar 12, 2022

bors bot commented Mar 12, 2022

IceSentry commented Mar 12, 2022 •

edited

Loading

cart commented Mar 15, 2022

cart commented Mar 15, 2022

cart left a comment

cart commented Mar 15, 2022

alice-i-cecile commented Mar 15, 2022

bors bot commented Mar 15, 2022

cart commented Mar 15, 2022

[Merged by Bors] - Split bevy_hierarchy out from bevy_transform #4168

[Merged by Bors] - Split bevy_hierarchy out from bevy_transform #4168

Conversation

alice-i-cecile commented Mar 10, 2022 • edited Loading

Objective

Solution

Frustrations

Additional context

alice-i-cecile commented Mar 10, 2022

alice-i-cecile commented Mar 12, 2022

bors bot commented Mar 12, 2022

try

IceSentry commented Mar 12, 2022 • edited Loading

cart commented Mar 15, 2022

cart commented Mar 15, 2022

cart left a comment

Choose a reason for hiding this comment

cart commented Mar 15, 2022

alice-i-cecile commented Mar 15, 2022

bors bot commented Mar 15, 2022

cart commented Mar 15, 2022

alice-i-cecile commented Mar 10, 2022 •

edited

Loading

IceSentry commented Mar 12, 2022 •

edited

Loading