Fix `CanvasItem.draw_rect` function with `filled = false` #41239

dalexeev · 2020-08-14T00:56:57Z

Special thanks to @clayjohn and @kleonc for the explanations.

aaronfranke · 2021-02-27T17:21:36Z

@dalexeev Is this still desired? If so, it needs to be rebased and tested on the latest master branch. While there are no conflicts, rebasing is important so that reviewers can easily test the PR.

dalexeev · 2021-03-29T13:23:12Z

R^{E^BA}S_{E_D}A^{N^D}R_{E_AD}Y^{^FO}R_{_RE}V^{I^EW}

scene/main/canvas_item.cpp

aaronfranke

Using the code from #35384 on this PR, if I have the base window size set to be small and let Godot stretch the Window to make things more visible, it looks correct:

However, if I scale up the Node2D instead of stretching the window, it doesn't look quite right:

I'm not super familiar with this function so I'm not sure if it's supposed to draw "one pixel thick" on the screen or relative to the Node2D, but this output is definitely not either.

Also, in addition to the blue rectangle not looking correct, the green one is drawn using filled set to false, but a size of one, but it looks like it's filled. If I change it to a size of not 1 it looks similarly broken to how the blue one is broken.

scene/main/canvas_item.cpp

dalexeev · 2021-08-22T07:34:37Z

Thanks for the review. It looks like the bug in the canvas_item_add_line function, it does not take into account scaling for 1 pixel lines, but does take into account other transformations (for example rotation).

Green and yellow lines are 1 pixel thick, blue and pink lines are 2 pixel thick. Node2D is scaled.

If I set the border thickness to 2 pixels, then the rectangle is drawn correctly:

I'll take a look at the canvas_item_add_line function, but I'm not sure if I can fix this error.

scene/main/canvas_item.cpp

dalexeev · 2021-08-22T08:24:12Z

Also I would like to know which option is better.

dalexeev · 2022-10-04T17:34:57Z

I fixed the bug, but the solution is quite hacky. The main drawback is p_width rounding.

Also, there is a hack to check the width is 1 due to #51978:

godot/servers/rendering/renderer_canvas_cull.cpp

Lines 582 to 602 in 0056acf

    
           if (p_width > 1.001) { 
        
           	begin_left = p_from + t; 
        
           	begin_right = p_from - t; 
        
           	end_left = p_to + t; 
        
           	end_right = p_to - t; 
        
           	line->points[0] = begin_left; 
        
           	line->points[1] = begin_right; 
        
           	line->points[2] = end_right; 
        
           	line->points[3] = end_left; 
        
           	line->point_count = 4; 
        
           } else { 
        
           	begin_left = p_from; 
        
           	begin_right = p_from; 
        
           	end_left = p_to; 
        
           	end_right = p_to; 
        
           	line->points[0] = p_from; 
        
           	line->points[1] = p_to; 
        
           	line->point_count = 2; 
        
           }

In general, with integer scaling factors, everything is displayed correctly. For non-integer scales, lines can be drawn with different widths due to inaccurate mapping to the pixel grid. But I'm not sure if this problem can be solved at all.

With the current behavior, errors occur even at integer scales and linewidths, especially if the color is translucent.

MewPurPur · 2022-12-01T02:15:58Z

Does this fix #58775?

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

Or maybe not and it needs to be rebased on top of #69851?

dalexeev · 2023-01-16T12:20:36Z

There are a couple of things in this PR that are not in #69851. I will rebase it soon.

dalexeev · 2023-01-16T13:27:02Z

Essentially this PR is now cosmetic, the main bug was fixed in #69851. For negative widths, #58775 remains, but there is now a note about it in the docs. @kleonc, is there anything else we can do to reduce the chance of missing corners?

kleonc · 2023-01-16T18:25:45Z

Essentially this PR is now cosmetic, the main bug was fixed in #69851. For negative widths, #58775 remains, but there is now a note about it in the docs. @kleonc, is there anything else we can do to reduce the chance of missing corners?

