-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Source of truth for Transform data #229
Comments
I would propose another solution to this problem (copied from another issue that has been closed as duplicate) Wherever |
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 |
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? |
+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 |
@cart great thread! I think this post summarizes it fairly well. @GrantMoyer That's kind of the idea, but instead we would have one 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 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 What do you guys think about the linked solution? |
I think this issue can be closed now. |
@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. |
@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.
This is certainly useful information 😄
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:
|
In any case, I don't think we should have a non-source of truth component for the local transform. I think helper methods to set the source of truth are a much more intuitive interface. |
Pulling in @termhn because of her linear algebra / ultraviolet experience |
Here's a few more considerations to throw into the mix (why
All of these requirements is why I felt separate components for everything was the right call. Your mileage may vary of course. If you really want local transforms to be one component, I would recommend something along the lines of: pub enum Scale {
Identity,
Uniform(f32),
NonUniform(Vec3)
}
pub struct Transform3D {
translation: Vec3,
rotation: Quat,
scale: Scale,
}
// Plus nice `impl` methods for `Transform3D` with separate components to store local space and global space matrices (these are the same on root objects). This still doesn't help with the problem of hierarchies being out of date though. There isn't a perfect solution to that one sadly. For that I would say... make recomputing the hierarchy (updating |
I am personally a fan of "option 1". I think it's the best compromise in terms of the things that @AThilenius has said with usability from the user's perspective. A Another thing to consider is that the composition (i.e. combining two together to get the result of applying their transformations in sequence) of Similarities (or transforms stored in the same manner) is faster than the composition of a full Mat4, but transforming a Point3/Vec3 with a Similarity is slower than transforming it with a Mat4 because rotating a Vec/Point with a Quaternion is slower than with a rotation matrix... however, this perf difference can be made negligible by transforming multiple points at once because you can compute some 'state' upfront and then only do a small amount of computation per point. Finally, one more thing to toss into the hat is that I would consider restricting any scaling at all to be only possible on entities that are "leaves" of the hierarchy graph. This could be done by using an
|
If you are interested in trying Scale, Rotation and Translation in a single 3D transform these currently exist in |
Regarding position/rotation/scale being in a single component or spread across components:
The internals of how position/rotation/scale are stored in a component that contains all three is a changeable implementation detail, so I think that takes a lot of pressure off of picking "the right way" today (as if there is such a thing.) I prefer approach (1). I think there are options for a Mat4 approach (like stashing scale in unused w components) but this seems like something that could be investigated later, hopefully with the benefit of profiling something a little more "real" than we have today. |
Unreal (and other engines) don't use an ECS so you're comparing apples to race cars. Unity DOTs has been separate components from the start and they don't plan on changing that because the performance is exceptional. That gist is only talking about aspects, which are just 'views' on top of set of components. Breaking things apart rather than combining them is almost always faster. Instead of the hierarchy system needing to loop over every chunk with a These are all separate concepts and must be considered independently (I'm seeing these getting blurred):
Edit: One thing I never explored that has some interesting possibilities, is to integrate the concept on hierarchies directly into the ECS core. We might be able to significantly clean up the builder/manipulation/query API if that's done. It also might incur some overhead, but I would GLADLY accept that for a better hierarchy API. Never looked into it, but this is a path I would personally throw some time into because it sounds like heaven after 8 months of Unity DOTs for my day job. |
I'd like to propose a solution to this problem, which IMO is in line with the general consensus here, as well as the current solution in regards changes that would be needed to make. Geometric data is currently stored as
Seeing as only
struct Transform {
pub translation: Vec3,
pub rotation: Quat,
pub scale: Vec3,
} With something like This doesn't have to be the definitive answer, but can be seen as 'good for now' and can be altered if need be. One extension could be to compute The main reasons I see to not convert to a system like that right now:
These points could be irrelevant for other engines, and rightfully so, because they don't prioritize simplicity. Regarding scale: keeping ease of use in mind, we can just leave it a Vec3, which will likely stay Vec3::one() all the time. If a more performat solution is needed here, I like the I'll try to get a PR ready in the next week to see what you guys think! |
Hi Merek,
Have you seen how unity does their ecs hierarchy? It’s quite elegant imo.
https://docs.unity3d.com/Packages/com.unity.entities@0.0/manual/transform_system.html
It doesn’t include the parameterized data because there are many ways to create the matrix. Prescribing a TRS approach is limiting.
Cheers,
Ryan Cleven
… On Sep 27, 2020, at 2:24 AM, Marek Legris ***@***.***> wrote:
I'd like to propose a solution to this problem, which IMO is in line with the general consensus here, as well as the current solution in regards changes that would be needed to make.
Geometric data is currently stored as Transform and GlobalTransform:
Transform represents local data
GlobalTransform is used as the global data and gets computed after every update.
Seeing as only GlobalTransform gets used for any rendering, it can keep being represented as a Mat4 - minus the mutating methods except matrix_mut(..) ..
Transform could look like this
struct Transform {
pub translation: Vec3,
pub rotation: Quat,
pub scale: Vec3,
}
With something like pub fn compute_matrix(mut self) -> Mat4 to compute the respective Mat4 used to compute the GlobalTransform.
This doesn't have to be the definitive answer, but can be seen as 'good for now' and can be altered if need be. One extension could be to compute GlobalTransform only from a Tuple of e.g. (&Translation, &Rotation), if no Transform is present.
The main reasons I see to not convert to a system like that right now:
It either relies on newtypes, which IMO are more wonky to use as a single Transform, or we would need to rewrite a lot of the glam stuff.
We could not provide methods like Transform::orbit_around(&mut self, center: Vec3, axis: Vec3, t: f32) without a system, because they operate on more than one component.
These points could be irrelevant for other engines, and rightfully so, because they don't prioritize simplicity.
Regarding scale: keeping ease of use in mind, we can just leave it a Vec3, which will likely stay Vec3::one() all the time. If a more performat solution is needed here, I like the enum approach shown by @AThilenius.
Regarding hierarchy: We can just leave it as it is, because it works. As above, if we need to rewrite it that would likely be decoupled from Transforms anyway.
I'll try to get a PR ready in the next week to see what you guys think!
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@onehundredfeet thanks for the article! It's always nice to see different approaches. |
What happens when a parent has N children? Do you recompute the mat4 for each child? What happens when N systems need access to the global transform for M entities? Do you just accept the O(MN) complexity, or do you cache it somewhere? If you cache it somewhere... then how is that different from the way I originally setup |
@AThilenius bevy has a transform propagation system which queries all I don't know how this is different from legions implementation, because I didn't look at it. |
I'm aware, I wrote the original impl 😋 I'm confused though, can you clarify exactly which components you intend to have, and what they contain? |
That's good to know 😅 I just adopted the system as it was when we had both Relevant components are:
That's about it, really. A quick look into |
I was wondering why it would be necessary to rewrite a lot of |
@bitshifter For example: I may be wrong, however, and there is a neat solution after all.. If that's the case I'd love to be corrected! |
@MarekLg ah thanks. I was just wondering if you were hitting some limitation in |
@bitshifter absolutely correct |
Resolved by #596 |
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.
The text was updated successfully, but these errors were encountered: