Transform rewrite - initial draft #374

MarekLg · 2020-08-27T16:32:01Z

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 Transform as 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.

GrantMoyer

I have some comments, but I'm not familiar enough with the parenting system to see what's wrong with that component.

crates/bevy_transform/src/components/transform.rs

crates/bevy_transform/src/transform_propagate_system.rs

examples/3d/z_sort_debug.rs

MarekLg · 2020-09-01T08:42:53Z

Parenting works now. The problem came from replacing LocalTransform with Transform, especially in ChildBuilder and WorkChildBuilder, where it was set to default after the Entity has been added to world with the proper Transform.

MarekLg · 2020-09-03T17:18:29Z

The last test that's failing is hierarchy_maintenance_system::correct_children()
It seems to fail because the commands passed during parent_update_system() don't seem to be applied right away (reparenting works if schedule is run 2 times and fails if schedule is run 1time). This causes transform_propagate_system() to try to access a child that, although already being world.despawned, still is being referenced to in Children.

Does anyone have an idea how to solve this?

cart · 2020-09-08T23:29:23Z

I'll be diving into this soon. I'll hopefully have answers to your questions in the near future.

cart · 2020-09-09T00:06:45Z

I love how much simpler this makes the code ❤️

cart · 2020-09-09T00:28:46Z

crates/bevy_transform/src/components/transform.rs

-                    // ideally setting the transform automatically propagates back to position / translation / rotation,
-                    // but right now they are always considered the source of truth
+    local: Mat4,
+    global: Mat4,
 }

 impl Transform {


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/child_builder.rs

crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs

cart

(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.

cart · 2020-09-09T00:34:13Z

crates/bevy_transform/src/hierarchy/world_child_builder.rs

@@ -26,7 +26,7 @@ impl<'a, 'b> WorldChildBuilder<'a, 'b> {
            .with_bundle((
                Parent(parent_entity),
                PreviousParent(Some(parent_entity)),
-                LocalTransform::default(),
+                //Transform::default(),


I really like that this no longer couples hierarchy to transforms

crates/bevy_transform/src/lib.rs

cart · 2020-09-09T00:35:50Z

crates/bevy_transform/src/transform_propagate_system.rs

            }
        }
    }
 }

+// ANSWERME: maybe speed this up with compute


Short term I don't think using compute is worth the complexity. For now this is totally fine

crates/bevy_ui/src/flex/mod.rs

cart · 2020-09-09T00:45:45Z

examples/3d/3d_scene.rs

            ..Default::default()
        })
        // camera
        .spawn(Camera3dComponents {
-            transform: Transform::new_sync_disabled(Mat4::face_toward(
+            transform: Transform::new(Mat4::face_toward(


Mmmm new_sync_disabled was a pretty big papercut. Removing it is a big win

examples/3d/3d_scene.rs

examples/3d/load_model.rs

examples/3d/parenting.rs

crates/bevy_transform/src/components/transform.rs

cart · 2020-09-09T01:07:49Z

crates/bevy_transform/src/hierarchy/child_builder.rs

-        assert!(world.get::<LocalTransform>(child1).is_ok());
-        assert!(world.get::<LocalTransform>(child2).is_ok());
+        assert!(world.get::<Transform>(child1).is_ok());
+        assert!(world.get::<Transform>(child2).is_ok());
    }



We can remove these asserts as we no longer automatically add Transforms via with_children (which is a good thing)

crates/bevy_transform/src/hierarchy/child_builder.rs

crates/bevy_transform/src/transform_propagate_system.rs

MarekLg · 2020-09-11T11:40:37Z

@cart Thanks for the great comments! Your help is greatly appreciated. I resolved the comments which I consider addressed in the following commits.

MarekLg · 2020-09-12T09:23:16Z

If everything works in 3D, how should Transform be implemented for 2D? I see 2 options:

Make 2D Components also use Transform (maybe implement Transform::translate() for Vec2)
Make Entities without Transform and GlobalTransform also be renderable by generating GlobalTransform from Translation etc.

What are your guys opinions?

cart · 2020-09-12T17:59:47Z

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.

MarekLg · 2020-09-14T08:50:37Z

I'm on it. Do you think Translation, Rotation, Scale and NonUniformScale are still needed, or can they be removed?

cart · 2020-09-14T08:55:49Z

Let's remove them

MarekLg · 2020-09-14T09:16:11Z

The shader/ examples are the only one that don't work yet.

MarekLg · 2020-09-14T09:53:44Z

Everything works now.

cart

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

initial draft

cfa64e8

MarekLg marked this pull request as draft August 27, 2020 16:33

GrantMoyer reviewed Aug 27, 2020

View reviewed changes

replaced TransformBuilder, replaced e.g. Translation by Vec3

6889524

karroffel added C-Code-Quality A section of code that is hard to understand or change A-ECS Entities, components, systems, and events labels Aug 28, 2020

MarekLg added 3 commits August 28, 2020 13:10

removed pass by reference in e.g. Transform::translate()

dfced53

parenting working (see PR bevyengine#374 for more info)

03372ec

removed print_children system

8129f51

tristanpemble mentioned this pull request Sep 2, 2020

Optimize transform systems to only run on change #417

Merged

MarekLg added 2 commits September 4, 2020 14:45

Merge branch 'master' into transform-rewrite

13b4f19

removed unused imports, kept commented code

f8a6d58

cart reviewed Sep 9, 2020

View reviewed changes

crates/bevy_transform/src/hierarchy/child_builder.rs Outdated Show resolved Hide resolved

cart reviewed Sep 9, 2020

View reviewed changes

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

cart requested changes Sep 9, 2020

View reviewed changes

cart reviewed Sep 9, 2020

View reviewed changes

MarekLg added 9 commits September 11, 2020 12:35

added missing transformations

b75c2ab

removed unnecessary comments

86c8d63

removed unnecessary unit struct components

c757af2

removed unnecessary comments

fd439f2

removed registrations

a949b70

forgetting optimization by compute shader for now

4025802

changed local_matrix().w_axis to local_transform

39e8b84

removed unnecessary childs

4ea9cbd

changed to set_local_translation()

db6ab51

MarekLg added 3 commits September 11, 2020 13:18

removed unused imports

fe04ecf

changed to Vec3::new() constructor

e3936d3

removed unnecessary asserts

3e120cc

MarekLg added 3 commits September 11, 2020 13:49

Merge upstream/master into transform-rewrite

c0cccfc

fix correct_children() test failing

7cc90b1

split up local and global Transform

8329f96

MarekLg added 2 commits September 14, 2020 11:07

adapted 2D components to Transforms

79c898a

removed old Transform Components

717f2b0

forgot to add global_transform

8a17481

MarekLg marked this pull request as ready for review September 14, 2020 09:53

MarekLg requested a review from cart September 14, 2020 09:55

cart approved these changes Sep 14, 2020

View reviewed changes

cart merged commit 474bb54 into bevyengine:master Sep 14, 2020

MarekLg deleted the transform-rewrite branch September 15, 2020 07:23

guimcaballero mentioned this pull request Sep 15, 2020

Update transforms after rework in bevy#374 aevyrie/bevy_mod_picking#29

Merged

utherpally mentioned this pull request Sep 16, 2020

The example with scale component return different result after transform rewrite #374 #499

Closed

mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020

Transform Rewrite (bevyengine#374)

645ebe8

Remove individual Translation / Rotation / Scale components in favor of a combined Transform component

cart mentioned this pull request Apr 27, 2021

Reduce code duplication between Transform and GlobalTransform #2026

Closed

cart mentioned this pull request May 6, 2021

Revert transform back to using Mat4 #1755

Open

notverymoe mentioned this pull request Jul 26, 2021

Enhance 2D Transform/Vec2 Quality of Life #2548

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transform rewrite - initial draft #374

Transform rewrite - initial draft #374

MarekLg commented Aug 27, 2020

GrantMoyer left a comment

MarekLg commented Sep 1, 2020

MarekLg commented Sep 3, 2020

cart commented Sep 8, 2020

cart commented Sep 9, 2020

cart Sep 9, 2020 •

edited

Loading

cart left a comment

cart Sep 9, 2020

cart Sep 9, 2020

cart Sep 9, 2020

cart Sep 9, 2020

MarekLg commented Sep 11, 2020

MarekLg commented Sep 12, 2020

cart commented Sep 12, 2020

MarekLg commented Sep 14, 2020

cart commented Sep 14, 2020

MarekLg commented Sep 14, 2020

MarekLg commented Sep 14, 2020

cart left a comment

Transform rewrite - initial draft #374

Transform rewrite - initial draft #374

Conversation

MarekLg commented Aug 27, 2020

GrantMoyer left a comment

Choose a reason for hiding this comment

MarekLg commented Sep 1, 2020

MarekLg commented Sep 3, 2020

cart commented Sep 8, 2020

cart commented Sep 9, 2020

cart Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

cart left a comment

Choose a reason for hiding this comment

cart Sep 9, 2020

Choose a reason for hiding this comment

cart Sep 9, 2020

Choose a reason for hiding this comment

cart Sep 9, 2020

Choose a reason for hiding this comment

cart Sep 9, 2020

Choose a reason for hiding this comment

MarekLg commented Sep 11, 2020

MarekLg commented Sep 12, 2020

cart commented Sep 12, 2020

MarekLg commented Sep 14, 2020

cart commented Sep 14, 2020

MarekLg commented Sep 14, 2020

MarekLg commented Sep 14, 2020

cart left a comment

Choose a reason for hiding this comment

cart Sep 9, 2020 •

edited

Loading