Add a way to draw 1-pixel wide lines that scale with viewport zoom

[m6502]

Describe the project you are working on:

I'm coding some tools we need for our current project and at one point I needed to draw a rectangle enclosing an area of a sprite.

Describe the problem or limitation you are having in your project:

I set the line width to 1 expecting a thickness of one pixel of the sprite. Instead, the line is literally of 1 pixel width, giving an unexpected result as it wasn't taking into account the amount of zoom the viewport had. Further experimentation showed the problem happened while in game too, not only in the editor viewport.

I opened an issue but it looks like the bug was already reported (but I missed it when I searched before opening a new one):

Mine: godotengine/godot#41176
Older: godotengine/godot#19844

As the bug was already reported I closed my issue, but as suggested by @pouleyKetchoupp I'm bringing this to the Godot proposals. In my report there are screenshots showing both the bug and the expected result, the first patch I did at the code of Godot and a small Godot project showing the problem (shown in the second set of before-after images).

The issue is present both in the 3.x and 4.x branches, and affects Vulkan, GLES2 and GLES3. It is caused by a optimization where the line draw function checks if the width is <= 1 so it can use a simpler approach for speed reasons. As it can be seen in my issue I tried to patch the check and recompile Godot but I was a bit clumsy with the pull requests :-) Also I didn't take into account Vulkan (I'm currently at version 3.2.2).

This problem happens when using draw_rect(), etc, as in the end it's the line draw function that is called.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:

Reading the old issue has been interesting because we all arrived at similar conclusions. Probably not really a good idea to fix for 3.x at this point? But there are use cases both for the correct width and the fixed 1 pixel width, so for the sake of correctness it would be nice to make this work.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

I propose to change the size check for a constant with a negative value, instead of less or equal than 1. A value of 0 should give a non-visible line. A value of 0.5 should give a line that is half the width of a line 1 pixel thick if the zoom level allows for it. Or have a new draw line function that only draws lines of 1 pixel screen space pixels width.

I'm quite happy with Godot and plan to contribute when I can, so I would be happy to make a new pull request after we agree on the best way to fix the issue.

If this enhancement will not be used often, can it be worked around with a few lines of script?:

It can't be worked around.

Is there a reason why this should be core and not an add-on in the asset library?:

It's a change on "core" code.

[pouleyKetchoupp]

Here's the code that handles line width in 4.0 for reference:
https://github.com/godotengine/godot/blob/d3b5c0948c943b3fbcb4b5322262c59c92abfa83/servers/rendering/rendering_server_canvas.cpp#L465-L487

void RenderingServerCanvas::canvas_item_add_line(RID p_item, const Point2 &p_from, const Point2 &p_to, const Color &p_color, float p_width) { 
 	Item *canvas_item = canvas_item_owner.getornull(p_item); 
 	ERR_FAIL_COND(!canvas_item); 
  
 	Item::CommandPrimitive *line = canvas_item->alloc_command<Item::CommandPrimitive>(); 
 	ERR_FAIL_COND(!line); 
 	if (p_width > 1.001) { 
 		Vector2 t = (p_from - p_to).tangent().normalized(); 
 		line->points[0] = p_from + t * p_width; 
 		line->points[1] = p_from - t * p_width; 
 		line->points[2] = p_to - t * p_width; 
 		line->points[3] = p_to + t * p_width; 
 		line->point_count = 4; 
 	} else { 
 		line->point_count = 2; 
 		line->points[0] = p_from; 
 		line->points[1] = p_to; 
 	} 
 	for (uint32_t i = 0; i < line->point_count; i++) { 
 		line->colors[i] = p_color; 
 	} 
 	line->specular_shininess = Color(1, 1, 1, 1); 
 } 

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

[m6502]

I agree :-)