[Merged by Bors] - Split bevy_hierarchy out from bevy_transform by alice-i-cecile · Pull Request #4168 · bevyengine/bevy

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

Uh oh!

Conversation

alice-i-cecile commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Frustrations

Additional context

Uh oh!

alice-i-cecile commented Mar 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Mar 12, 2022

Uh oh!

bors bot commented Mar 12, 2022

try

Uh oh!

IceSentry commented Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cart commented Mar 15, 2022

Uh oh!

cart commented Mar 15, 2022

Uh oh!

cart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cart commented Mar 15, 2022

Uh oh!

alice-i-cecile commented Mar 15, 2022

Uh oh!

bors bot commented Mar 15, 2022

Uh oh!

cart commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alice-i-cecile commented Mar 10, 2022 •

edited

Loading

IceSentry commented Mar 12, 2022 •

edited

Loading