Transform hierarchy split #2789

finegeometer · 2021-09-07T23:52:47Z

As well as the changes described in the original issue, I split out a HierarchyPlugin from TransformPlugin. I had missed that detail when I first wrote the issue.

Notes

I don't like that bevy_transform depends on bevy_hierarchy, but the latter is necessary for transform_propagate_system.
It would be nice if transform_propagate_system could somehow default to just setting GlobalTransform = Transform in the case where there is no hierarchy.
When I forgot to make HierarchyPlugin a default plugin, there were no test or CI failures (as tested by the instructions in CONTRIBUTING.md).
When I split the crate, I kept the original module structure. I didn't check whether it makes sense to move modules around as a result of the split.
This is my first pull request — did I do anything wrong?

…t it's important.

finegeometer · 2021-09-08T00:16:35Z

What do I do about the failing CI check? As far as I can tell, the failure isn't my code's fault.

MinerSebas · 2021-09-08T00:18:53Z

Carts response from another PR:

Sadly check-advisories will fail until we upgrade wgpu (which can't happen until rust 2021 drops because they decided to use the new cargo dependency resolver). Upgrading now would mean that all of our users would need to manually edit their cargo.toml to opt in to the new resolver (which will become the default in about a month). Not an ideal situation.

For now, don't worry about it. We have removed the check-advisories step from the bors validation step, so we can still merge this normally. Sorry for the weirdness!

finegeometer · 2021-09-08T00:20:21Z

Carts response from another PR:

Sadly check-advisories will fail until we upgrade wgpu (which can't happen until rust 2021 drops because they decided to use the new cargo dependency resolver). Upgrading now would mean that all of our users would need to manually edit their cargo.toml to opt in to the new resolver (which will become the default in about a month). Not an ideal situation.
For now, don't worry about it. We have removed the check-advisories step from the bors validation step, so we can still merge this normally. Sorry for the weirdness!

Thanks!

alice-i-cecile · 2021-09-08T00:29:41Z

I don't like that bevy_transform depends on bevy_hierarchy, but the latter is necessary for transform_propagate_system.

TBH, GlobalTransform serves no purpose without a hierarchy. I'd just push that into the proposed bevy_hierarchy crate along with transform_propagate_system.

When I forgot to make HierarchyPlugin a default plugin, there were no test or CI failures (as tested by the instructions in CONTRIBUTING.md).

This doesn't surprise me; our integration testing is very weak right now. See #1057 for some of the challenges there.

When I split the crate, I kept the original module structure. I didn't check whether it makes sense to move modules around as a result of the split.

This is a sensible choice; better to keep the changes small.

This is my first pull request — did I do anything wrong?

Looks solid, thanks!

alice-i-cecile · 2021-09-08T00:31:06Z

crates/bevy_hierarchy/README.md

@@ -0,0 +1,3 @@
+# Bevy Hierarchy
+
+Provides the ability to work with a parent-child hierarchy of entities.


This should probably be expanded somewhat to describe how it propagates transforms down the tree.

alice-i-cecile · 2021-09-08T00:33:38Z

crates/bevy_transform/README.md

@@ -1,3 +1,5 @@
 # Bevy Transform

-This crate is largely a 1:1 port from [legion_transform](https://github.com/AThilenius/legion_transform) (ecs: legion, math: nalgebra) to bevy (ecs: bevy_ecs, math: glam)
+This crate was largely a 1:1 port from [legion_transform](https://github.com/AThilenius/legion_transform) (ecs: legion, math: nalgebra) to bevy (ecs: bevy_ecs, math: glam)


I think this line is just fully obsolete at this point; we've changed the data structures around since then.

alice-i-cecile · 2021-09-08T00:35:21Z

crates/bevy_transform/src/transform_propagate_system.rs

@@ -191,3 +192,104 @@ mod test {
        );
    }
 }
+
+#[cfg(test)]
+mod test2 {


This should have a more descriptive name.

It was moved from another file, where it was just named "test". I honestly didn't check what it's actually testing!

alice-i-cecile · 2021-09-08T00:36:08Z

tools/publish.sh

@@ -16,6 +16,7 @@ crates=(
    bevy_audio
    bevy_core
    bevy_diagnostic
+    bevy_hierarchy


This is interesting; I wonder if this would publish a not-very-useful standalone bevy_hierarchy 0.5...

I don't know how this works; I was just following the advice in CONTRIBUTING.md. That advice told me to add it, but I don't know if I did something wrong.

finegeometer · 2021-09-08T01:05:28Z

TBH, GlobalTransform serves no purpose without a hierarchy. I'd just push that into the proposed bevy_hierarchy crate along with transform_propagate_system.

But that would force bevy_hierarchy to depend on bevy_transform instead!

Also, crates like bevy_render require GlobalTransform to exist.

alice-i-cecile · 2021-09-08T01:20:45Z

But that would force bevy_hierarchy to depend on bevy_transform instead!

This seems like the correct behavior: for more generalized parent-child-like behavior, we need something along the lines of relations to allow users to create their own hierarchies (or other graphs) without risking mangling.

Reusing the Parent component for other hierarchical behavior is dangerous: it's likely to cause very confusing positioning bugs even though that wasn't the desired approach. FWIW, I pretty strongly feel that Parent should be TransformParent for this reason: you're far from the first user I've seen try to use them outside of a transform-updating context.

finegeometer · 2021-09-08T15:47:32Z

But that would force bevy_hierarchy to depend on bevy_transform instead!

This seems like the correct behavior: for more generalized parent-child-like behavior, we need something along the lines of relations to allow users to create their own hierarchies (or other graphs) without risking mangling.

Reusing the Parent component for other hierarchical behavior is dangerous: it's likely to cause very confusing positioning bugs even though that wasn't the desired approach. FWIW, I pretty strongly feel that Parent should be TransformParent for this reason: you're far from the first user I've seen try to use them outside of a transform-updating context.

Have you seen the comment where I explained my motivation for this pull? I wouldn't call this "trying to use them outside of a transform-updating context".

But even so, this comment is starting to make me rethink whether I want to make this pull request at all! When I wrote the issue and pull, I was working under the belief that transform and hierarchy were mostly independent concerns. They'd interact, but not that strongly. But you're saying that they should be much more closely tied than that. And if that's true, this pull doesn't make much sense.

I guess what I'd like to know is if Bevy's other main contributors agree with your stance here. If so, I'll probably cancel the pull request.

FWIW, I pretty strongly feel that Parent should be TransformParent for this reason ...

A possibility that occurred to me: What if the types were parameterized, like Parent<Label> and Child<Label>?
Then your transform hierarchy could be Parent<Transform> and Child<Transform>, while Parent<SomethingElse> and Child<SomethingElse> could form an entirely unrelated hierarchy!

Even if this is a good idea, though, it should probably be a separate issue.

alice-i-cecile · 2021-09-08T16:21:54Z

Have you seen the comment where I explained my motivation for this pull? I wouldn't call this "trying to use them outside of a transform-updating context".

Aha, thanks for calling this out: that's an interesting use case! The most elegant solution for your design would be something like a Transformlike trait, and then make all of these systems generic and add a TransformType resource to configure which one you're using. Note that non-Euclidean spaces are not the only possible application of this! #1680 is a much more usual application of changing the transform type that would be solved by that design too.

But even so, this comment is starting to make me rethink whether I want to make this pull request at all! When I wrote the issue and pull, I was working under the belief that transform and hierarchy were mostly independent concerns. They'd interact, but not that strongly. But you're saying that they should be much more closely tied than that. And if that's true, this pull doesn't make much sense.

FWIW, I would like them to be much more independent than they are, which is why I'm generally supportive of this split. But without a way to construct variants of the hierarchy (so then we can keep the hierarchy maintenance and commands bits without the transform propagation) we're kind of stuck. To me, this is effectively ground-work for that decoupling.

A possibility that occurred to me: What if the types were parameterized, like Parent and Child?
Then your transform hierarchy could be Parent and Child, while Parent and Child could form an entirely unrelated hierarchy!

Yep, this is a solid idea! This is the fundamental driver behind the relations project; where we're attempting to explore how to represent these parametrically typed graphs within the ECS :) Trees are an important special case of this.

finegeometer · 2021-09-08T16:44:32Z

Aha, thanks for calling this out: that's an interesting use case! The most elegant solution for your design would be something like a Transformlike trait, and then make all of these systems generic and add a TransformType resource to configure which one you're using. Note that non-Euclidean spaces are not the only possible application of this! #1680 is a much more usual application of changing the transform type that would be solved by that design too.

So maybe I should close this, and open a general "Support user-defined Transform types" issue?

FWIW, I would like them to be much more independent than they are, which is why I'm generally supportive of this split. But without a way to construct variants of the hierarchy (so then we can keep the hierarchy maintenance and commands bits without the transform propagation) we're kind of stuck. To me, this is effectively ground-work for that decoupling.

I have to admit, I don't really understand your reasoning here. But since you still care about this split, and I don't anymore, is there some way for me to "give you" this pull request?

alice-i-cecile · 2021-09-08T17:01:10Z

So maybe I should close this, and open a general "Support user-defined Transform types" issue?

That sounds reasonable. I think a Discussion or RFC is probably more appropriate at this stage, as #1680 already exists and we either need to chat about strategies to get there or to present a concrete design.

I have to admit, I don't really understand your reasoning here. But since you still care about this split, and I don't anymore, is there some way for me to "give you" this pull request?

I think the split can probably wait, so I'll just close this out for now. We can revive it by forking your branch if it's relevant in the future.

@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>

@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>

@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>

finegeometer added 3 commits September 7, 2021 13:36

Initial splitting. Contains exactly those changes laid out in the issue.

33c13f1

Split . I didn't think of this when I wrote the issue description, bu…

61731b5

…t it's important.

Make TransformPlugin not call HierarchyPlugin.

ce6f53e

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 7, 2021

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 Sep 8, 2021

alice-i-cecile reviewed Sep 8, 2021

View reviewed changes

alice-i-cecile closed this Sep 8, 2021

james7132 mentioned this pull request Mar 8, 2022

Hierarchy optimized systems and storage #4141

Open

maniwani mentioned this pull request Mar 8, 2022

Child's Transform component ignored when parent does not have the GlobalTransform/Transform components #2277

Closed

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

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transform hierarchy split #2789

Transform hierarchy split #2789

finegeometer commented Sep 7, 2021 •

edited

Loading

finegeometer commented Sep 8, 2021

MinerSebas commented Sep 8, 2021

finegeometer commented Sep 8, 2021

alice-i-cecile commented Sep 8, 2021

alice-i-cecile Sep 8, 2021

alice-i-cecile Sep 8, 2021

alice-i-cecile Sep 8, 2021

finegeometer Sep 8, 2021

alice-i-cecile Sep 8, 2021

finegeometer Sep 8, 2021

finegeometer commented Sep 8, 2021

alice-i-cecile commented Sep 8, 2021

finegeometer commented Sep 8, 2021

alice-i-cecile commented Sep 8, 2021

finegeometer commented Sep 8, 2021

alice-i-cecile commented Sep 8, 2021

		@@ -0,0 +1,3 @@
		# Bevy Hierarchy

		Provides the ability to work with a parent-child hierarchy of entities.

Transform hierarchy split #2789

Transform hierarchy split #2789

Conversation

finegeometer commented Sep 7, 2021 • edited Loading

Notes

finegeometer commented Sep 8, 2021

MinerSebas commented Sep 8, 2021

finegeometer commented Sep 8, 2021

alice-i-cecile commented Sep 8, 2021

alice-i-cecile Sep 8, 2021

Choose a reason for hiding this comment

alice-i-cecile Sep 8, 2021

Choose a reason for hiding this comment

alice-i-cecile Sep 8, 2021

Choose a reason for hiding this comment

finegeometer Sep 8, 2021

Choose a reason for hiding this comment

alice-i-cecile Sep 8, 2021

Choose a reason for hiding this comment

finegeometer Sep 8, 2021

Choose a reason for hiding this comment

finegeometer commented Sep 8, 2021

alice-i-cecile commented Sep 8, 2021

finegeometer commented Sep 8, 2021

alice-i-cecile commented Sep 8, 2021

finegeometer commented Sep 8, 2021

alice-i-cecile commented Sep 8, 2021

finegeometer commented Sep 7, 2021 •

edited

Loading