Skip to content
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

sample_baked_with_rotation gives wrong transform #78361

Closed
0xafbf opened this issue Jun 17, 2023 · 20 comments · Fixed by #78378
Closed

sample_baked_with_rotation gives wrong transform #78361

0xafbf opened this issue Jun 17, 2023 · 20 comments · Fixed by #78378

Comments

@0xafbf
Copy link
Contributor

0xafbf commented Jun 17, 2023

Godot version

4.0.3.stable

System information

Windows 10

Issue description

evaluating a curve's sample_baked_with_rotation gives wrong Transform2D:

55374fd468146e8611b46b327faa6843.mp4

The texture should read A B C D correctly, you can see it flips the texture vertically

It is trying to match textures Positive Y to the path direction, and then positive x to the right of the path.

I think the two correct options would be:

  • map negative y (which would be up) to the path direction
  • map positive x to the path direction (which I would like better, but would like to know your opinion)

I found this while working on this PR #77819
I could give a shot at fixing this during the weekend

Steps to reproduce

Just run the example project

Minimal reproduction project

PathSampleBug.zip

@AThousandShips
Copy link
Member

AThousandShips commented Jun 17, 2023

Will look into it, yes, it seems to flip the axes

It does work correctly with a PathFollow2D

@AThousandShips
Copy link
Member

AThousandShips commented Jun 17, 2023

It appears the transform returned from the Curve2D is not a real transform, as the documentation shows to extract origin and rotation, but should be clearer, it works if you do:

target.position = tx.get_origin()
target.rotation = tx.get_rotation()

I'm unsure why it is using this unconventional format, but changing it would break compatibility so best to just clarify in the documentation.

Will fix the documentation

@capnm
Copy link
Contributor

capnm commented Jun 17, 2023

It appears the transform returned from the Curve2D is not a real transform, as the documentation shows to extract origin and rotation, but should be clearer, it works if you do:

I'm not sure what you exactly mean. The negative y-scale in transform seems to be the culprit or bug here...

print(tx.get_scale())
(1, -1)

I.e. this works as expected:

target.transform = curve.sample_baked_with_rotation(distance)
target.scale = Vector2(1,1)

@AThousandShips
Copy link
Member

AThousandShips commented Jun 17, 2023

I mean no that gives a different result than the behaviour of PathFollow2D, it's not a negative y-axis, it's that the x and y axes are swapped, it only works if you treat the curve as having the y axis along the curve

The returned transform is: Transform2D(side, forward, pos), where forward is along the curve, i.e. the x-axis in a normal transform, based on the code, and how that code is used, it's not intended to be treated as a conventional transform

If you create a curve of the points [(0, 0), (1, 0)] it will return [(0, 1), (1, 0), (0, 0)] if sampled at 0.0 for example

"Forward" isn't a thing in the 2D coordinates but one would expect it to be the X-axis, otherwise a curve would be very strange to use no? If you want a character to move along a circular curve shouldn't it be facing normally at the top of that curve?

Considering how this code was directly moved from PathFollow2D the assumption should really be that the axes, and use of it from there, is the way to go

One can argue that this convention is confusing and harmful, but changing it would not be possible at this time, so documenting it and clarifying how it's made up is the best way

@capnm
Copy link
Contributor

capnm commented Jun 17, 2023

I haven't had time to read/decipher the corresponding code. Other practical question, why it here manipulates the scale of the target transform at all?

@AThousandShips
Copy link
Member

AThousandShips commented Jun 17, 2023

This is as I said because it swaps the X and Y axes, meaning that it no longer follows the left hand convention, by default the Y axis is 90 degrees clockwise of the X axis, but in this case it is counter clockwise, because of the swapped axes, the scale is extracted in a way that assumes the left hand convention

Remember that scale from a transform is calculated not stored, it makes some assumptions, like certain correctness or rather conventions

@capnm
Copy link
Contributor

capnm commented Jun 17, 2023

Thanks a lot. I'm starting to not know what my left and right hands are in the matrix. 🔃 🔁 🔄 Godot obviously prefers 3D and 2D objects from behind :) Personally, I would rather break whatever compatibility and fix it once forever ...

@AThousandShips
Copy link
Member

No problems! I had a hard time getting my head around it for a while, and my right wrist is all sore from trying to work out the left and right hand rules, note that 3D in Godot is right hand, but 2D is left hand, very confusing

Unfortunately we can't do that, at least not for some time, I suspect not until 5.0 as it's an easy thing to resolve with documentation clarification, and doesn't break anything, as long as you compensate for it, you don't really need the whole transform anyway as it doesn't scale or skew, we could just return a Vector3 with a rotation alone

See if the documentation update in the linked PR is clear to you if you wouldn't mind

@0xafbf
Copy link
Contributor Author

0xafbf commented Jun 17, 2023

I just tried the PathFollow2D Node and it indeed does what I expected to do. I guess I didn't try it because I was focused on the text transform effect PR.

I think we should just make this function just return a good transform and match the behavior of PathFollow2D. I get the idea of using a Transform2D and then reading Y and X axis as forward and right, but the convenience of just using the transform to slap it into some visual element is too good to let it pass.

I like that PathFollow2D chooses X+ for forward. It is my preferred way of solving the problem. It makes it seamless to integrate with the work I did in the linked PR for flowing the text over a path.

I think that, given the use case is currently broken, and the docs previously had no suggestion on how to use this. I feel we should do an appropiate fix now instead of documenting the quirkiness of current implementation.

