Fix scaling issue in `draw_line` and similar methods #69851

dalexeev · 2022-12-10T12:00:39Z

CanvasItem.draw_line and a few other methods have width parameter. There is a check in the current implementation: if width is greater than 1, a four-point primitive is drawn, otherwise a two-point primitive. This causes the lines of width <= 1 to remain thin when the CanvasItem is scaled, while the thickness of other lines scales proportionately. This issue has been reported several times, for example #51978.

While in some cases this behavior is desirable, in other cases (such as pixel games) it can be inconvenient and inconsistent (you cannot use values between 0.0 and 1.0). The only workaround for this problem is a hack with width = 1.002.

This PR implements kleonc's suggestion to change the API as follows. Change the default value of the width parameter to -1 and treat negative values as an indicator of a two-point line primitive. In this case, if you explicitly specify width = 1, then the line will scale correctly.

List of methods to check (which have width parameter and are related to line drawing)

CanvasItem.draw_line ✔️
CanvasItem.draw_dashed_line ✔️
CanvasItem.draw_multiline ✔️
CanvasItem.draw_multiline_colors ✔️
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.
CanvasItem.draw_primitive ✔️ - Removed unused width parameter.
CanvasItem.draw_rect ✔️ - The thin option does not work perfectly, there is a note in the docs.
RenderingServer.canvas_item_add_line ✔️
RenderingServer.canvas_item_add_multiline ✔️
RenderingServer.canvas_item_add_polyline ❌ - The thin option is not supported.
RenderingServer.canvas_item_add_primitive ✔️ - Removed unused width parameter.

MewPurPur · 2022-12-10T12:04:04Z

For completeness, this is kind of a re-implementation of #44361 (which was closed because the PR author lost interest)

scene/resources/convex_polygon_shape_2d.cpp

dalexeev · 2022-12-10T15:44:21Z

This changes the public API, so the "breaks compat" label is needed (however, the default behavior remains the same).

kleonc · 2022-12-10T19:13:29Z

Indeed the current approach in this PR (which I suggested) is compat breaking as it changes the behavior for cases where user explicitely passes width from (0, 1.001] range. Before it would result in drawing a line primitive, in this PR it results in drawing a quad.

A possible non compat breaking alternative suggested in godotengine/godot-proposals#1363 (comment):

From what I can gather from previous discussions, we could simply add an extra argument to this method and CanvasItem::draw_line which would allow the first branch to be taken in any case (disabled by default). This way we'll be able to:
* Keep the established convention by default to avoid breaking existing functionalities

* Keep the optimized path whenever possible by default (simple line with 1px width is much faster)

* Make it easy to document and easy to understand when reading the code

So e.g draw_line's signature would be changed from:

draw_line(Vector2 from, Vector2 to, Color color, float width=1.0, bool antialiased=false)

to:

draw_line(Vector2 from, Vector2 to, Color color, float width=1.0, bool antialiased=false, bool force_quad=false)

Bool parameters are cryptic/meh and with such change when someone always wants a quad then they would also be force to specify the antialiased parameter but it's still likely a less of a problem than changing the behavior for some existing use cases.

CanvasItem.draw_rect x - "thin rectangle" is not easily feasible

@dalexeev I think you've misunderstood me. I was talking about if it would need to be done automatically. But exposing the choice to the user is as simple as for draw_line and other similar methods. It's just a matter of passing the relevant draw_rect's parameter to its internal draw_line calls.

dalexeev · 2022-12-10T21:45:18Z

I think you've misunderstood me. I was talking about if it would need to be done automatically. But exposing the choice to the user is as simple as for draw_line and other similar methods. It's just a matter of passing the relevant draw_rect's parameter to its internal draw_line calls.

The difficulty with the "thin rectangle" is that the offset must depend on the CanvasItem's scale, otherwise the rectangle will be scaled into 4 separate lines. This requires more complex code as far as I understand.

kleonc · 2022-12-10T22:28:44Z

