Refactor local transform #660

derekdreery · Apr 22, 2018

I've refactored the LocalTransform to be more like what I would expect. I don't know if this change is correct, but hopefully if there is criticism of it then that will help me understand the function of LocalTransform in amethyst.

This change is

One thing I would say/change I probably need to make is to correct the asymmetry between move_along_local/global and rotate_global/local. They handle the degenerate case axis == zero differently.

derekdreery · Apr 22, 2018

One thing I would say/change I probably need to make is to correct the asymmetry between move_along_local/global and rotate_global/local. They handle the degenerate case axis == zero differently.

jojolepro · Apr 22, 2018

jojolepro requested changes Apr 22, 2018

View changes

jojolepro · Apr 22, 2018

amethyst_core/src/transform/components/local_transform.rs

         self
     }
 
     /// Returns the local object matrix for the transform.
     ///
     /// Combined with the parent's `GlobalTransform` component it gives
     /// the global (or world) matrix for the current entity.
+    // this is a hot function


First letter uppercase please x)
Also it should either be /// or it should be inside of the function //.

jojolepro · Apr 22, 2018

amethyst_core/src/transform/components/local_transform.rs

-        let delta = Quaternion::from(self.rotation) * axis.normalize() * amount;
+    /// Move a distance along an axis.
+    ///
+    /// Will not move in the case where the axis is zero, for any distance.


jojolepro · Apr 22, 2018

amethyst_core/src/transform/components/local_transform.rs

-        self.move_local(orientation.forward.into(), amount)
+    #[inline]
+    pub fn move_forward(&mut self, amount: f32) -> &mut Self {
+        // sign is reversed because z comes towards us


I thought it was the inverse?
Anyway, after this change the fly_camera forward would be inverted.

I matched the OpenGL orientation (in the left-hand right-hand sense). I should check the shaders to see which orientation we use. If you run the fly camera example now it should all work -> if it doesn't that's a bug in my PR.

EDIT I checked and it's the right way round (caveat emptor)

Looks pretty good. It will be way easier without Orientation in the way.

jojolepro · Apr 22, 2018

Looks pretty good. It will be way easier without Orientation in the way.

Can always add Orientation back in if it is useful for something.

derekdreery · Apr 22, 2018

Can always add Orientation back in if it is useful for something.

is there a way to get the forward/right/up vectors?

jojolepro · Apr 22, 2018

is there a way to get the forward/right/up vectors?

So assume the default view is the following square (it is for GL, it's flipped in y axis for vulkan). The up of the object can be find by rotating the 'up' of the original system ((0, 1, 0) in GL) using the transform rotation. forward is (0, 0, -1) and right is (1, 0, 0).

EDIT I've checked with the powers that be and updated this comment. You can get the forward/right/up vectors by converting the quaternion into a matrix and reading off the columns, x is right, y is up, z is towards you. Negate z to get going away.

      y
┏━━━━━┯━━━━━┓
┃     1     ┃
┃     │     ┃
┃-1───┼────1┃ x
┃     │     ┃
┃    -1     ┃
┗━━━━━┷━━━━━┛

I can't really draw the z axis (whichever way it goes ;P)

derekdreery · Apr 22, 2018

So assume the default view is the following square (it is for GL, it's flipped in y axis for vulkan). The up of the object can be find by rotating the 'up' of the original system ((0, 1, 0) in GL) using the transform rotation. forward is (0, 0, -1) and right is (1, 0, 0).

EDIT I've checked with the powers that be and updated this comment. You can get the forward/right/up vectors by converting the quaternion into a matrix and reading off the columns, x is right, y is up, z is towards you. Negate z to get going away.

      y
┏━━━━━┯━━━━━┓
┃     1     ┃
┃     │     ┃
┃-1───┼────1┃ x
┃     │     ┃
┃    -1     ┃
┗━━━━━┷━━━━━┛

I can't really draw the z axis (whichever way it goes ;P)

@jojolepro I added a method for getting the information you requested, since it probably reduces cognitive load.

derekdreery · Apr 22, 2018

@jojolepro I added a method for getting the information you requested, since it probably reduces cognitive load.

I disagree with the removal of Orientation in the _global and _local pitch/roll/yaw methods.

The point is to allow the user to work with a different Orientation. The default orientation you chose might make sense for some obscure computer graphics reason but I don't know about it, and I should'nt need to know about it.

I like to work with the {forward: [0,1,0], right: [1,0,0], up: [0,0,1]} Orientation. With the current methods, I can just write down this orientation somewhere, and then call pitch/yaw/roll with it. With the proposed methods, I'm forced to understand the nasty Orientation you're working with, ~~and it isn't even written anywhere~~.

JadeSnail · Apr 22, 2018

I disagree with the removal of Orientation in the _global and _local pitch/roll/yaw methods.

The point is to allow the user to work with a different Orientation. The default orientation you chose might make sense for some obscure computer graphics reason but I don't know about it, and I should'nt need to know about it.

I like to work with the {forward: [0,1,0], right: [1,0,0], up: [0,0,1]} Orientation. With the current methods, I can just write down this orientation somewhere, and then call pitch/yaw/roll with it. With the proposed methods, I'm forced to understand the nasty Orientation you're working with, ~~and it isn't even written anywhere~~.

@JadeSnail Can you explain how the Orientation works? Is it to specify what coordinate system you use? If so, what does the current pitch_local method mean, for example? Does Orientation need to be a full linear operation, or could it be a small enum (LeftUpOut, LeftOutUp, ... RightInDown, etc.) I think there would be 6 ^ 2 variants, including negation.

In other words is this statement correct:

The orientation is there to specify what coordinate system you want to work in, which is then converted to GL coordinate system.

If so this sounds like a good idea, and I'll refactor the PR put it back in and explain what it's for and maybe change its internal representation.

derekdreery · Apr 22, 2018

@JadeSnail Can you explain how the Orientation works? Is it to specify what coordinate system you use? If so, what does the current pitch_local method mean, for example? Does Orientation need to be a full linear operation, or could it be a small enum (LeftUpOut, LeftOutUp, ... RightInDown, etc.) I think there would be 6 ^ 2 variants, including negation.

In other words is this statement correct:

The orientation is there to specify what coordinate system you want to work in, which is then converted to GL coordinate system.

If so this sounds like a good idea, and I'll refactor the PR put it back in and explain what it's for and maybe change its internal representation.

I'm far from an authority on this.

What I do know is that for pitch/roll/yaw to make sense you need to specify three axis. So Orientation shouldn't be the small enum you suggest.

I think it would be useful if there was some kind of duality between Orientation's and Transform.rotation. That is, I think it would make sense for an Orientation to be three orthogonal directions, with up being the vector product of right and forward.

JadeSnail · Apr 22, 2018

I'm far from an authority on this.

What I do know is that for pitch/roll/yaw to make sense you need to specify three axis. So Orientation shouldn't be the small enum you suggest.

I think it would be useful if there was some kind of duality between Orientation's and Transform.rotation. That is, I think it would make sense for an Orientation to be three orthogonal directions, with up being the vector product of right and forward.

EDITED

So you can view a pitch/yaw/roll orientation as a rotation from the starting rotation (the one where you are looking down negative z with x left-to-right and y bottom-to-top (whatever the graphics coordinate system says it is, this is the GL one). There is a 1-1 correspondence (ish) relationship between rotation (or normalized) quaternions and 3 orthogonal normalized vectors (up, left, out). The transform for the current object is stored internally as a quaternion, so the orientation of the transformed entity is already stored.

I think it would be useful if there was some kind of duality between Orientation's and Transform.rotation.

If you want to pitch/yaw/roll according to model up/left/towards or whatever, just use the local_ methods. If you need the vectors you mention, then they are exactly the columns of the matrix represetation of the rotation. I've provided a convenience method for extracting them as an orientation.

Let me know if you have any questions, I may not be explaining it the best way.

derekdreery · Apr 22, 2018

EDITED

So you can view a pitch/yaw/roll orientation as a rotation from the starting rotation (the one where you are looking down negative z with x left-to-right and y bottom-to-top (whatever the graphics coordinate system says it is, this is the GL one). There is a 1-1 correspondence (ish) relationship between rotation (or normalized) quaternions and 3 orthogonal normalized vectors (up, left, out). The transform for the current object is stored internally as a quaternion, so the orientation of the transformed entity is already stored.

I think it would be useful if there was some kind of duality between Orientation's and Transform.rotation.

If you want to pitch/yaw/roll according to model up/left/towards or whatever, just use the local_ methods. If you need the vectors you mention, then they are exactly the columns of the matrix represetation of the rotation. I've provided a convenience method for extracting them as an orientation.

Let me know if you have any questions, I may not be explaining it the best way.

The whole point is that there are no global axes, or local axes. A 3D model doesn't come with axes, the world doesn't come with axes. You choose the axes, and once that choice is made, yaw/roll/pitching make sense.

As I said, I like to think using the axes {forward: [0,1,0], right: [1,0,0], up: [0,0,1]}.

JadeSnail · Apr 22, 2018

The whole point is that there are no global axes, or local axes. A 3D model doesn't come with axes, the world doesn't come with axes. You choose the axes, and once that choice is made, yaw/roll/pitching make sense.

As I said, I like to think using the axes {forward: [0,1,0], right: [1,0,0], up: [0,0,1]}.

But when you load a 3D model, you get coordinates for the vertices. These numbers are relative to an origin of some kind.

derekdreery · Apr 22, 2018

But when you load a 3D model, you get coordinates for the vertices. These numbers are relative to an origin of some kind.

Do you have some example code I can look at using the existing system, so I understand how it's used?

derekdreery · Apr 22, 2018

Do you have some example code I can look at using the existing system, so I understand how it's used?

Let's say I've got a plane in 3D space. No transform has been applied to it, and it looks just like a plane usually looks, that is the «top» of the plane is in the z direction.

Now I want to yaw this plane. As in what yawing usually refers to for planes. The usual yaw, see https://en.wikipedia.org/wiki/Aircraft_principal_axes

Well with what you propose, calling either yaw_global, or yaw_local doesn't yaw the plane.

What does yaw the plane is calling the current yaw_global, or yaw_local, with the {forward: [0,1,0], right: [1,0,0], up: [0,0,1]} Orientation.

JadeSnail · Apr 22, 2018

Let's say I've got a plane in 3D space. No transform has been applied to it, and it looks just like a plane usually looks, that is the «top» of the plane is in the z direction.

Now I want to yaw this plane. As in what yawing usually refers to for planes. The usual yaw, see https://en.wikipedia.org/wiki/Aircraft_principal_axes

Well with what you propose, calling either yaw_global, or yaw_local doesn't yaw the plane.

What does yaw the plane is calling the current yaw_global, or yaw_local, with the {forward: [0,1,0], right: [1,0,0], up: [0,0,1]} Orientation.

{forward: [0,1,0], right: [1,0,0], up: [0,0,1]}

Then this orientation should be a constant.

EDIT on 4/26: I copy-pasted that orientation without really looking at it. Now that I have it turns out I don't like it. The one I like is {forward: [0,0,1], right: [1,0,0], up: [0,1,0]}

Xaeroxe · Apr 22, 2018

{forward: [0,1,0], right: [1,0,0], up: [0,0,1]}

Then this orientation should be a constant.

EDIT on 4/26: I copy-pasted that orientation without really looking at it. Now that I have it turns out I don't like it. The one I like is {forward: [0,0,1], right: [1,0,0], up: [0,1,0]}

i.e. not provided in a function argument, but implied.

Xaeroxe · Apr 22, 2018

i.e. not provided in a function argument, but implied.

Could you have a TransformedTransform, or an OrientedTransform that works like this?

EDIT alternatively just have a parent transform that sets the coordinate system how you want it.

derekdreery · Apr 22, 2018

Could you have a TransformedTransform, or an OrientedTransform that works like this?

EDIT alternatively just have a parent transform that sets the coordinate system how you want it.

{forward: [0,1,0], right: [1,0,0], up: [0,0,1]}
Then this orientation should be a constant.

I'm not sure that's a good idea: what if someone likes to use a different orientation, such as {forward: [1,0,0], right: [0,-1,0], up: [0,0,1]}. Depending on wether the plane in my example has his wings along the x axis or the y-axis, to roll the plane you have to call the rolll method with one or the other of these orientations.
It can also make sense to work with the proposed default Orientation forward: -Vector3::unit_z(),right: Vector3::unit_x(),up: Vector3::unit_y(). Altough slightly less intuitive, working with it might be more efficient in some cases since it's the one used under the hood.

EDIT alternatively just have a parent transform that sets the coordinate system how you want it.

I think I like this idea. Maybe have a Reorient component/entity to which you provide your Orienation and that you add as a component/parent

It precludes you from working with two different Orientations at the same time, but what kind of madman would do that.

JadeSnail · Apr 23, 2018

{forward: [0,1,0], right: [1,0,0], up: [0,0,1]}
Then this orientation should be a constant.

I'm not sure that's a good idea: what if someone likes to use a different orientation, such as {forward: [1,0,0], right: [0,-1,0], up: [0,0,1]}. Depending on wether the plane in my example has his wings along the x axis or the y-axis, to roll the plane you have to call the rolll method with one or the other of these orientations.
It can also make sense to work with the proposed default Orientation forward: -Vector3::unit_z(),right: Vector3::unit_x(),up: Vector3::unit_y(). Altough slightly less intuitive, working with it might be more efficient in some cases since it's the one used under the hood.

EDIT alternatively just have a parent transform that sets the coordinate system how you want it.

I think I like this idea. Maybe have a Reorient component/entity to which you provide your Orienation and that you add as a component/parent

It precludes you from working with two different Orientations at the same time, but what kind of madman would do that.

Thanks for the refactoring. I am really fighting with the math, so improvements are always good :) As the one who introduced Orientation in the first place, I did so, because:

I didn't like the default orientation used under the hood, so I wanted to make it configurable
the idea was to have one Orientation as a resource in the world, which then could be changed and used
However I did not want to use that resource in the methods internally, as there might be the need to do calculations with a different orientation at some point for some obscure game feature (mirror worlds etc.). Imho, passing the orientation does not necessarily make the usage a lot more complex, once you wrap your head around the idea that you need to define up, left and right to do useful transformations.

I think, using a parent transform, which sets the orientation, might make the creation of entities more painful, but the translation easier in most cases, but imho overall does not make stuff a lot simpler. Your opinion might be different, though. It might affect performance and memory usage though, if we set the orientation per entity. It would use more memory, but be more performant.

One idea I would like to pitch in is: Maybe we could create a helper instead, which does automatically use the Orientation resource, so if you have no use-case for varying orientations, you might just use the helper and not worry about orientation anymore.

minecrawler · Apr 23, 2018

Thanks for the refactoring. I am really fighting with the math, so improvements are always good :) As the one who introduced Orientation in the first place, I did so, because:

I didn't like the default orientation used under the hood, so I wanted to make it configurable
the idea was to have one Orientation as a resource in the world, which then could be changed and used
However I did not want to use that resource in the methods internally, as there might be the need to do calculations with a different orientation at some point for some obscure game feature (mirror worlds etc.). Imho, passing the orientation does not necessarily make the usage a lot more complex, once you wrap your head around the idea that you need to define up, left and right to do useful transformations.

I think, using a parent transform, which sets the orientation, might make the creation of entities more painful, but the translation easier in most cases, but imho overall does not make stuff a lot simpler. Your opinion might be different, though. It might affect performance and memory usage though, if we set the orientation per entity. It would use more memory, but be more performant.

One idea I would like to pitch in is: Maybe we could create a helper instead, which does automatically use the Orientation resource, so if you have no use-case for varying orientations, you might just use the helper and not worry about orientation anymore.

I think a constant is enough for most cases, there's no need to configure it at runtime.

torkleyy · Apr 23, 2018

I think a constant is enough for most cases, there's no need to configure it at runtime.

@minecrawler What didn't you like about the default orientation? Would the vulkan coordinate system satisfy you?

Xaeroxe · Apr 23, 2018

@minecrawler What didn't you like about the default orientation? Would the vulkan coordinate system satisfy you?

So I've been reading some and I know some more now.

Swapping the direction of any dimensions is easy - you just negate that value in the projection matrix.

Swapping the position of 2 dimensions is a pretty simple re-arranging of values and negations. It should certainly not be done as a general linear transformation as this is a LOT of operations. The questions is: if someone wants to use a coordinate system that's not the default vulkan one (x goes right, y goes down, z goes away), where should they specify it. I honestly think most people will use this coordinate system, so it's not worth adding complexity to the basic transform. OTOH people who want to work in a different coordinate system should be able to.

I think that the coordinate system used should be restricted to reordering and negating axes, since this is compact (an enum with 36 values, smaller than a pointer) and quick to apply. It should definitely not be a general linear transformation (3 vectors, 9 * 32bit = 9 * 4 byte = 36 byte!), this is a lot of extra data on an entity and will kill the cache. When you pitch/yaw/roll you do it related to either the orientation of the object (local) or its parent (global). If you want to do something else there should be a utility to make this easy, but that does not impact the component.

derekdreery · Apr 23, 2018

So I've been reading some and I know some more now.

Swapping the direction of any dimensions is easy - you just negate that value in the projection matrix.

Swapping the position of 2 dimensions is a pretty simple re-arranging of values and negations. It should certainly not be done as a general linear transformation as this is a LOT of operations. The questions is: if someone wants to use a coordinate system that's not the default vulkan one (x goes right, y goes down, z goes away), where should they specify it. I honestly think most people will use this coordinate system, so it's not worth adding complexity to the basic transform. OTOH people who want to work in a different coordinate system should be able to.

I think that the coordinate system used should be restricted to reordering and negating axes, since this is compact (an enum with 36 values, smaller than a pointer) and quick to apply. It should definitely not be a general linear transformation (3 vectors, 9 * 32bit = 9 * 4 byte = 36 byte!), this is a lot of extra data on an entity and will kill the cache. When you pitch/yaw/roll you do it related to either the orientation of the object (local) or its parent (global). If you want to do something else there should be a utility to make this easy, but that does not impact the component.

My proposed compromise

I still think any configurability is excessive and overkill for this. However I would not be opposed to a resource being used for this, which is initialized by default with the typical Vulkan system we've been discussing.

This way no one has to configure this, they can just use the defaults, but if for some reason you really need that to be different then you can write code to modify this resource.

Rather than having such a fat enum though I'd prefer it if the resource was just an instance of the Orientation structure we have now.

Explaining why I feel the way I do

Now what I really want to know is why people feel such flexibility is necessary. What Orientation would you personally use as the default? Are there mathematical branches you can show us that just don't work with a particular orientation? I hear a lot of "well maybe someone else will want different", and my response to that is two questions

Why would they want something different beyond mere arbitrary personal preference? I want mathematical justification.
Why couldn't they just live with it? What can't they do that configurability here would enable them to?

Personally I feel like configurability here is big failure to follow the KISS principle.

Xaeroxe · Apr 23, 2018

My proposed compromise

I still think any configurability is excessive and overkill for this. However I would not be opposed to a resource being used for this, which is initialized by default with the typical Vulkan system we've been discussing.

This way no one has to configure this, they can just use the defaults, but if for some reason you really need that to be different then you can write code to modify this resource.

Rather than having such a fat enum though I'd prefer it if the resource was just an instance of the Orientation structure we have now.

Explaining why I feel the way I do

Now what I really want to know is why people feel such flexibility is necessary. What Orientation would you personally use as the default? Are there mathematical branches you can show us that just don't work with a particular orientation? I hear a lot of "well maybe someone else will want different", and my response to that is two questions

Why would they want something different beyond mere arbitrary personal preference? I want mathematical justification.
Why couldn't they just live with it? What can't they do that configurability here would enable them to?

Personally I feel like configurability here is big failure to follow the KISS principle.

https://math.stackexchange.com/questions/1817242/why-do-we-need-basis-in-linear-algebra
I think having parenting defeats the arguments of this though.. ^

jojolepro · Apr 23, 2018

https://math.stackexchange.com/questions/1817242/why-do-we-need-basis-in-linear-algebra
I think having parenting defeats the arguments of this though.. ^

Xaeroxe · Apr 23, 2018

Xaeroxe approved these changes Apr 23, 2018 • edited

View changes

Looks good to me! Thank you!

torkleyy · Apr 23, 2018

torkleyy reviewed Apr 23, 2018

View changes

Where does this apply the configured orientation?

jojolepro · Apr 23, 2018

jojolepro reviewed Apr 23, 2018

View changes

jojolepro · Apr 23, 2018

amethyst_core/src/transform/components/local_transform.rs

@@ -35,110 +34,179 @@ impl Transform {
     /// the global (or world) matrix for the current entity.


First letter uppercase please

jojolepro · Apr 23, 2018

amethyst_core/src/transform/components/local_transform.rs

-        self.move_local(orientation.forward.into(), amount)
+    #[inline]
+    pub fn move_forward(&mut self, amount: f32) -> &mut Self {
+        // sign is reversed because z comes towards us


Right now it doesn't. I approved it because I like the approach anyways, but I'd approve it again if we moved to the compromise

Xaeroxe · Apr 23, 2018

Right now it doesn't. I approved it because I like the approach anyways, but I'd approve it again if we moved to the compromise

Would the Orientation be a per-entity component (and only be applied when that component is present) or a resource applied to all entities with a transform? What about global transforms? When should the orientation be applied e.g. before or after the object transform?

I'd rather build consensus before this gets merged.

derekdreery · Apr 23, 2018

Would the Orientation be a per-entity component (and only be applied when that component is present) or a resource applied to all entities with a transform? What about global transforms? When should the orientation be applied e.g. before or after the object transform?

I'd rather build consensus before this gets merged.

I'd make it a global resource, anything else is too much overhead imo

Xaeroxe · Apr 23, 2018

I'd make it a global resource, anything else is too much overhead imo

I personally won't use orientation at all so I don't really care.
Orientation would be global, as having a parenting system is equivalent to the benefits having orientation per entity provides.

jojolepro · Apr 23, 2018

I personally won't use orientation at all so I don't really care.
Orientation would be global, as having a parenting system is equivalent to the benefits having orientation per entity provides.

@jojolepro @Xaeroxe got you.

derekdreery · Apr 23, 2018

@jojolepro @Xaeroxe got you.

I need to think about how this applies to all the math.

derekdreery · Apr 23, 2018

I need to think about how this applies to all the math.

Ok so I have looked at how amethyst generates the projection matrix. It just uses cgmath, that copies glFrustrum for perspective, and glOrtho for orthographic, which use the convention of GL where it reverses the z direction. So, in essence the current coordinate system is: x -> right, y -> up, z -> towards.

I think I'd recommend keeping this coordinate system for now, with the option to request a different one that I will add to this PR. It will be interesting to see whether people use this system with a shim, or the new Vulkan one. I think we should see what happens.

derekdreery · Apr 23, 2018

Ok so I have looked at how amethyst generates the projection matrix. It just uses cgmath, that copies glFrustrum for perspective, and glOrtho for orthographic, which use the convention of GL where it reverses the z direction. So, in essence the current coordinate system is: x -> right, y -> up, z -> towards.

I think I'd recommend keeping this coordinate system for now, with the option to request a different one that I will add to this PR. It will be interesting to see whether people use this system with a shim, or the new Vulkan one. I think we should see what happens.

Ok so I've pushed a version of the Orientation struct that should be robust (not possible to have inconsistent internal state), and easy to use.

It is just for relabelling axes.

Now I need to work out how to apply the generated matrix at the correct place. If anyone can help that'd be great. :)

derekdreery · Apr 25, 2018

Ok so I've pushed a version of the Orientation struct that should be robust (not possible to have inconsistent internal state), and easy to use.

It is just for relabelling axes.

Now I need to work out how to apply the generated matrix at the correct place. If anyone can help that'd be great. :)

I think this version is a lot harder to understand than the previous one. I'd guess it'd be harder to use too, but since it isn't clear what it should be used for, it's hard to tell.

I don't think the new found robustness warrants the code complexity and the lack of flexibiliy.

I don't think the amethyst user should have to manually deal with Matrix4 but rather only with the Transform components and methods. Therefore I think you should only be able to get a Quaternion, and not a Matrix4 from an Orientation (which is only possible for a subset of your proposed Orientations).

Here's an example of what the Orientation could be used for: I want to make my player look in a direction. The issue is that setting its Transform rotation to Quaternion.look_at(direction, up) doesn't work. It only works if in my player model, the player looks at Orientation::default().forward. Somehow it would be nice if I could set my chosen Orientation (which defines in what direction my player model is looking) somewhere and have look_at (or some variant) work like expected. This should only be possible if you can get a Quaternion from an Orientation.

Another natural use case for a more flexible Orientation is to keep an Orientation struct (computed from the Transform.rotation) with my player at all times. It allows easy access to its forward/right/up vectors which are constantly used.

JadeSnail · Apr 26, 2018

I think this version is a lot harder to understand than the previous one. I'd guess it'd be harder to use too, but since it isn't clear what it should be used for, it's hard to tell.

I don't think the new found robustness warrants the code complexity and the lack of flexibiliy.

I don't think the amethyst user should have to manually deal with Matrix4 but rather only with the Transform components and methods. Therefore I think you should only be able to get a Quaternion, and not a Matrix4 from an Orientation (which is only possible for a subset of your proposed Orientations).

Here's an example of what the Orientation could be used for: I want to make my player look in a direction. The issue is that setting its Transform rotation to Quaternion.look_at(direction, up) doesn't work. It only works if in my player model, the player looks at Orientation::default().forward. Somehow it would be nice if I could set my chosen Orientation (which defines in what direction my player model is looking) somewhere and have look_at (or some variant) work like expected. This should only be possible if you can get a Quaternion from an Orientation.

Another natural use case for a more flexible Orientation is to keep an Orientation struct (computed from the Transform.rotation) with my player at all times. It allows easy access to its forward/right/up vectors which are constantly used.

Alright yeah my compromise was evidently not the greatest idea.

@derekdreery we can revert the last commit on this PR. Sorry for all the trouble, I'll try and think my ideas through a bit better in the future.

It only works if in my player model, the player looks at Orientation::default().forward.

This is actually pretty standard fair in game dev in my experience. Usually the model is rotated rather than messing up all the coordinate math in the game. That's what I'd rather do personally.

One thing we could do in Amethyst is on the mesh itself we could have a Quaternion that rotates the display of the model but doesn't actually mess with the transform of the entity.

I'm of the opinion that would be the more correct approach to resolving this problem, because then we don't need a special Orientation based on which entity we're transforming.

Xaeroxe · Apr 26, 2018

Alright yeah my compromise was evidently not the greatest idea.

@derekdreery we can revert the last commit on this PR. Sorry for all the trouble, I'll try and think my ideas through a bit better in the future.

It only works if in my player model, the player looks at Orientation::default().forward.

This is actually pretty standard fair in game dev in my experience. Usually the model is rotated rather than messing up all the coordinate math in the game. That's what I'd rather do personally.

One thing we could do in Amethyst is on the mesh itself we could have a Quaternion that rotates the display of the model but doesn't actually mess with the transform of the entity.

I'm of the opinion that would be the more correct approach to resolving this problem, because then we don't need a special Orientation based on which entity we're transforming.

For what you are suggesting, you would not use this orientation. A typical user who wanted a custom orientation (axes labelling) would just create one and add it as a resource. Then the graphics engine would apply that orientation to everything. If you want something that's not global you would have a parent transform.

derekdreery · Apr 26, 2018

For what you are suggesting, you would not use this orientation. A typical user who wanted a custom orientation (axes labelling) would just create one and add it as a resource. Then the graphics engine would apply that orientation to everything. If you want something that's not global you would have a parent transform.

I don't think having a mismatch between the graphics and the actual positioning is a good idea.

jojolepro · Apr 26, 2018

I don't think having a mismatch between the graphics and the actual positioning is a good idea.

Doesn't work because scaling affect both rotation and translation

Rhuagh · Apr 27, 2018

Doesn't work because scaling affect both rotation and translation

@Rhuagh I can imagine that maybe we could create the unscaled transformation then multiply the diff on each axis by the scaled amount, but my limited mathematical experience probably means I'm way off trajectory.

Xaeroxe · Apr 27, 2018

@Rhuagh I can imagine that maybe we could create the unscaled transformation then multiply the diff on each axis by the scaled amount, but my limited mathematical experience probably means I'm way off trajectory.

The problem is the parenting system really, it would be fine if it was a single transformation, but when you apply potentially a large amount of combines it becomes incredibly complex.

Rhuagh · Apr 27, 2018

The problem is the parenting system really, it would be fine if it was a single transformation, but when you apply potentially a large amount of combines it becomes incredibly complex.

What I meant was pub type Transform = Decomposed<.. >

Rhuagh · Apr 27, 2018

What I meant was pub type Transform = Decomposed<.. >

@Rhuagh can you add methods to a type alias?

derekdreery · Apr 27, 2018

@Rhuagh can you add methods to a type alias?

With a trait yes

Rhuagh · Apr 27, 2018

With a trait yes

I'll finish this version, then do one with an alias and then both will be in the history.

derekdreery · Apr 27, 2018

I'll finish this version, then do one with an alias and then both will be in the history.

This still breaks the orphan rule. A type alias won't change that.

Xaeroxe · Apr 27, 2018

This still breaks the orphan rule. A type alias won't change that.

What's the orphan rule, sorry?

derekdreery · Apr 27, 2018

What's the orphan rule, sorry?

Why would that be a problem though @Xaeroxe?

Rhuagh · Apr 27, 2018

Why would that be a problem though @Xaeroxe?

Right I've fixed all the exmaples, so when it (maybe) passes CI, there's just the philosophical questions to answer.

derekdreery · Apr 27, 2018

Right I've fixed all the exmaples, so when it (maybe) passes CI, there's just the philosophical questions to answer.

http://smallcultfollowing.com/babysteps/blog/2015/01/14/little-orphan-impls/

This may prevent us from implementing component for the transform.

Xaeroxe · Apr 27, 2018

http://smallcultfollowing.com/babysteps/blog/2015/01/14/little-orphan-impls/

This may prevent us from implementing component for the transform.

Ah, right, Component would be a problem, crap

Rhuagh · Apr 27, 2018

Ah, right, Component would be a problem, crap

So we can't use Decomposed directly, we need to wrap it in a newtype.

Rhuagh · Apr 27, 2018

So we can't use Decomposed directly, we need to wrap it in a newtype.

And if that's the case I'm not sure how much value we actually get out of Decomposed to begin with :/

Rhuagh · Apr 27, 2018

And if that's the case I'm not sure how much value we actually get out of Decomposed to begin with :/

Btw, why do we need Mul implemented for the transform ?

Rhuagh · Apr 27, 2018

Btw, why do we need Mul implemented for the transform ?

@JadeSnail yes, there are only 9 parameters in the amethyst Transform, but that's because it's decomposed, there's no loss of information compared to a matrix. A transformation matrix has a certain redundancy of information that can be derived from the decomposed representation.

Rhuagh · Apr 27, 2018

@JadeSnail yes, there are only 9 parameters in the amethyst Transform, but that's because it's decomposed, there's no loss of information compared to a matrix. A transformation matrix has a certain redundancy of information that can be derived from the decomposed representation.

And the parenting system computes matrices for the combined transforms, so the end result should be accurate.

Rhuagh · Apr 27, 2018

And the parenting system computes matrices for the combined transforms, so the end result should be accurate.

Reverted to old transform.

derekdreery · Apr 27, 2018

Reverted to old transform.

To be clear, my understanding is as follows. @Rhuagh

When you start composing Transforms, which the parenting system does, you can get any affine transformation of the space (what you refer to as «matrix»: an affine transformation is actually given by a 3by3 matrix and a vector in R^3, these two things are then usually put in a 4by4 matrix, where the last line is (0,0,0,1))

As I said, an affine transformation has 12 parameters, which means that not every affine transformation can be represented by a Transform.

Nonetheless, what the parenting system does is cast the Transforms as affine transformations (using the to matrix4 method) and then multiply these affine transformations. The resulting affine transformation need not be representable by a Transform (though it is, if the two multiplied transforms had uniform scaling).

JadeSnail · Apr 27, 2018

To be clear, my understanding is as follows. @Rhuagh

When you start composing Transforms, which the parenting system does, you can get any affine transformation of the space (what you refer to as «matrix»: an affine transformation is actually given by a 3by3 matrix and a vector in R^3, these two things are then usually put in a 4by4 matrix, where the last line is (0,0,0,1))

As I said, an affine transformation has 12 parameters, which means that not every affine transformation can be represented by a Transform.

Nonetheless, what the parenting system does is cast the Transforms as affine transformations (using the to matrix4 method) and then multiply these affine transformations. The resulting affine transformation need not be representable by a Transform (though it is, if the two multiplied transforms had uniform scaling).

True. Which is why I asked specifically why we'd want a Mul for Transform. I don't see a real use case for it.

Rhuagh · Apr 27, 2018

True. Which is why I asked specifically why we'd want a Mul for Transform. I don't see a real use case for it.

Me neither !

JadeSnail · Apr 27, 2018

Me neither !

I've removed Mul, since it's not really multiply any more (if you use non-uniform scaling).

EDIT: read the above comments :)

derekdreery · Apr 27, 2018

I've removed Mul, since it's not really multiply any more (if you use non-uniform scaling).

EDIT: read the above comments :)

Rhuagh · Apr 28, 2018

Rhuagh approved these changes Apr 28, 2018

View changes

LGTM!

Rhuagh · Apr 28, 2018

LGTM!

This is ready for review

derekdreery · Apr 28, 2018

This is ready for review

Xaeroxe · Apr 28, 2018

Xaeroxe approved these changes Apr 28, 2018

View changes

LGTM! Thanks!

r? @jojolepro

Xaeroxe · Apr 28, 2018

r? @jojolepro

jojolepro · Apr 28, 2018

jojolepro approved these changes Apr 28, 2018

View changes

bors r=Xaeroxe,Rhuagh,jojolepro

Rhuagh · Apr 28, 2018

bors r=Xaeroxe,Rhuagh,jojolepro

Build succeeded

bors · Apr 28, 2018

Build succeeded

Richard Dodd and others added some commits Apr 22, 2018

Simplify local transform

8f048a7

Add example to look_at

efc7675

Fix some comments

984dffa

Neatening up

64eb1d3

Fix gimbal lock, more to do

600c356

Cleanup

Loading status checks…

04f3ac9

bors bot merged commit 04f3ac9 into amethyst:develop Apr 28, 2018
3 checks passed

3 checks passed

bors Build succeeded
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

derekdreery deleted the derekdreery:simplify_transform branch Apr 29, 2018

amethyst/amethyst

Join GitHub today

Refactor local transform #660

Conversation

derekdreery commented Apr 22, 2018 • edited by torkleyy Edited 3 times torkleyy edited Apr 22, 2018 (most recent) derekdreery edited Apr 22, 2018 torkleyy edited Apr 22, 2018

This comment has been minimized.

derekdreery commented Apr 22, 2018

jojolepro requested changes Apr 22, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

derekdreery Apr 22, 2018 • edited Edited 1 time derekdreery edited Apr 22, 2018 (most recent)

This comment has been minimized.

This comment has been minimized.

jojolepro commented Apr 22, 2018

This comment has been minimized.

derekdreery commented Apr 22, 2018

This comment has been minimized.

jojolepro commented Apr 22, 2018

This comment has been minimized.

derekdreery commented Apr 22, 2018 • edited Edited 6 times derekdreery edited Apr 22, 2018 (most recent) derekdreery edited Apr 22, 2018 derekdreery edited Apr 22, 2018 derekdreery edited Apr 22, 2018 derekdreery edited Apr 22, 2018 derekdreery edited Apr 22, 2018

This comment has been minimized.

derekdreery commented Apr 22, 2018

This comment has been minimized.

JadeSnail commented Apr 22, 2018 • edited Edited 1 time JadeSnail edited Apr 22, 2018 (most recent)

This comment has been minimized.

derekdreery commented Apr 22, 2018 • edited Edited 4 times derekdreery edited Apr 22, 2018 (most recent) derekdreery edited Apr 22, 2018 derekdreery edited Apr 22, 2018 derekdreery edited Apr 22, 2018

This comment has been minimized.

JadeSnail commented Apr 22, 2018 • edited Edited 1 time JadeSnail edited Apr 22, 2018 (most recent)

This comment has been minimized.

derekdreery commented Apr 22, 2018 • edited Edited 3 times derekdreery edited Apr 22, 2018 (most recent) derekdreery edited Apr 22, 2018 derekdreery edited Apr 22, 2018

This comment has been minimized.

JadeSnail commented Apr 22, 2018 • edited Edited 1 time JadeSnail edited Apr 22, 2018 (most recent)

This comment has been minimized.

derekdreery commented Apr 22, 2018

This comment has been minimized.

derekdreery commented Apr 22, 2018

This comment has been minimized.

JadeSnail commented Apr 22, 2018 • edited Edited 3 times JadeSnail edited Apr 22, 2018 (most recent) JadeSnail edited Apr 22, 2018 JadeSnail edited Apr 22, 2018

This comment has been minimized.

Xaeroxe commented Apr 22, 2018 • edited Edited 1 time Xaeroxe edited Apr 26, 2018 (most recent)

This comment has been minimized.

Xaeroxe commented Apr 22, 2018

This comment has been minimized.

derekdreery commented Apr 22, 2018 • edited Edited 1 time derekdreery edited Apr 22, 2018 (most recent)

This comment has been minimized.

JadeSnail commented Apr 23, 2018 • edited Edited 1 time JadeSnail edited Apr 23, 2018 (most recent)

This comment has been minimized.

minecrawler commented Apr 23, 2018

This comment has been minimized.

torkleyy commented Apr 23, 2018

This comment has been minimized.

Xaeroxe commented Apr 23, 2018

This comment has been minimized.

derekdreery commented Apr 23, 2018 • edited Edited 1 time derekdreery edited Apr 23, 2018 (most recent)

This comment has been minimized.

My proposed compromise

Explaining why I feel the way I do

Xaeroxe commented Apr 23, 2018 • edited Edited 1 time Xaeroxe edited Apr 23, 2018 (most recent)

My proposed compromise

Explaining why I feel the way I do

This comment has been minimized.

jojolepro commented Apr 23, 2018

Xaeroxe approved these changes Apr 23, 2018 • edited View changes

torkleyy reviewed Apr 23, 2018 View changes

jojolepro reviewed Apr 23, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Apr 23, 2018

This comment has been minimized.

derekdreery commented Apr 23, 2018 • edited Edited 2 times derekdreery edited Apr 23, 2018 (most recent) derekdreery edited Apr 23, 2018

This comment has been minimized.

Xaeroxe commented Apr 23, 2018

This comment has been minimized.

jojolepro commented Apr 23, 2018

This comment has been minimized.

derekdreery commented Apr 23, 2018

derekdreery commented Apr 22, 2018 •

edited by torkleyy

Edited 3 times

torkleyy edited Apr 22, 2018 (most recent)

derekdreery edited Apr 22, 2018

torkleyy edited Apr 22, 2018

jojolepro requested changes Apr 22, 2018

View changes

derekdreery Apr 22, 2018 •

edited

Edited 1 time

derekdreery edited Apr 22, 2018 (most recent)

derekdreery commented Apr 22, 2018 •

edited

Edited 6 times

derekdreery edited Apr 22, 2018 (most recent)

derekdreery edited Apr 22, 2018

derekdreery edited Apr 22, 2018

derekdreery edited Apr 22, 2018

derekdreery edited Apr 22, 2018

derekdreery edited Apr 22, 2018

JadeSnail commented Apr 22, 2018 •

edited

Edited 1 time

JadeSnail edited Apr 22, 2018 (most recent)

derekdreery commented Apr 22, 2018 •

edited

Edited 4 times

derekdreery edited Apr 22, 2018 (most recent)

derekdreery edited Apr 22, 2018

derekdreery edited Apr 22, 2018

derekdreery edited Apr 22, 2018

JadeSnail commented Apr 22, 2018 •

edited

Edited 1 time

JadeSnail edited Apr 22, 2018 (most recent)

derekdreery commented Apr 22, 2018 •

edited

Edited 3 times

derekdreery edited Apr 22, 2018 (most recent)

derekdreery edited Apr 22, 2018

derekdreery edited Apr 22, 2018

JadeSnail commented Apr 22, 2018 •

edited

Edited 1 time

JadeSnail edited Apr 22, 2018 (most recent)

JadeSnail commented Apr 22, 2018 •

edited

Edited 3 times

JadeSnail edited Apr 22, 2018 (most recent)

JadeSnail edited Apr 22, 2018

JadeSnail edited Apr 22, 2018

Xaeroxe commented Apr 22, 2018 •

edited

Edited 1 time

Xaeroxe edited Apr 26, 2018 (most recent)

derekdreery commented Apr 22, 2018 •

edited

Edited 1 time

derekdreery edited Apr 22, 2018 (most recent)

JadeSnail commented Apr 23, 2018 •

edited

Edited 1 time

JadeSnail edited Apr 23, 2018 (most recent)

derekdreery commented Apr 23, 2018 •

edited

Edited 1 time

derekdreery edited Apr 23, 2018 (most recent)

Xaeroxe commented Apr 23, 2018 •

edited

Edited 1 time

Xaeroxe edited Apr 23, 2018 (most recent)

Xaeroxe approved these changes Apr 23, 2018 • edited

View changes

torkleyy reviewed Apr 23, 2018

View changes

jojolepro reviewed Apr 23, 2018

View changes

derekdreery commented Apr 23, 2018 •

edited

Edited 2 times

derekdreery edited Apr 23, 2018 (most recent)

derekdreery edited Apr 23, 2018