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

Preview resolution value for curves to alleviate performance issues of long curves #90357

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AyyZerrAsa
Copy link

@AyyZerrAsa AyyZerrAsa commented Apr 7, 2024

Fixes #81707
Curves now preview in editor with a fixed resolution per curve section that can be defined by the user. This helps avoid the issue with long curves producing a very large amount of primitives/draw items, which decrease the responsiveness of editing path3d/path2d curves.

Godot.Path3d.Fix_2.mp4

@AyyZerrAsa AyyZerrAsa marked this pull request as ready for review April 7, 2024 18:45
@AyyZerrAsa AyyZerrAsa requested review from a team as code owners April 7, 2024 18:45
@AyyZerrAsa
Copy link
Author

Would be best to wait for this change before attempting a merge #86195

@AyyZerrAsa
Copy link
Author

Unsure why the unit test is failing with logged: Unnamed argument in position 0 of method 'Curve2D.set_preview_resolution'.
Also checked with #86195 and a change will definitely need to be made to support the closed curve logic.

@AThousandShips
Copy link
Member

You need to add the argument to the bind:

ClassDB::bind_method(D_METHOD("set_preview_resolution", "resolution"), &Curve2D::set_preview_resolution);

doc/classes/Curve2D.xml Outdated Show resolved Hide resolved
…sues of long curves.

Curves now preview in editor with a fixed resolution per curve section that can be defined by the user.
@KoBeWi KoBeWi modified the milestones: 4.3, 4.4 Jul 28, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 739019e), it works as expected.

path_3d_detail_settings.mp4

One concern I have though is that the debug_preview_resolution property is part of the Curve2D/Curve3D resource, while I think it should be part of the Path2D/Path3D node, like #82321 does for its debug color property. The resource does not have any visual representation of its own, so a subdivision count property feels out of place (and may be confused with the existing Bake Resolution).

@Mickeon
Copy link
Contributor

Mickeon commented Aug 9, 2024

I don't see the point in not having this as an editor setting, instead.

@Calinou
Copy link
Member

Calinou commented Aug 10, 2024

I don't see the point in not having this as an editor setting, instead.

You may need per-path adjustments to ensure performance remains good in the editor (while preserving as much detail as possible), as some paths can be very long and complex while others can be shorter.

Comment on lines +302 to +303
real_t offset;
point == 0 ? offset = 0 : offset = c->get_baked_distance_at_point(point);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
real_t offset;
point == 0 ? offset = 0 : offset = c->get_baked_distance_at_point(point);
real_t offset = point == 0 ? 0 : c->get_baked_distance_at_point(point);

Don't use assignment inside a ternary

Comment on lines +122 to +123
real_t offset;
point == 0 ? offset = 0 : offset = curve->get_baked_distance_at_point(point);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
real_t offset;
point == 0 ? offset = 0 : offset = curve->get_baked_distance_at_point(point);
real_t offset = point == 0 ? 0 : curve->get_baked_distance_at_point(point);

@kleonc
Copy link
Member

kleonc commented Sep 18, 2024

@AyyZerrAsa Are you planning to apply suggestions / improve this PR?

What I don't like about this PR is that no matter how short given path segment is, it would still draw all debug_preview_resolution fishbones for it. It might make sense to add some lower bound for the sampling interval length.

Also another missing optimization/simplification is using only a single segment for path segments with relevant out/in zero vectors (they're straight line segments, no need to subdive them).

+I support godotengine/godot-proposals#9459, disabling the fishbones/arrows drawing should be possible one way or another (whether by a project setting, or per Path2D/3D property (like the one being added in this PR)).

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.

Path3D has editor performance issues when points are added over a long distance
6 participants