The difficulty with the "thin rectangle" is that the offset must depend on the CanvasItem's scale, otherwise the rectangle will be scaled into 4 separate lines. This requires more complex code as far as I understand.

If you're referring to something like this it could be handled by just zeroing the offset used in the relevant part of draw_rect. Something like: https://github.com/godotengine/godot/pull/41239/files#r1045144295.

dalexeev · 2022-12-10T22:35:54Z

If you're referring to something like this it could be handled by just zeroing the offset

The lines will overlap in the corners, which will be noticeable with a translucent color.

dalexeev · 2022-12-11T06:39:18Z

A possible non compat breaking alternative suggested in godotengine/godot-proposals#1363 (comment):

From what I can gather from previous discussions, we could simply add an extra argument to this method and CanvasItem::draw_line which would allow the first branch to be taken in any case (disabled by default).

There are 3 options:

Separate methods (for example, draw_line and draw_thin_line). Pros: Clearer API. Cons: breaks compatibility, significantly increases the number of methods, requires revision of all places where methods are used.
Optional parameter. Pros: does not break compatibility. Cons: inconvenient to use (you already listed the problems).
The current option (-1 by default, negative width is treated as a two-point primitive indicator). Pros: quite convenient and logical, does not require revision of all places where methods are used. Cons: Formally breaks compatibility, although the default behavior does not change (if width is not specified).

dalexeev · 2022-12-12T09:41:39Z

Test project: fix-scaling-issue.zip

Notes:

