Source of truth for Transform data

[andreasterrius]

Currently the Transform is not considered the ssot for position, rotation, and scale (based on the comments in transform.rs). However in the examples (3d_scene, load_model) translation and rotation for the Camera3dComponents is usually never set. This could cause confusion in regards of the ssot of the position, rotation and scale data.

The position data will be needed in the future to calculate more advanced pbr shaders.

[MarekLg]

I would propose another solution to this problem (copied from another issue that has been closed as duplicate)

Wherever Transform is required, if there is none attached, build it (maybe with #266) from Translation, Rotation or Scale, or Trasform::identity(), if nothing else is present. DynamicBundles would have to restrict creation to just Transform, which could however incorporate Translation, Rotation and/or Scale by using the above mentioned TransformBuilder.

[cart]

Yeah currently transform is the "output" used in shaders, but position, rotation, and scale will be used to set the Transform, if they exist. The benefits are that systems can pull in exactly what they want, which is faster (less memory being accessed) and easier to parallelize.

The downside is that Transform needs to be updated after app logic finishes, which means Transform will be a frame behind in the UPDATE stage. It also creates issues like "setting the transform does not automatically update source components.

The alternative is to use a combined transform for everything. We are heavily considering this option.

The Amethyst folks have already discussed the issue at length, so please read that thread first before weighing in here: https://community.amethyst.rs/t/legion-transform-design-discussion

[GrantMoyer]

Couldn't we update the Transform every time Scale, NonUniformScale, Rotation, or Translation are mutated (what we do now), and visa versa (which we don't do now)? That way, the Transform and the individual components are always kept in sync, and we could throw out the sync member of Transform. What am I missing?

[aclysma]

+1 for using a single transform component (my preference is that it contains a simple 4x4 matrix). My rationale was on the thread linked by @cart here: https://community.amethyst.rs/t/legion-transform-design-discussion/1582/16?u=aclysma

[MarekLg]

@cart great thread! I think this post summarizes it fairly well.

@GrantMoyer That's kind of the idea, but instead we would have one Transform-component that implements e.g. translate(). The problem lies more in the hierarchy: When a Transform is translated, for example, all children would have to be moved as well, which is not possible without access to them. You can look at the linked post for how the amethyst folks want to solve this.

As I see it, the biggest drawback of their solution, in terms of performance, is that there are more matrix calculations needed to be done by the CPU. In terms of usability it would be nice to be able to mutate the global-part of the Transform.

I think, however, those drawbacks are negligible, at least for the moment. A possible solution for the performance-part could be to load it off to a compute shader, if it all happens at the same time and I'm not terribly mistaken.

Another extension to this solution could still be to allow just e.g. a Translation-component on the entity and create a Transform based on that. Also, could it be possible to perform those global transform calculations in visible_entities_system(..)?.

What do you guys think about the linked solution?

[MarekLg]

I think this issue can be closed now.

[AThilenius]

@cart @MarekLg @aclysma I'm sorry I didn't know about Bevy until a few days ago, so didn't know about this conversation. I wrote the original legion_transform library. I'm all for a 'simpler' API if you guys feel that's a better fit for Bevy (I personally much prefer separate Translation/Rotation/Scale components as keeping entity sizes down is crucial for ECS performance and being able to 'throw entities at the problem'). I'm sorry so much work has already gone into this, but...

You cannot store the source of truth for an Affine transform as a mat4, it accumulates significant errors over time as you manipulate rotation. This is because rotation and scale are encoded together in the upper left 3x3, rotation will end up effecting scale and it will drift, see this article for example. This must be reverted back to separate Translation (Vec2/3), Rotation (Quat3/4), Scale(Float) and/or NonUniformScale (float2/3) even if you chose to have all of those live in one component (which again, I disagree with, but that's your call 🤷‍♂️).

Also NonUniformScale was chosen as an exception not the default because Affine transforms (ones with non-uniforms scale) are evil in games. They incur significant precision errors when converted to a mat4 (especially in a hierarchy), plus they cannot be used by a physics engine in any meaningful way without a nightmarish amount of corner-cases and performance degradation.

[cart]

@AThilenius thanks for the heads up! You have a lot of context here, so your perspective is much appreciated. We currently make no stability guarantees, so I'm more than happy to make breaking changes in the interest of improving the engine. I'm re-opening this issue and I encourage everyone to weigh in on @AThilenius's comments.

You cannot store the source of truth for an Affine transform as a mat4, it accumulates significant errors over time as you manipulate rotation

This is certainly useful information 😄
Sounds like a problem we should solve, but how we do that is up for debate:

1. Completely replace Mat4 with pos/rot/scale inside the Transform component and re-compute matrix on demand when it is asked for. Direct matrix read/writes are now "more expensive" and accessing rotation and scale are now "less expensive".
2. Revert to the old model. "Transform" is now back to being out of sync during the UPDATE stage. "directly setting" matrix values is now cumbersome because the individual components are the source of truth. I would probably want to work out a nice way to "sync back" direct Transform changes to the "source of truth" components.
3. Store pos/rot/scale/mat4 together and make sure they stay in-sync internally. This makes the component "very big" and is almost certainly not what we should shoot for. It might be worth benchmarking this just to see how much performance we lose.

NonUniformScale was chosen as an exception not the default

Yeah I totally agree on this. I tried to mirror that in the new transform api by using the "scale" term to refer to a uniform scale:

bevy/crates/bevy_transform/src/components/transform.rs

Line 31 in 3abfcad

pub fn from_scale(scale: f32) -> Self {