I think #58775 is not really solvable in a general case. It's not just a matter of obtaining proper final positions of the line endpoints in the coordinate space of the viewport/framebuffer/render target (or whatever you call it) which would be needed to be able to apply a proper offset in that space (small final offset; regardless of the zoom etc.). What we'd also need to know to determine what such offset should be is the exact algorithm used by the Vulkan/OpenGL/etc. for rasterizing the line primitives. And that's problematic, because these can differ between implementations since specifications aren't super strict:

So even assuming that rasterization is done according to the diamond exit rule (which is not guaranteed; but maybe it's actually the most common case?:thinking: no idea about that) we would still need to find a way to somehow obtain the final positions of the line endpoints within the fragments' coordinate space (I don't see how we could currently do it). And then we would still need to find a way of how to offset the vertices (probably branching into many different cases) to always produce satisfying fragments. E.g. take a look at the image from this post and imagine all possible configurations of the vertices (colored dots); and yeah, the rect can end up being transformed in there (so take into consideration rotations or... skewing; yeah, good luck with that 😄).

TL;DR: I don't see us solving it. Would love to be surprised by someone but I don't think it will happen.

For a non-general solution / hacky solution what maybe could be done is adding a method (are a bool param to the current draw_rect) which would be supposed to work properly only in a very specific conditions: when the given CanvasItem is not transformed so the offset we would apply to the vertices would actually be equivalent to the offset in the fragments' space. Still it wouldn't be guaranteed to always work because of the loose specifications mentioned previously. Anyway, I'm not convinced that trying to handle such edge case is a good idea.

Regarding #35384 I guess it's solved by combination of #69851 + this PR + godotengine/godot-docs#6455?

kleonc

There's a question whether we should handle rects with negative size properly? Currently (AFAICT, haven't tested) only filled ones would be drawn correctly (I mean that rect with negative size would be drawn the same as rect.abs()). Shouldn't be hard to handle them too.

dalexeev · 2023-01-17T06:25:32Z

There's a question whether we should handle rects with negative size properly?

Fixed. Rect2 rect = p_rect.abs(); is OK?

kleonc · 2023-01-17T15:39:22Z

Fixed. Rect2 rect = p_rect.abs(); is OK?

I guess it should be fine, that's a quite cheap call. Potentially could be done just for non-filled rects though.

Also does changing how we draw the border for width >= 0.0 fixes some specific case (after #69851)?:thinking:

(left=before; right=this PR)

I wonder if there is any specific reason to prefer one over the other.

dalexeev · 2023-01-17T15:44:52Z

I wonder if there is any specific reason to prefer one over the other.

I'm not sure since this PR started quite a while ago and these changes have carried over through all the rebases. I will remove them if they are unnecessary.

kleonc · 2023-01-17T16:23:24Z

I'm not sure since this PR started quite a while ago and these changes have carried over through all the rebases. I will remove them if they are unnecessary.

To me they do seem like not changing anything (so leaving the changes should be fine too).

kleonc

LGTM. Would welcome @clayjohn's opinion on #41239 (comment) before someone will merge this.

dalexeev · 2023-01-19T18:34:59Z

scene/main/canvas_item.cpp

+		// Top line.
 		RenderingServer::get_singleton()->canvas_item_add_line(
 				canvas_item,
-				p_rect.position + Size2(-offset, 0),
-				p_rect.position + Size2(p_rect.size.width + offset, 0),
+				rect.position + Size2(-offset, 0),
+				rect.position + Size2(-offset + rect.size.width, 0),
 				p_color,
 				p_width);
+		// Right line.
 		RenderingServer::get_singleton()->canvas_item_add_line(
 				canvas_item,
-				p_rect.position + Size2(p_rect.size.width, offset),
-				p_rect.position + Size2(p_rect.size.width, p_rect.size.height - offset),
+				rect.position + Size2(rect.size.width, -offset),
+				rect.position + Size2(rect.size.width, -offset + rect.size.height),
 				p_color,
 				p_width);
+		// Bottom line.
 		RenderingServer::get_singleton()->canvas_item_add_line(
 				canvas_item,
-				p_rect.position + Size2(p_rect.size.width + offset, p_rect.size.height),
-				p_rect.position + Size2(-offset, p_rect.size.height),
+				rect.position + Size2(offset + rect.size.width, rect.size.height),
+				rect.position + Size2(offset, rect.size.height),
 				p_color,
 				p_width);
+		// Left line.
 		RenderingServer::get_singleton()->canvas_item_add_line(
 				canvas_item,
-				p_rect.position + Size2(0, p_rect.size.height - offset),
-				p_rect.position + Size2(0, offset),
+				rect.position + Size2(0, offset + rect.size.height),
+				rect.position + Size2(0, offset),
 				p_color,
 				p_width);


Should draw_lines be replaced with canvas_item_add_polyline after #71679 is merged? Like here:

godot/scene/resources/rectangle_shape_2d.cpp

Lines 74 to 88 in 14fdd28

Vector<Vector2> stroke_points;

stroke_points.resize(5);

stroke_points.write[0] = -size * 0.5;

stroke_points.write[1] = Vector2(size.x, -size.y) * 0.5;

stroke_points.write[2] = size * 0.5;

stroke_points.write[3] = Vector2(-size.x, size.y) * 0.5;

stroke_points.write[4] = -size * 0.5;

Vector<Color> stroke_colors;

stroke_colors.resize(5);

for (int i = 0; i < 5; i++) {

stroke_colors.write[i] = (p_color);

}

RenderingServer::get_singleton()->canvas_item_add_polyline(p_to_rid, stroke_points, stroke_colors);

(draw_rect doesn't have antialiased parameter anyway.)

I guess so. A polyline should be slightly faster than having multiple regular lines. But that should be done in a later PR and needs be profiled before changing

clayjohn · 2023-01-19T19:38:47Z

LGTM. Would welcome @clayjohn's opinion on #41239 (comment) before someone will merge this.

I don't think there is a good reason to prefer one behaviour over the other.

akien-mga · 2023-01-19T21:49:30Z

Thanks!

pouleyKetchoupp added bug topic:rendering labels Aug 14, 2020

pouleyKetchoupp added this to the 4.0 milestone Aug 14, 2020

akien-mga requested review from reduz, clayjohn and lawnjelly August 14, 2020 09:53

dalexeev force-pushed the fix_draw_rect branch from 0d785cc to cd1b716 Compare February 28, 2021 12:31

dalexeev requested a review from a team as a code owner February 28, 2021 12:31

dalexeev force-pushed the fix_draw_rect branch from cd1b716 to db990c0 Compare March 29, 2021 12:52

aaronfranke reviewed Mar 29, 2021

View reviewed changes

scene/main/canvas_item.cpp Outdated Show resolved Hide resolved

dalexeev force-pushed the fix_draw_rect branch from db990c0 to 34df7b7 Compare March 29, 2021 19:44

dalexeev force-pushed the fix_draw_rect branch from 34df7b7 to f5bbffa Compare August 21, 2021 20:53

dalexeev requested review from aaronfranke and removed request for a team August 22, 2021 06:01

aaronfranke reviewed Aug 22, 2021

View reviewed changes

scene/main/canvas_item.cpp Outdated Show resolved Hide resolved

scene/main/canvas_item.cpp Outdated Show resolved Hide resolved

scene/main/canvas_item.cpp Outdated Show resolved Hide resolved

scene/main/canvas_item.cpp Outdated Show resolved Hide resolved

dalexeev force-pushed the fix_draw_rect branch from f5bbffa to 969ee68 Compare August 22, 2021 07:37

dalexeev commented Aug 22, 2021

View reviewed changes

scene/main/canvas_item.cpp Outdated Show resolved Hide resolved

dalexeev mentioned this pull request Aug 22, 2021

1 pixel wide lines remain thin when CanvasItem is scaled #51978

Closed

YuriSizov requested review from a team August 24, 2021 23:29

YuriSizov added the topic:2d label Aug 24, 2021

dalexeev force-pushed the fix_draw_rect branch from 969ee68 to 062dcce Compare October 4, 2022 17:17

dalexeev mentioned this pull request Oct 21, 2022

CollisionShape2D debug drawing exhibits unstable graphical glitches #56320

Open

akien-mga reopened this Jan 16, 2023

akien-mga removed the archived label Jan 16, 2023

dalexeev force-pushed the fix_draw_rect branch from 2074af3 to ef23ee2 Compare January 16, 2023 13:17

dalexeev requested review from kleonc and removed request for reduz, clayjohn and lawnjelly January 16, 2023 13:27

kleonc reviewed Jan 16, 2023

View reviewed changes

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

Fix CanvasItem.draw_rect function with filled = false

c046bb8

dalexeev force-pushed the fix_draw_rect branch from ef23ee2 to c046bb8 Compare January 17, 2023 06:15

kleonc approved these changes Jan 17, 2023

View reviewed changes

kleonc requested a review from clayjohn January 17, 2023 16:24

dalexeev commented Jan 19, 2023

View reviewed changes

clayjohn approved these changes Jan 19, 2023

View reviewed changes

akien-mga merged commit 360b610 into godotengine:master Jan 19, 2023

dalexeev deleted the fix_draw_rect branch January 20, 2023 03:46

dalexeev mentioned this pull request Jan 31, 2023

Fix polyline stroke #63425

Closed

dalexeev mentioned this pull request Feb 8, 2023

Optimize draw_dashed_line() and draw_rect() #72880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `CanvasItem.draw_rect` function with `filled = false` #41239

Fix `CanvasItem.draw_rect` function with `filled = false` #41239

dalexeev commented Aug 14, 2020 •

edited

Loading

aaronfranke commented Feb 27, 2021

dalexeev commented Mar 29, 2021

aaronfranke left a comment

dalexeev commented Aug 22, 2021 •

edited

Loading

dalexeev commented Aug 22, 2021

dalexeev commented Oct 4, 2022

MewPurPur commented Dec 1, 2022

akien-mga commented Jan 16, 2023

dalexeev commented Jan 16, 2023

dalexeev commented Jan 16, 2023

kleonc commented Jan 16, 2023

kleonc left a comment

dalexeev commented Jan 17, 2023

kleonc commented Jan 17, 2023

dalexeev commented Jan 17, 2023

kleonc commented Jan 17, 2023

kleonc left a comment

dalexeev Jan 19, 2023 •

edited

Loading

clayjohn Jan 19, 2023 •

edited

Loading

clayjohn commented Jan 19, 2023

akien-mga commented Jan 19, 2023

	Vector<Vector2> stroke_points;
	stroke_points.resize(5);
	stroke_points.write[0] = -size * 0.5;
	stroke_points.write[1] = Vector2(size.x, -size.y) * 0.5;
	stroke_points.write[2] = size * 0.5;
	stroke_points.write[3] = Vector2(-size.x, size.y) * 0.5;
	stroke_points.write[4] = -size * 0.5;

	Vector<Color> stroke_colors;
	stroke_colors.resize(5);
	for (int i = 0; i < 5; i++) {
	stroke_colors.write[i] = (p_color);
	}

	RenderingServer::get_singleton()->canvas_item_add_polyline(p_to_rid, stroke_points, stroke_colors);

Fix CanvasItem.draw_rect function with filled = false #41239

Fix CanvasItem.draw_rect function with filled = false #41239

Conversation

dalexeev commented Aug 14, 2020 • edited Loading

aaronfranke commented Feb 27, 2021

dalexeev commented Mar 29, 2021

aaronfranke left a comment

Choose a reason for hiding this comment

dalexeev commented Aug 22, 2021 • edited Loading

dalexeev commented Aug 22, 2021

dalexeev commented Oct 4, 2022

MewPurPur commented Dec 1, 2022

akien-mga commented Jan 16, 2023

dalexeev commented Jan 16, 2023

dalexeev commented Jan 16, 2023

kleonc commented Jan 16, 2023

kleonc left a comment

Choose a reason for hiding this comment

dalexeev commented Jan 17, 2023

kleonc commented Jan 17, 2023

dalexeev commented Jan 17, 2023

kleonc commented Jan 17, 2023

kleonc left a comment

Choose a reason for hiding this comment

dalexeev Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

clayjohn Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

clayjohn commented Jan 19, 2023

akien-mga commented Jan 19, 2023

Fix `CanvasItem.draw_rect` function with `filled = false` #41239

Fix `CanvasItem.draw_rect` function with `filled = false` #41239

dalexeev commented Aug 14, 2020 •

edited

Loading

dalexeev commented Aug 22, 2021 •

edited

Loading

dalexeev Jan 19, 2023 •

edited

Loading

clayjohn Jan 19, 2023 •

edited

Loading