draw_polyline* and draw_arc are left unchanged, since they do not have a "thin option" implemented in master. In these methods, -width is equivalent to width. These methods probably need to be changed too (see Restored thin polylines if line width <= 0 #44361), but I'm not sure if I have the skills to implement this.
draw_multiline_colors and canvas_item_add_multiline have a bug where thin lines use white instead of the specified colors. The bug is in master too, can be fixed independently.
draw_rect - See the discussion above.
draw_primitive - canvas_item_add_primitive does not use the p_width parameter. ~~Should it be removed?~~ Done.

godot/servers/rendering/renderer_canvas_cull.cpp

Lines 1316 to 1343 in 97df6de

    
           void RendererCanvasCull::canvas_item_add_primitive(RID p_item, const Vector<Point2> &p_points, const Vector<Color> &p_colors, const Vector<Point2> &p_uvs, RID p_texture, float p_width) { 
        
           	uint32_t pc = p_points.size(); 
        
           	ERR_FAIL_COND(pc == 0 || pc > 4); 
        
           	Item *canvas_item = canvas_item_owner.get_or_null(p_item); 
        
           	ERR_FAIL_COND(!canvas_item); 
        
           	Item::CommandPrimitive *prim = canvas_item->alloc_command<Item::CommandPrimitive>(); 
        
           	ERR_FAIL_COND(!prim); 
        
           	for (int i = 0; i < p_points.size(); i++) { 
        
           		prim->points[i] = p_points[i]; 
        
           		if (i < p_uvs.size()) { 
        
           			prim->uvs[i] = p_uvs[i]; 
        
           		} 
        
           		if (i < p_colors.size()) { 
        
           			prim->colors[i] = p_colors[i]; 
        
           		} else if (p_colors.size()) { 
        
           			prim->colors[i] = p_colors[0]; 
        
           		} else { 
        
           			prim->colors[i] = Color(1, 1, 1, 1); 
        
           		} 
        
           	} 
        
           	prim->point_count = p_points.size(); 
        
           	prim->texture = p_texture; 
        
           }

dalexeev · 2023-01-09T20:45:39Z

@clayjohn @kleonc Could you review or at least comment on this PR please? This is needed for #41239 and issues like #71131.

See also this comment about polyline.

clayjohn · 2023-01-10T20:30:10Z

This change makes sense to me. It is a very small breaking change, but indeed it is very counterintuitive that a line of width 1.002 would scale with the canvasitem while a line of 0.999 won't.

clayjohn

I think we should merge this before 4.0. It is a breaking change, but the new behaviour makes much more sense, and it likely won't break many projects because the only change in behaviour will be for people using this with a width (0.0, 1.001] (which with the old behaviour is the exact same as using the default of 1.0, but now will actually scale).

clayjohn · 2023-01-14T01:02:19Z

Will just need to be rebased onto current master

dalexeev · 2023-01-14T05:57:54Z

Rebased. The only thing that bothers me is that the polyline and arc methods remain inconsistent.

doc/classes/CanvasItem.xml

kleonc · 2023-01-14T17:49:49Z

servers/rendering/renderer_canvas_cull.cpp

@@ -597,7 +597,7 @@ void RendererCanvasCull::canvas_item_add_line(RID p_item, const Point2 &p_from,
 	Vector2 end_left;
 	Vector2 end_right;

-	if (p_width > 1.001) {
+	if (p_width >= 0.0) {


@clayjohn Should we maybe not draw anything when p_width == 0.0? Probably this is such a corner case that it's not worth checking but I wonder what's your opinion on this.

kleonc · 2023-01-14T18:46:11Z

servers/rendering/renderer_canvas_cull.cpp

@@ -628,7 +628,7 @@ void RendererCanvasCull::canvas_item_add_line(RID p_item, const Point2 &p_from,
 		// This value is empirically determined to provide good antialiasing quality
 		// while not making lines appear too soft.
 		float border_size = 1.25f;
-		if (p_width < 1.0f) {
+		if (0.0f <= p_width && p_width < 1.0f) {


Note that this doesn't change the previous behavior for default thin lines, meaning antialiasing for them is still kinda broken. Here's the same zoomed in line drawn in v4.0.beta11.official [91713ce] with width = 1.0 and without/with antialiasing (with this PR the default width = -1.0 will give the same result):

It should be fine to leave it like that in this PR. Just wanted to point it out, maybe we should somehow address antialiasing thin lines later, in a separate PR.

kleonc · 2023-01-14T19:06:20Z

scene/main/canvas_item.cpp

@@ -556,6 +556,7 @@ void CanvasItem::draw_multiline_colors(const Vector<Point2> &p_points, const Vec

 void CanvasItem::draw_rect(const Rect2 &p_rect, const Color &p_color, bool p_filled, real_t p_width) {
 	ERR_FAIL_COND_MSG(!drawing, "Drawing is only allowed inside NOTIFICATION_DRAW, _draw() function or 'draw' signal.");
+	ERR_FAIL_COND_MSG(p_width < 0, "The draw_rect() \"width\" argument must be non-negative.");


I'm not convinced about disallowing to draw a rect with line primitives. I think it would make more sense to make the p_width work the same as in draw_line etc. and just add a relevant note in draw_rect's docs about its limitations (like potential brighter corner for translucent colors caused by the overlap, missing pixels at the corners, etc.).

E.g. for debugging you often don't really care about these issues, you just want to draw a simple rect (and width not scaling with the zoom might be desired).

Also with such change now by default non-filled rects would be drawn as quads so I wonder how it would affect all built-in controls / editor plugins. I think many of them could be using draw_rect with default width. It could also affect performance.

So yeah, I think we should allow thin rects despite of all their quirks.

Implementation-wise: just draw the thin sides with zero offsets (right from the corners) and accept this often won't be perfect.

scene/resources/convex_polygon_shape_2d.cpp

kleonc · 2023-01-14T19:28:41Z

The only thing that bothers me is that the polyline and arc methods remain inconsistent.

@dalexeev PRIMITIVE_LINE_STRIP is available in the RenderingServer:

godot/servers/rendering_server.h

Line 288 in 60d0317

PRIMITIVE_LINE_STRIP,

so it shouldn't be hard to make polyline and arc methods interpret the width in the same manner.

kleonc

LGTM, thanks @dalexeev for all the patience with the subject. 🙂

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.

Adding support for thin lines in polylines/arc (in a manner consistent with this PR) is a potential material for a separate PR.

akien-mga · 2023-01-16T12:15:22Z

Thanks!

akien-mga · 2023-01-16T12:18:46Z

Does this fix either of the related issues, or it's only the first step towards fixing them?

draw_rect is drawn at wrong position. Also size and shape is wrong if filled parapeter is false. #35384
CanvasItem.draw_polyline does not draw 1px lines when width<=1.0 #44332
A top corner pixel is missing in non-filled CanvasItem.draw_rect() #58775

dalexeev · 2023-01-16T12:23:46Z

#44332 is definitely not fixed, #35384 and #58775 are mostly fixed by this PR (for thick rectangle), but maybe we can come up with something better later on for thin rectangle?

kleonc · 2023-01-16T18:59:18Z

#44332 would be fixed by adding support for thin lines in polylines. I can tackle this (+arc). Unless @dalexeev you've already started doing so?

dalexeev · 2023-01-16T19:22:37Z

No, I didn't start. I can try later, but if someone has a desire to deal with this, I will be glad, as I am not sure of my skills.

kleonc · 2023-01-16T19:57:09Z

No, I didn't start. I can try later, but if someone has a desire to deal with this, I will be glad, as I am not sure of my skills.

Yeah, I'll (try to) do it.

dalexeev commented Dec 10, 2022

View reviewed changes

scene/resources/convex_polygon_shape_2d.cpp Outdated Show resolved Hide resolved

Chaosus added enhancement topic:core topic:rendering topic:2d labels Dec 10, 2022

kleonc added the breaks compat label Dec 10, 2022

kleonc mentioned this pull request Dec 10, 2022

Fix CanvasItem.draw_rect function with filled = false #41239

Merged

dalexeev force-pushed the fix-scaling-issue branch from 66ee292 to 52ca69d Compare December 12, 2022 09:09

dalexeev marked this pull request as ready for review December 12, 2022 09:41

dalexeev requested review from a team as code owners December 12, 2022 09:41

dalexeev force-pushed the fix-scaling-issue branch 2 times, most recently from 36e4f2a to d4b01e5 Compare December 12, 2022 15:52

Chaosus added this to the 4.0 milestone Dec 13, 2022

clayjohn approved these changes Jan 14, 2023

View reviewed changes

dalexeev force-pushed the fix-scaling-issue branch from d4b01e5 to 2743f53 Compare January 14, 2023 05:53

kleonc reviewed Jan 14, 2023

View reviewed changes

dalexeev force-pushed the fix-scaling-issue branch from 2743f53 to ac0b82f Compare January 16, 2023 07:01

kleonc approved these changes Jan 16, 2023

View reviewed changes

Fix scaling issue in draw_line and similar methods

a8cbb62

dalexeev force-pushed the fix-scaling-issue branch from ac0b82f to a8cbb62 Compare January 16, 2023 09:50

kleonc approved these changes Jan 16, 2023

View reviewed changes

akien-mga merged commit 673d9c5 into godotengine:master Jan 16, 2023

dalexeev deleted the fix-scaling-issue branch January 16, 2023 12:16

This was referenced Jan 16, 2023

A top corner pixel is missing in non-filled CanvasItem.draw_rect() #58775

Open

draw_rect is drawn at wrong position. Also size and shape is wrong if filled parapeter is false. #35384

Closed

This was referenced Jan 18, 2023

Lines too thick in "View UV1/UV2" menus #71647

Closed

Use a fixed width of 1 pixel for editor debug UV drawing #71648

Merged

kleonc mentioned this pull request Jan 19, 2023

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

Merged

This was referenced Jan 19, 2023

using "draw_line" function draws lines with incorrect width (and scales incorrectly on some values) #19844

Closed

Give an option to scale custom drawing like the rest of the viewport godotengine/godot-proposals#2391

Open

aaronfranke mentioned this pull request Jan 31, 2023

CollisionShape2D debug drawing exhibits unstable graphical glitches #56320

Open

dalexeev mentioned this pull request Jan 31, 2023

Fix polyline stroke #63425

Closed

YuriSizov mentioned this pull request Feb 22, 2023

line_draw always draws with width=1 #8289

Closed

dalexeev mentioned this pull request Feb 22, 2023

CanvasItem.draw_multiline width and antialiasing not functional #16448

Closed

dalexeev mentioned this pull request Apr 4, 2023

Vulkan: Support drawing thin/Bresenham lines on NVIDIA #75654

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scaling issue in `draw_line` and similar methods #69851

Fix scaling issue in `draw_line` and similar methods #69851

dalexeev commented Dec 10, 2022 •

edited

Loading

MewPurPur commented Dec 10, 2022 •

edited

Loading

dalexeev commented Dec 10, 2022

kleonc commented Dec 10, 2022

dalexeev commented Dec 10, 2022

kleonc commented Dec 10, 2022 •

edited

Loading

dalexeev commented Dec 10, 2022

dalexeev commented Dec 11, 2022

dalexeev commented Dec 12, 2022 •

edited

Loading

dalexeev commented Jan 9, 2023 •

edited

Loading

clayjohn commented Jan 10, 2023

clayjohn left a comment

clayjohn commented Jan 14, 2023

dalexeev commented Jan 14, 2023

kleonc Jan 14, 2023

kleonc Jan 14, 2023

kleonc Jan 14, 2023 •

edited

Loading

dalexeev Jan 16, 2023

kleonc commented Jan 14, 2023 •

edited

Loading

kleonc left a comment •

edited

Loading

akien-mga commented Jan 16, 2023

akien-mga commented Jan 16, 2023

dalexeev commented Jan 16, 2023 •

edited

Loading

kleonc commented Jan 16, 2023 •

edited

Loading

dalexeev commented Jan 16, 2023

kleonc commented Jan 16, 2023

Fix scaling issue in draw_line and similar methods #69851

Fix scaling issue in draw_line and similar methods #69851

Conversation

dalexeev commented Dec 10, 2022 • edited Loading

MewPurPur commented Dec 10, 2022 • edited Loading

dalexeev commented Dec 10, 2022

kleonc commented Dec 10, 2022

dalexeev commented Dec 10, 2022

kleonc commented Dec 10, 2022 • edited Loading

dalexeev commented Dec 10, 2022

dalexeev commented Dec 11, 2022

dalexeev commented Dec 12, 2022 • edited Loading

dalexeev commented Jan 9, 2023 • edited Loading

clayjohn commented Jan 10, 2023

clayjohn left a comment

Choose a reason for hiding this comment

clayjohn commented Jan 14, 2023

dalexeev commented Jan 14, 2023

kleonc Jan 14, 2023

Choose a reason for hiding this comment

kleonc Jan 14, 2023

Choose a reason for hiding this comment

kleonc Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

dalexeev Jan 16, 2023

Choose a reason for hiding this comment

kleonc commented Jan 14, 2023 • edited Loading

kleonc left a comment • edited Loading

Choose a reason for hiding this comment

akien-mga commented Jan 16, 2023

akien-mga commented Jan 16, 2023

dalexeev commented Jan 16, 2023 • edited Loading

kleonc commented Jan 16, 2023 • edited Loading

dalexeev commented Jan 16, 2023

kleonc commented Jan 16, 2023

Fix scaling issue in `draw_line` and similar methods #69851

Fix scaling issue in `draw_line` and similar methods #69851

dalexeev commented Dec 10, 2022 •

edited

Loading

MewPurPur commented Dec 10, 2022 •

edited

Loading

kleonc commented Dec 10, 2022 •

edited

Loading

dalexeev commented Dec 12, 2022 •

edited

Loading

dalexeev commented Jan 9, 2023 •

edited

Loading

kleonc Jan 14, 2023 •

edited

Loading

kleonc commented Jan 14, 2023 •

edited

Loading

kleonc left a comment •

edited

Loading

dalexeev commented Jan 16, 2023 •

edited

Loading

kleonc commented Jan 16, 2023 •

edited

Loading