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

CanvasItem::draw_polyline Support thin polylines drawn using line strip #71679

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Jan 19, 2023

Follow-up to #69851 aiming to solve the remaining parts:

  • CanvasItem.draw_polyline ❌ - The thin option is not supported.
  • CanvasItem.draw_polyline_colors ❌ - The thin option is not supported.
  • CanvasItem.draw_arc ❌ - The thin option is not supported.
  • RenderingServer.canvas_item_add_polyline ❌ - The thin option is not supported.

This PR makes the width parameter for the methods listed above work consistently with the other methods changed in #69851 i.e. width < 0 results in drawing thin polyline using line strip (arc is drawn as polyline too). For width >= 0 a scalable polyline is drawn using triangle strips, this is unchanged.

In the changed methods I've made antialiasing not supported for width < 0. In some methods unchanged by this PR e.g. in CanvasItem::draw_line antialiasing for width < 0 is "supported" but it's done in the same manner as for the width >= 0 case. This results in a buggy behavior because at different zoom levels the line itself remains thin but the triangle strips used for antialiasing do scale. Hence I haven't added similar antialiasing "support" for thin polylines.
I haven't changed other methods but I do think it would make sense to make them consistent by making antialiasing not supported for width < 0 (can be done in a separate PR).

Compat-breaking because just as #69851 this PR changes the default value for width to -1.0 so the thin polylines are drawn by default (so if someone was not passing a width then now they will get unscalable thin polylines instead of the scalable ones).

Resolves #44332.
Resolves godotengine/godot-proposals#1363 (combined with #69851).

cc @dalexeev

Bugsquad edit:
Fully resolves #19844 (according to @dalexeev)

@dalexeev
Copy link
Member

I have tested this PR, works correctly and consistent with #69851.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good to me. Should be fine to merge once the error is changed to a warning (or an ERR_CONTINUE if you prefer)

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great!

@akien-mga akien-mga merged commit 9fc4012 into godotengine:master Jan 20, 2023
@akien-mga
Copy link
Member

Thanks!

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