godot: sample_baked_with_rotation gives wrong transform

Godot version

4.0.3.stable

System information

Windows 10

Issue description

evaluating a curve’s sample_baked_with_rotation gives wrong Transform2D:

https://github.com/godotengine/godot/assets/1028756/078552a4-3965-4d36-9446-022054ade672

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 https://github.com/godotengine/godot/pull/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

About this issue

  • Original URL
  • State: closed
  • Created a year ago
  • Comments: 20 (20 by maintainers)

Most upvoted comments

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)

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

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

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 …

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

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?

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