Skip to content

Conversation

@smix8
Copy link
Contributor

@smix8 smix8 commented Apr 21, 2025

Improves Path2D node debug performance by changing the debug to use mesh and multimesh instead of the slower canvas line draw functions.

This PR removes the old remnants of what was a fake open-gl line anti-aliasing that no longer works (it rendered everything twice with 0.5 alpha, aka very efficient).

The PR also removes the debug path width that did not really work and caused a lot of rendering issues on high res and dpi displays or different zoom and scale levels in the editor. It is now all thin "hardware" mesh lines.

linezoom

This largely means it replaces the old debug rendering. For editing paths the internal arrays got replaced with more efficient LocalVectors that keep the memory between path edits.

The rather slow canvas item draw for each segment and point were replaced with a combination of using static meshes for path lines and several multimeshes for the rendering of the editor handles and textures.

The nodes now always render the static debug visuals while the editor plugin only renders the editing parts like handles of the selected node on top.

thick

The result is a very noticeable performance boost editing and rendering many complex 2d paths at the same time.

@Mickeon
Copy link
Member

Mickeon commented Apr 21, 2025

I personally feel like the extent in which this PR goes to customize debug drawing is quite excessive. Especially the custom sampling. It looks like something that the editor could infer automatically based on current performance, distance from the curve, overall length, and so on.

Does this PR happen to also supersede #90357 ?

@smix8
Copy link
Contributor Author

smix8 commented Apr 21, 2025

@Mickeon

Does this PR happen to also supersede 90357 ?

While there is a little crossover the linked PR for the most part changes the actual Curve resources and their baked function and point sampling, something that this PR does not touch.

I personally feel like the extent in which this PR goes to customize debug drawing is quite excessive. Especially the custom sampling. It looks like something that the editor could infer automatically based on current performance, distance from the curve, overall length, and so on.

The editor can not infer this as long as it is not living in the users head. This is not about performance only but also user intention, so it needs settings.

E.g. multiple times different people tried to convince me that a few more fish bones, a few less bones, or even so many fish bones that you can not spot the line anymore would be good and appropriated (the older PRs that reduced it to a hard-coded 4 interval had this discussions as well). Then we had people that said they needed the detail for only some important paths, and wanted all the less important to be coarse. Some said they did not care at all and wanted it off ... oh, and only for some paths btw.

With such vastly different views and uses among just a handful of people how would you automate this? it is not possible.

For improving performance there are also a lot of things still untapped, some I mentioned in the todo. The gizmo classes in general are highly inefficient the more complex the object is as they throw everything away and start from scratch on any change that causes a redraw even if just a minor change. They have like zero update granularity.

@Calinou
Copy link
Member

Calinou commented Apr 21, 2025

Can the curve sampling be automatically determined based on whether the current/previous/next points use any curve tangents? If it's a straight line, it can be drawn with a single vertex. This would also optimize line drawing for lines that only have some curved points (but not all).

Also, I think Custom Color should be moved at the top of the custom options (below Custom Enabled) as it's likely the one users will tweak the most.

@smix8
Copy link
Contributor Author

smix8 commented Apr 21, 2025

Can the curve sampling be automatically determined based on whether the current/previous/next points use any curve tangents?

Yes oc you can preprocess everything but always the question if you gain more than you lose. This info if a segment is straight or curved should likely be part of the actual Curve resource point meta cache and not of the Path2D/3D Node as you don't want to recalculated it again for every single node that uses the curve.

Also, I think Custom Color should be moved at the top of the custom options (below Custom Enabled) as it's likely the one users will tweak the most.

Agree, moved the debug_custom_color below the debug_custom_enabled in the property list.

Copy link
Member

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still very much in the opinion that all of these. I think we seriously need to be careful about property bloat. Please let's tread carefully.

At the very least, these should be surrounded with DEBUG_ENABLED (if they're not already)

The documentation review is not exhaustive but I'm getting just a few out of the way.
It feels like the descriptions were written by programmers, for programmers (and, I mean, of course they were). It's a bit hard to explain how, but it is especially problematic because these properties are prominently present in the Inspector and potentially changed by Godot developers (such as level designers), not necessarily users that know how to code.

@smix8 smix8 force-pushed the path_nodes branch 5 times, most recently from a80b044 to bfe700e Compare April 22, 2025 08:30
@smix8 smix8 force-pushed the path_nodes branch 2 times, most recently from 2f31ce4 to f4f82ec Compare June 7, 2025 09:48
@smix8 smix8 changed the title Improve Path2D/3D debug customization and performance Improve Path2D debug performance Jun 7, 2025
@smix8
Copy link
Contributor Author

smix8 commented Jun 7, 2025

After running stale because I did not really find a satisfying way to accommodate all those review comments I decided to refocus the PR entirely on its core and just keep the Path2D debug performance improvements. For stuff that was now removed I might make a dedicated PR later if it is still relevant.

@smix8 smix8 removed the topic:3d label Jun 7, 2025
@KoBeWi
Copy link
Member

KoBeWi commented Jun 9, 2025

The error spam is still not fixed.

Improves Path2D node debug performance by changing the debug to use mesh and multimesh instead of slower canvas draw functions.
@smix8
Copy link
Contributor Author

smix8 commented Jun 9, 2025

The error spam is still not fixed.

@KoBeWi I think I found the issue and fixed it now. At least I was not able to trigger such error again.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not compare performance, but it works correctly.

@KoBeWi KoBeWi removed request for a team June 9, 2025 21:21
@Repiteo Repiteo merged commit 8bff3c9 into godotengine:master Jun 9, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jun 9, 2025

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.

6 participants