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

Make use of NavigationObstacle3D's transform #96730

Merged

Conversation

tracefree
Copy link
Contributor

@tracefree tracefree commented Sep 8, 2024

Fixes #85722.

(Edit: This PR is basically done, if you'd like to see it merged please test it in your own projects and let me know if it works as expected!)

Currently, the vertices of a NavigationObstacle3D are not affected by the node's transform. This severely limits their usefulness in my opinion since level designers can not place instances of pre-made obstacles in a level.

For example, let's say a project has a "fallen tree" scene which contains a NavigationObstacle3D node that is set up with vertices that match the outline of the tree. A level designer would now like to place instances of this scene in the world to affect the navigation mesh. But rotating or scaling the tree does not apply to the vertices of the obstacle, leading to incorrect navigation. They would instead have to place individual obstacle nodes and set up the vertices again for each of them, making iteration of the level layout very labor intensive.

In #82593 it was mentioned that letting the transform affect the vertices would break compatibility, but given the problem described above I would argue it is worth it. From a user perspective the current behavior is very unexpected and seems like a bug.

One remaining problem I have with my PR is that I don't know how to deal with the obstacle's radius since the node's scaling can be non-uniform. It's not as big of an issue as the vertices since you only have to adjust a single parameter so for now I did not touch it.

Edit: There now is also a 2D version at #99030.

@tracefree tracefree requested a review from a team as a code owner September 8, 2024 20:21
@smix8
Copy link
Contributor

smix8 commented Sep 8, 2024

A static avoidance obstacle is plane projected, it has no transform, especially not a 3D transform that can be rotated in all axis and corrupt the state of the obstacle easily.

@tracefree
Copy link
Contributor Author

Good point, I had not considered what would happen when rotating around any axis other than y. Then how about only using/inheriting the y-rotation? Or alternatively, transform the vertices fully and then project them to the xz-plane again. I can also see now how this will complicate the vertex editing process when the node is already rotated 🤔 I'll play around with it some more and see if I can find a good solution.

@tracefree
Copy link
Contributor Author

tracefree commented Sep 9, 2024

Okay I made it so only the global rotation around the y-axis is taken into account. I think this is the best approach since re-projecting to the xz-plane would be more complicated, confusing to users, and not very useful imo.

If rotated around another axis a configuration warning is displayed, similar to how physics bones warn about non-uniform scaling. Scaling also works as one would suspect as long as the node is only rotated around the y axis. Height is also affected by the scale.

What do you think? Have I missed anything, or done something with unintended consequences? If this approach looks reasonable to you I would proceed to adapt the vertex edit tool to be compatible with these changes 🙂

@tracefree tracefree force-pushed the properly_transforming_obstacles branch from c45981d to ef75957 Compare September 9, 2024 11:42
@smix8
Copy link
Contributor

smix8 commented Sep 10, 2024

Yes y-axis rotation alone is the only thing that makes sense for the static obstacle vertices. Rotating around other axis easily corrupts the winding breaking the obstacle behavior and potentially entire avoidance simulation.

The agent radius can only be uniformly scaled.

To not have different behavior between dynamic obstacle radius and static obstacle vertices I would say pick the largest scale axis and discard the others. That behavior is easier to communicate in documentation and scaling non uniformly is also a recipe for precision disasters.

Downscaling to zero or below needs to be prevented. We learned from physics that a noticeable amount of users have very creative ideas with scaling things to zero to "hide" it having no clue about all the systems that get broken by doing that.

For the editor plugin you likely need to counter the node transform considering that the editor gizmos transform all the handles and visuals. You can look at my older PR #93059 that improved on the editor tooling and gizmos but was not finished.

@tracefree
Copy link
Contributor Author

tracefree commented Sep 10, 2024

Alright, thanks for that link! What would be a good way to prevent scaling <= 0? I'm thinking of displaying a warning just like for invalid rotation, and to internally clamp the scale on each axis to a small positive value, say 0.001 (I think this is the smallest positive value you can enter in the editor by just dragging it with the mouse).

Edit: I just pushed my implementation of that solution, and for the uniform agent radius scaling.

@tracefree tracefree force-pushed the properly_transforming_obstacles branch from ef75957 to eeacf18 Compare September 10, 2024 16:32
@tracefree
Copy link
Contributor Author

I just discovered two interesting things about the obstacle editor plugin:

  1. Currently, in vanilla Godot 4.3, when rotating the NavigationObstacle3D the vertex edit mode is bugged and puts vertices somewhere you haven't clicked. Is there a known issue for this that can be linked to?

  2. With my PR that bug just happens to cancel out as long as the transform is valid (scaling or rotated only around y-axis), i.e. without touching the gizmo code it already works as expected in that case.

I'll try to fix that bug completely so that even when there is an unsupported rotation around the x or z axis the vertex editor keeps working.

@tracefree tracefree force-pushed the properly_transforming_obstacles branch from eeacf18 to fda46ca Compare September 10, 2024 19:28
@tracefree tracefree requested a review from a team as a code owner September 10, 2024 19:28
@tracefree tracefree force-pushed the properly_transforming_obstacles branch 2 times, most recently from 9114ab6 to d732830 Compare September 10, 2024 19:33
@tracefree
Copy link
Contributor Author

tracefree commented Sep 10, 2024

Okay, I haven't tested it out super thoroughly yet, and the code can likely be cleaner, but I think everything works as discussed now including the vertex edit mode 🙂 Some remaining minor details:

  • The AABB of NavigationObstacle3D still shows rotations around the x and z axis and is thus inaccurate. Haven't yet looked at how to update that.
  • For now I update the gizmo's mesh instance transform in forward_3d_gui_input since I already calculate the proper global transform there. Is that fine or should I do this somewhere else (like in edit())?
  • I noticed the point_lines_meshinstance has a small translation in the z-direction - why is that? Should this maybe have been in the y-direction so it doesn't intersect with the grid?
  • As far as I understand navigation meshes can be vertical as well, and obstacles work for those too, but currently the xz-plane is given special treatment in the editor. Are there plans to accommodate workflows for wall-mounted obstacles in the future? Just asking to make sure the design of my PR doesn't make such future work in that direction harder without breaking compatibility.

Edit: Thinking about the last point some more, maybe static obstacles should be able to rotate around all three axes after all, and have the vertices get projected to the plane of the navigation region 🤔 This way obstacles on walls could be supported. In the editor the obstacle would be shown with its normal rotation and you could edit the vertices on a rotated plane as well. Then in NavMeshGenerator3D where the orientation of the navmesh region is known, project vertices to that plane. Does that make sense? I'll make a proof of concept for that idea when I next have time, so please don't merge this version just yet :)

@smix8
Copy link
Contributor

smix8 commented Sep 11, 2024

I noticed the point_lines_meshinstance has a small translation in the z-direction - why is that?

For z-fighting when editor gizmo or other things all have overlap with the line meshes you get flickering without any offset. Considering that obstacles are mostly viewed from top or bottom camera angles a y-axis offset is unpractical and offsets along z-axis or x-axis work better.

As far as I understand navigation meshes can be vertical as well

Navigation meshes as resources have no transform so they are always at origin basically. They also do not know how they will be used by the navigation region or map. A navigation mesh surface normal can't be rotated 90° or more away from the current navigation map up orientation or else things start to break.

Support for "wall" or "roof" navigation mesh is out of the question. There is a reason why basically no common game engine supports this and it is only seen in highly game specific custom engines. For starters nearly the entire third party eco assumes flat plane or y-axis restrictions to keep things simple and performant. No one wants to rewrite and support all that for a couple of niche maybe-games.

@tracefree
Copy link
Contributor Author

I see! I didn't know wall navigation is not possible - I was under the impression that it was because I could rotate a NavigationRegion3D node and navmesh baking then affected the walls, but I haven't tried actually using it this way. Maybe a warning should be displayed in that case as well?

@smix8
Copy link
Contributor

smix8 commented Sep 12, 2024

Maybe a warning should be displayed in that case as well?

The navmesh baking is alread y-axis restricted by the thirdparty ReCast library. Also if the region is rotated 90° or more away from the navigation map up vector the server prints a warning.

Everything else would require to reparse and transform check every single navmesh vertex and face on the navigation map at runtime for verification, not feasible and 99.9% and projects don't even try it anyway so why make their performance worse.

@tracefree
Copy link
Contributor Author

tracefree commented Sep 13, 2024

Ah, I had missed that warning! I see it now. Okay, then I have one last question (just want to really make sure to design this PR right): Changing the navigation map up vector is supported, right? I see the default up vector can be set in the project settings. Not to navigate between floor and walls, but maybe someone wants their entire game to be aligned to the xy-plane for whatever reason. Is that possible, or is it really the y-axis that is hardcoded in ReCast (in which case my follow up question would be what a use case of changing the navigation region up vector would be)?

In case you can have a vertical navigation map, would one be expected to rotate the NavigationRegion3D before baking the navmesh? How do (or should) static navigation obstacles play into this? My intuitive guess as a user would be to rotate the obstacle so its local up vector is aligned with the navigation map up vector, but the current state of my PR would make that impossible.

@smix8
Copy link
Contributor

smix8 commented Sep 14, 2024

The option to change the navigation map up vector primarily exists for games that use custom navmesh.

It makes little sense for anything else and the vast majority of navigation related libraries do not at all support such things because it adds a major performance and maintenance problem for an absolute niche of the niche thing.

@tracefree tracefree force-pushed the properly_transforming_obstacles branch from d732830 to e8d7767 Compare September 25, 2024 18:31
@tracefree
Copy link
Contributor Author

tracefree commented Sep 25, 2024

Okay, since it seems there really is no feasible use case for ever rotating an obstacle around any axis other than y it makes sense to stick with the current implementation. I just did a rebase and also removed the dummy AABB as discussed on Rocket Chat. Hope I got everything related to that / didn't remove something unrelated.

How does this code look? Anything that can be improved or should be added before the PR can be considered for merging? Should I look into doing something similar for the 2D equivalents for example (in this or a follow up PR)? Just did a quick test and NavigationObstacle2D does seem buggy when rotating and scaling.

@tracefree tracefree force-pushed the properly_transforming_obstacles branch from e8d7767 to c6ef1c2 Compare September 25, 2024 19:17
@smix8 smix8 removed the needs work label Oct 17, 2024
@tracefree
Copy link
Contributor Author

To anyone interested in this PR: I'd really appreciate it if you could please test it in your own projects and let me know if there are any problems or if it works as expected!

@tracefree
Copy link
Contributor Author

tracefree commented Oct 28, 2024

Just pushed a fix to this PR by @a0kami (added as a co-author) who graciously tested it and found out I had missed transforming the vertices of the radius-based shape properly (using a uniformly scaled transform) and added some optimizations! I also rebased the PR on current master.

@a0kami
Copy link
Contributor

a0kami commented Oct 31, 2024

Hey, basically tested the PR as Rie asked and started writing a comment with videos showing it worked fine until I started playing with the radius property. In previous PR version, it turned out the radius defined shape could be non uniformly scaled and would not match the debug visuals.

We just made sure the obstacle's 2 geometry shapes and their 2 respective debug visual primitives were accordingly transformed each in their own way while trying to keep the code as clean as possible.
The polygon shape can be non uniformly scaled but only takes Y axis rotations; the ("cylinder") radius defined shape is only uniformly scaled taking the value from the most-highly scaled axis.

There are a few demo media on the fediverse thread documenting our journey, and even a standalone test scene if need be.
I'd like to thank everyone for the hardwork on this amazing navmesh system, cheers!

@smix8
Copy link
Contributor

smix8 commented Nov 4, 2024

Tested and works fine for horizontal node rotation and scale but both X-Axis and Z-Axis node transforms create multiple problems.

When nodes are rotated vertical e.g. X-Axis +/- the navmeshed baked objects all have the wrong shape baked. The baked shape does not match with the debug visuals from radius or vertices of the transformed obstacle.

The elevation related code also does not seem to be compatibly or updated with the new transformed obstacle height in mind, e.g if a node change rotates the obstacle height it gets ignored all to easily while the debug visuals show that it should work.

Whatever the solution the debug visuals need to match what is done by the obstacle for both navmesh baking and avoidance, it can't show something that does not at all match.

@tracefree
Copy link
Contributor Author

Thanks for testing! Strange, I thought all this worked correctly when I tested 🤔 Will investigate.

@smix8
Copy link
Contributor

smix8 commented Nov 9, 2024

If the xz-axis issues cant be solved easily I am fine with limiting it back to the y-axis for the time being and approve that.

Considering that y-axis rotation and basic scaling covers already all the more reasonable use cases it would be already a good improvement.

@tracefree
Copy link
Contributor Author

tracefree commented Nov 9, 2024

Hey, I finally got around to testing! I can't seem to reproduce the issues you describe, or maybe I'm misunderstanding the problem. I tested it by giving an obstacle vertices, then rotating it around the x, z, and a bit around the y axis, and also scaling it non-uniformly for good measure. I set the cell size to something very low and activated "Affect navmesh" and "Carve navmesh". When I bake the navmesh the hole created by the obstacle exactly fits the debug visuals (seen on the left of the screenshot).

For the height I'm not sure what the best way to test it is, but I gave an on obstacle a height of 0.1, which is too low to have an effect on the navmesh, and then scaled it along the y-axis by 10, which then again carved a hole into the navmesh (seen on the right).

Screenshot_20241109_173152

Here's the project I tested with, using this PR rebased on current upstream master:
obstacle_test.zip

Am I testing wrong, or maybe there are different debug visuals somewhere that I missed?

@tracefree
Copy link
Contributor Author

By the way, during testing I discovered an entirely different bug: somehow when I drew the vertices for the second obstacle I saw two debug visuals for it, one them farther away. Going to investigate that next.

@tracefree tracefree force-pushed the properly_transforming_obstacles branch from ab569ff to 30b9d91 Compare November 10, 2024 10:55
@tracefree
Copy link
Contributor Author

tracefree commented Nov 10, 2024

By the way, during testing I discovered an entirely different bug: somehow when I drew the vertices for the second obstacle I saw two debug visuals for it, one them farther away. Going to investigate that next.

I couldn't manage to reproduce this bug again, and I'm not sure if it's even related to this PR so unless I run into it again I guess we will need to wait for someone else to run into it and make an MRP so we can fix it in a different PR. It wasn't a breaking issue in any case, just a weird visual glitch.

I rebased on current master branch and cleaned up dead code that didn't do anything anymore after my previous changes, and reordered a check to see if the polygon is empty before calling surface_begin to avoid an error complaining about ending the surface without adding any vertices.

Edit: Also I just prevented drawing the AABB of the debug visual mesh by adding it as a child of the scene tree root instead of the edited node.

Edit 2: Reverted my change to the AABB situation because it will be solved by #93059 anyway and this way rebasing will be easier.

@tracefree tracefree force-pushed the properly_transforming_obstacles branch 2 times, most recently from 59fb290 to 5758161 Compare November 10, 2024 12:24
@smix8 smix8 modified the milestones: 4.x, 4.4 Nov 10, 2024
@smix8
Copy link
Contributor

smix8 commented Nov 10, 2024

Works fine within the transform limitations that exist.

Still needs a code review.

Co-authored-by: a0kami <dev.aokami@netc.fr>
@tracefree tracefree force-pushed the properly_transforming_obstacles branch from 5758161 to 44ef3d3 Compare November 10, 2024 16:08
@Repiteo Repiteo merged commit 848d908 into godotengine:master Nov 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NavigationObstacle3D no longer applies transform rotation to its vertices in 4.2
5 participants