Make use of NavigationObstacle2D's transform

[tracefree]

Fixes #95820

2D version of #96730. Partially an enhancement and partially a fix because previously, editing a NavigationObstacle2D was broken if it or any of its ancestor nodes had a rotation, scale, or skew applied.

The only thing missing as far as I can tell is to modify NavigationObstacle2DEditor so that the vertices are transformed by the modified safe transform (which prevents non-uniform scaling), as discussed on RocketChat.

Edit: This should now be fully consistent and have feature parity with the 3D version of this PR. But I have not tested it extensively and the code may not be completely in line with Godot's conventions and guidelines, would need someone more familiar with the codebase to review and give feedback on it please 🙂

[tracefree]

Reverted changes meant to prevent obstacle vertices to be affected by zero or negative scaling like in the 3D version and only show a warning in that case instead. The reason is that doing so would require modification to sensitive code in AbstractPolygon2DEditor which is already planned to be moved away from as a dependency for NavigationObstacle2D, it makes more sense to reinstate that behavior once that refactor has been finished.

[smix8]

When I change the NavigationObstacle rotation the radius debug changes size.

In the setup I have both the parent Node2D and the child NavigationObstacle scaled and rotated and translated.

[tracefree]

I think this may be a side effect of the logic used to ensure the radius is scaled only uniformly by the value of the largest global scale axis, but you are right that it feels strange and I'll take a look to see if that really is the case or if there is a coding mistake.

[smix8]

I guess it is just the usual flaw with the Transform2D that has no real scale compare to 3D but just a rotation depending fake scale.

[tracefree]

Yeah, it looks like that's what's happening here. For the scale factor of the radius, I take the largest value of the global_scale of the obstacle node, clamped to 0.0001 to prevent it from becoming zero or negative.

How would you like me to handle this? I don't really see another sensible way of using the transform's scale to influence the radius, but the fact that it depends on the rotation is very counter-intuitive and I imagine would lead several people to report it as a bug. Do we leave it as is, or should we maybe just leave the radius alone and let it ignore the node transform? And if the latter, should I change the 3D PR to do it like that as well for consistency?

If we stay with the current behavior I could change the warning that is already displayed when scaling a radius-based obstacle non-uniformly to also warn about unexpected behavior. Currently it just says "The agent radius can only be scaled uniformly. The largest scale value along the two axes will be used and skew is ignored."

[smix8]

I would say just leave it as is, or add extra to the warning so people know that when they scale things the screw up same as they do with physics.

[tracefree]

Alright! I expanded the warning, added an additional warning for skew, and added a clarifying comment somewhere else in the code.

[smix8]

For the transform change.

Still needs code review.

[akien-mga]

Code style seems fine to me.

[tracefree]

Thank you for reviewing! I realized there is still one more missing functionality (transforming the vertices for the avoidance system) as discussed on RocketChat, so please don't merge the PR just yet 🙂

[Repiteo]

I realized there is still one more missing functionality (transforming the vertices for the avoidance system) as discussed on RocketChat, so please don't merge the PR just yet 🙂

NOOOOOO, I SAW THIS TOO LAAAATE 😭

It's all good tho, just handle that in a separate PR & I'll fast-track a merge for it 👍

[tracefree]

Haha no problem! I should have converted the PR to a draft, my bad. I'll need to fix both this and the 3D version of this PR anyway because of rebasing issues from another PR that was just merged so a making separate PR is suitable for that anyway.

[tracefree]

This is the follow up PR that implements what I missed in this one: #99176 🙂