I can do a PR later today or tomorrow with my suggested fix.

@AThousandShips
Copy link
Member

AThousandShips commented Jun 17, 2023

and the docs previously had no suggestion on how to use this

The docs did have the proposal yes, this:

var transform = curve.sample_baked_with_rotation(offset)
position = transform.get_origin()
rotation = transform.get_rotation()

This is a compatibility breaking change so I don't think it's going to be merged any time soon, at the very earliest for 4.2, I do not agree that this is a bug necessarily, but a quirk in the implementation, we could fix it eventually, but the already present suggestion, and clarification, is better than a compatibility breaking change here

And if this quirk was not known, why would the example of how to use it be included? Sounds strange to assume that the user doesn't know how to use a Transform2D right?

@capnm
Copy link
Contributor

capnm commented Jun 17, 2023

Also you mean that fixing the sample_baked_with_rotation output transform would break something else?

diff --git a/scene/resources/curve.cpp b/scene/resources/curve.cpp
index 12ee36654d..bf3256a9c3 100644
--- a/scene/resources/curve.cpp
+++ b/scene/resources/curve.cpp
@@ -1025,6 +1025,8 @@ Transform2D Curve2D::sample_baked_with_rotation(real_t p_offset, bool p_cubic) c
        // 2. Sample rotation frame.
        Transform2D frame = _sample_posture(interval);
        frame.set_origin(pos);
+       frame.set_scale(Vector2(-1.0, 1.0));
+       frame.set_rotation(frame.get_rotation() + Math_PI);
 
        return frame;
 }

@AThousandShips
Copy link
Member

AThousandShips commented Jun 17, 2023

It would break anyone's code that uses it, it would of course have to be adjusted in the code, like PathFollow2D

The change would instead be in the _sample_posture, swapping the order of the arguments, and updating any use to correct for the change:
From:

	return Transform2D(side, forward, Vector2(0.0, 0.0));

To:

	return Transform2D(forward, side, Vector2(0.0, 0.0));

const Vector2 forward = forward_begin.slerp(forward_end, frac).normalized();
const Vector2 side = Vector2(-forward.y, forward.x);
return Transform2D(side, forward, Vector2(0.0, 0.0));
}

The fact here IMHO is that this code, while strange, is not unusable, you just have to follow the instructions (including the current ones in the documentation), it's cumbersome, but it's not broken, just strange, to me that means that making changes that actually breaks people's code is a bad idea

@capnm
Copy link
Contributor

capnm commented Jun 17, 2023

It would break anyone's code that uses it ...

If it breaks the use case (see in the Minimal reproduction project) , I doubt someone will actually copy the transform to the target directly?

@AThousandShips
Copy link
Member

AThousandShips commented Jun 17, 2023

It isn't broken, it's just weird, you can use it fully, it being broken would mean it wouldn't return anything useable I'd argue

The people who follow the current instructions in the documentation would have their code no longer working

@AThousandShips

This comment was marked as outdated.

@capnm
Copy link
Contributor

capnm commented Jun 17, 2023

target.transform = curve.sample_baked_with_rotation(distance)
or
var tx = curve.sample_baked_with_rotation(distance)
target.position = tx.get_origin()
target.rotation = tx.get_rotation()

Works (tested only my patch) here in the MRP with same expected result:
Screenshot2

@AThousandShips
Copy link
Member

Realized that I had misinterpreted how it was used here, so changed the description in the documentation PR, a change to how the current implementation works would be good, but I still argue it would have to be delayed with consideration for compatability breaking

@0xafbf
Copy link
Contributor Author

0xafbf commented Jun 17, 2023

As you suggested, this does it for me

-	return Transform2D(side, forward, Vector2(0.0, 0.0));
+ 	return Transform2D(forward, side, Vector2(0.0, 0.0));

I agree that it breaks behavior for current users, but I also think that the sooner this gets fixed the better.

Not given the expected results of PathFollow2D, the expected results would be for it to be facing up on a flat curve, no?
I think this is debatable, and it ends up being a lot about preference.

I'm not sure what the expected result is here, but it isn't:

Why? there are many arguments in favor of this:

  • It is what PathFollow2D does.
  • It applies the least amount of transform for the simplest use case for example: a straight path from left to right wouldn't introduce any rotation.
  • not rotating an element in a left-to-right path would follow the principle of least surprise
  • nicely maps a positive value to a positive movement in an axis
  • if used for procedurally placing content, things such as trees-fences wouldn't be awkardly facing forward, but instead would naturally face up
  • it reduces the amount of code I need to write to lay out text with transforms in my other PR

Arguments in favor of keeping up as forward:

  • In top-view games people draw sprites with forward as up

In the end, if needed, rotation can be introduced by the user (although this applies both sides)

Overall I think this fix is kind-of critical for good performance, because using the get_rotation() function uses trigonometry and assigning it back to the object uses trigonometry again, which may be a concern when placing hundreds of items by code (which is what could happen with the text transform PR)

@AThousandShips
Copy link
Member

Agreed, realized that a scaling issue with the MRP caused me to fail to realize that the code as current does pointing along the path, not across it, as I thought, I was confused as to this part

@capnm
Copy link
Contributor

capnm commented Jun 18, 2023

In top-view games people draw sprites with forward as up

Flat-universe games people too. MRP + ship & rocket :)
78361-PathSample2.zip

@Calinou Calinou removed the bug label Jun 19, 2023
@YuriSizov YuriSizov added the bug label Jun 26, 2023
@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 26, 2023
@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants