-
-
Notifications
You must be signed in to change notification settings - Fork 23.9k
Improve NavigationAgent2D debug performance #107290
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
Conversation
f08290e to
02c5dec
Compare
doc/classes/NavigationAgent2D.xml
Outdated
| <member name="debug_path_custom_line_width" type="float" setter="set_debug_path_custom_line_width" getter="get_debug_path_custom_line_width" default="-1.0" deprecated="Deprecated and has no effect."> | ||
| Deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO deprecated APIs in principle should still be functional, they are just flagged as not recommended anymore (and under the risk of later removal). An API that's outright removed/unsupported would be "obsoleted", and that's not a concept we use because at that point we might as well just remove it.
While looking for deprecation vs obsolescence definitions though, I found https://github.com/mdn/content/blob/main/files/en-us/mdn/writing_guidelines/experimental_deprecated_obsolete/index.md?plain=1#L38-L50 where the descriptions match my understanding but it seems they also stopped using "obsolete".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view it can be removed. I just want to avoid that the hypothetical project that edits this property (I dont know any) runs into a script error instead of a depr warning or creates an issue not knowing why their line width is no longer working. Maybe it should be "obsoleted" for one version as a warning and removed in the next. In the linked example it reads "and may still work" so that depr stuff always works is at least not part of their definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <member name="debug_path_custom_line_width" type="float" setter="set_debug_path_custom_line_width" getter="get_debug_path_custom_line_width" default="-1.0" deprecated="Deprecated and has no effect."> | |
| Deprecated. | |
| <member name="debug_path_custom_line_width" type="float" setter="set_debug_path_custom_line_width" getter="get_debug_path_custom_line_width" default="-1.0" deprecated="Deprecated and has no effect."> | |
| This property does nothing. |
And, instead of "Deprecated and has no effect." it needs an actual proper explanation as to why that is, as well as providing alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no alternative as mentioned in the description as the mesh rendering does not support line width. The width is controlled by the "hardware" and always the same thin width no matter the display or zoom level. See PR #105606 for Path2D that does the same and has images to show what is the issue with this canvas item line width and why it is so bad for debug (apart from performance) for displays and zoom levels that are not super vanilla.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, as the worst case scenario, omit the message, like deprecated="". It's not actually obligatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Improves NavigationAgent2D debug performance by using static mesh and multimesh.
02c5dec to
1c9bc22
Compare
Improves NavigationAgent2D debug performance by using static mesh and multimesh.
The old debug used the classic canvas draw functions for line segments and rects. Those are really inefficient for longer paths due to each creating either a new internal throwaway mesh or a new draw command.
This PR changes the agent path debug to a single static mesh for all the line segments, aka a single draw.
All the path points reuse a single rect mesh that is rendered with a multimesh.
Should be a net-gain for all projects but projects with a lot of agents and longer path stability (aka not querying a new path every frame like a madman) will see the biggest performance boost by this change.
Since line width is not supported by mesh line primitives (and never worked great to begin with on a lot of hardware) the option is deprecated and hidden from the inspector.