Transform rewrite - initial draft#374
Conversation
GrantMoyer
left a comment
There was a problem hiding this comment.
I have some comments, but I'm not familiar enough with the parenting system to see what's wrong with that component.
|
Parenting works now. The problem came from replacing |
|
The last test that's failing is Does anyone have an idea how to solve this? |
|
I'll be diving into this soon. I'll hopefully have answers to your questions in the near future. |
|
I love how much simpler this makes the code ❤️ |
| global: Mat4, | ||
| } | ||
|
|
||
| impl Transform { |
There was a problem hiding this comment.
I like this api in general, but I don't think its a complete replacement for the current transform system until we have the following apis:
// sets the absolute local scale
set_local_scale(&mut self, scale: Vec3)
// gets the absolute local rotation
local_rotation(&self) -> Quat
// sets the absolute local rotation
set_local_rotation(&mut self, rotation: Quat)
crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs
Outdated
Show resolved
Hide resolved
cart
left a comment
There was a problem hiding this comment.
(sorry for the multiple reviews. still sorting out the best way to use the VSCode Github Pull Requests extension)
Just wrapped up my first pass. In general I think this is the right direction. Once we address comments and fix the broken tests I think this is merge-able.
| Parent(parent_entity), | ||
| PreviousParent(Some(parent_entity)), | ||
| LocalTransform::default(), | ||
| //Transform::default(), |
There was a problem hiding this comment.
I really like that this no longer couples hierarchy to transforms
| } | ||
| } | ||
|
|
||
| // ANSWERME: maybe speed this up with compute |
There was a problem hiding this comment.
Short term I don't think using compute is worth the complexity. For now this is totally fine
| // camera | ||
| .spawn(Camera3dComponents { | ||
| transform: Transform::new_sync_disabled(Mat4::face_toward( | ||
| transform: Transform::new(Mat4::face_toward( |
There was a problem hiding this comment.
Mmmm new_sync_disabled was a pretty big papercut. Removing it is a big win
| assert!(world.get::<Transform>(child1).is_ok()); | ||
| assert!(world.get::<Transform>(child2).is_ok()); | ||
| } | ||
|
|
There was a problem hiding this comment.
We can remove these asserts as we no longer automatically add Transforms via with_children (which is a good thing)
|
@cart Thanks for the great comments! Your help is greatly appreciated. I resolved the comments which I consider addressed in the following commits. |
|
If everything works in 3D, how should Transform be implemented for 2D? I see 2 options:
What are your guys opinions? |
|
In the immediate short term I'd like to keep this PR scoped to reworking the current transform system (which is used in both 2d and 3d). Eventually we might add a more 2d-centric API, but I'd like that to be a separate discussion. |
|
I'm on it. Do you think |
|
Let's remove them |
|
The |
|
Everything works now. |
cart
left a comment
There was a problem hiding this comment.
This is good to go. I'm really excited for these changes :)
Remove individual Translation / Rotation / Scale components in favor of a combined Transform component
This is the first draft of my rewrite of bevy's transformation system (corresponding issue #229).
Lights and root entity placement works as expected.
2 tests are failing:
transform-propagate-system::test::did_propagate_command_buffer()hierarchy_maintenance_system::test::correct_children()and I can't explain myself what's causing it..
Parenting does not work as expected: children seem to keep the same
Transformas their parent.All these issues seem to be related to
Commands, which I don't understand fully at this point.Also, I ignored UI and 2D for now.
I'm hoping for feedback from more experienced contributors, as I don't have much experience with bevy yet, so every help is greatly appreciated.