Add anti aliasing to transform constraints #107394

Yonatan-Maor · 2023-04-27T08:18:46+02:00

Yonatan-Maor commented

2023-04-27 08:18:46 +02:00

Transform constraints were being drawn on screen without anti-aliasing, now fixed to make them look smoother.
See #107038

Adding a comparison of before and after the change

Transform constraints were being drawn on screen without anti-aliasing, now fixed to make them look smoother. See #107038 Adding a comparison of before and after the change

Before.png

62 KiB

After.png

87 KiB

👍 4 👀 1 ❤️ 3 🎉 2

Yonatan-Maor requested review from Pablo Vazquez 2023-04-27 08:25:39 +02:00

Yonatan-Maor changed title from ~~fix_107038_add_anti_aliasing_to_transform_constraints~~ to Fix #107038: Add anti aliasing to transform constraints

2023-04-27 11:10:43 +02:00

Iliya Katushenock added this to the EEVEE & Viewport project 2023-04-27 16:28:52 +02:00

Iliya Katushenock changed title from ~~Fix #107038: Add anti aliasing to transform constraints~~ to Add anti aliasing to transform constraints

2023-04-27 16:29:19 +02:00

Iliya Katushenock commented

2023-04-27 16:31:04 +02:00

Hi, #107038 isn't a bug, but todo design. So name for this PR shouldn't contain Fix... prefix, this just new feature.

Hi, #107038 isn't a bug, but todo design. So name for this PR shouldn't contain `Fix...` prefix, this just new feature.

Yonatan-Maor commented

2023-04-27 16:41:15 +02:00

Thanks! I thought this was considered a bug, which is why I added Fix to the title.
If this is not a bug should I add a WIP prefix to the title?

Thanks! I thought this was considered a bug, which is why I added Fix to the title. If this is not a bug should I add a WIP prefix to the title?

Iliya Katushenock commented

2023-04-27 16:49:30 +02:00

WIP would say that this is not ready for review and approving and is still being developed. Some about this: https://wiki.blender.org/wiki/Style_Guide/Commit_Messages

Yonatan-Maor commented

2023-04-27 16:55:25 +02:00

WIP would say that this is not ready for review and approving and is still being developed. Some about this: https://wiki.blender.org/wiki/Style_Guide/Commit_Messages

Cool, I'll take a look

> WIP would say that this is not ready for review and approving and is still being developed. Some about this: https://wiki.blender.org/wiki/Style_Guide/Commit_Messages Cool, I'll take a look

Harley Acheson commented

2023-04-27 18:59:14 +02:00

Works well, code looks correct.

Yonatan-Maor commented

2023-04-27 19:30:12 +02:00

Thanks!
I'm kind of new here, who should be authorizing the pull request?

Thanks! I'm kind of new here, who should be authorizing the pull request?

Harley Acheson requested review from Clément Foucault 2023-04-27 19:35:09 +02:00

Harley Acheson commented

2023-04-27 19:40:54 +02:00

I'm kind of new here, who should be authorizing the pull request?

The two reviewers you now have are fine I think, with Clément being the blocking reviewer who will probably have the last word and approval, and would be the one to commit. This process could take as little as a few days to weeks, and you should make another comment in this thread if nothing happens for a while.

> I'm kind of new here, who should be authorizing the pull request? The two reviewers you now have are fine I think, with Clément being the blocking reviewer who will probably have the last word and approval, and would be the one to commit. This process could take as little as a few days to weeks, and you should make another comment in this thread if nothing happens for a while.

Clément Foucault requested changes 2023-04-27 20:19:19 +02:00

Clément Foucault left a comment

Thanks for looking into it! However, this isn't a portable fix. It will not work on MacOS / Metal backend.

The portable fix is to replace the shader program by GPU_SHADER_3D_POLYLINE_UNIFORM_COLOR. See source/blender/editors/gizmo_library/gizmo_types/button2d_gizmo.c line 86 for an example of the uniforms to give to the shader.

Note that GPU_SHADER_3D_POLYLINE_UNIFORM_COLOR also requires GPU_BLEND_ALPHA for correct anti-aliasing.

Thanks for looking into it! However, this isn't a portable fix. It will not work on MacOS / Metal backend. The portable fix is to replace the shader program by `GPU_SHADER_3D_POLYLINE_UNIFORM_COLOR`. See `source/blender/editors/gizmo_library/gizmo_types/button2d_gizmo.c` line 86 for an example of the uniforms to give to the shader. Note that `GPU_SHADER_3D_POLYLINE_UNIFORM_COLOR` also requires `GPU_BLEND_ALPHA` for correct anti-aliasing.

Yonatan-Maor commented

2023-04-27 20:59:01 +02:00

Not sure that I'm well acquainted enough with the code to do this, but I'll take a look.
If you have any time, could you explain why this wouldn't work on other platforms? I simply took the code that is used to draw the global axes (I'm guessing that this code works on other platforms)

Not sure that I'm well acquainted enough with the code to do this, but I'll take a look. If you have any time, could you explain why this wouldn't work on other platforms? I simply took the code that is used to draw the global axes (I'm guessing that this code works on other platforms)

Clément Foucault commented

2023-04-27 21:51:29 +02:00

could you explain why this wouldn't work on other platforms

The function call GPU_line_smooth(true); enables the deprecated OpenGL feature GL_LINE_SMOOTH that isn't available on Metal. We have a software workaround using a special shader (as mentionned) but it needs a bit more work. And, yes, anything using GPU_line_smooth(true); is currently not anti-aliased on Apple hardware.

> could you explain why this wouldn't work on other platforms The function call `GPU_line_smooth(true);` enables the deprecated OpenGL feature `GL_LINE_SMOOTH` that isn't available on Metal. We have a software workaround using a special shader (as mentionned) but it needs a bit more work. And, yes, anything using `GPU_line_smooth(true);` is currently not anti-aliased on Apple hardware.

Harley Acheson commented

2023-04-27 22:05:20 +02:00

Not sure that I'm well acquainted enough with the code to do this, but I'll take a look.

You can do it. If you look at the section of code that Clément referenced you will see there isn't a big difference, just a change of shader that needs a bit more information. It probably would look something like:

diff --git a/source/blender/editors/transform/transform_constraints.c b/source/blender/editors/transform/transform_constraints.c
index 9b92a37abdf..9a2f0c002f8 100644
--- a/source/blender/editors/transform/transform_constraints.c
+++ b/source/blender/editors/transform/transform_constraints.c
@@ -776,11 +776,18 @@ static void drawLine(
   }
   UI_make_axis_color(col, col2, axis);
 
   uint pos = GPU_vertformat_attr_add(immVertexFormat(), "pos", GPU_COMP_F32, 3, GPU_FETCH_FLOAT);
 
-  immBindBuiltinProgram(GPU_SHADER_3D_UNIFORM_COLOR);
+  float viewport[4];
+  GPU_viewport_size_get_f(viewport);
+  GPU_blend(GPU_BLEND_ALPHA);
+
+  immBindBuiltinProgram(GPU_SHADER_3D_POLYLINE_UNIFORM_COLOR);
+  immUniform2fv("viewportSize", &viewport[2]);
+  immUniform1f("lineWidth", U.pixelsize);
+
   immUniformColor3ubv(col2);
 
   immBegin(GPU_PRIM_LINES, 2);
   immVertex3fv(pos, v1);
   immVertex3fv(pos, v2);

> Not sure that I'm well acquainted enough with the code to do this, but I'll take a look. You can do it. If you look at the section of code that Clément referenced you will see there isn't a big difference, just a change of shader that needs a bit more information. It probably would look _something_ like: ```Diff diff --git a/source/blender/editors/transform/transform_constraints.c b/source/blender/editors/transform/transform_constraints.c index 9b92a37abdf..9a2f0c002f8 100644 --- a/source/blender/editors/transform/transform_constraints.c +++ b/source/blender/editors/transform/transform_constraints.c @@ -776,11 +776,18 @@ static void drawLine( } UI_make_axis_color(col, col2, axis); uint pos = GPU_vertformat_attr_add(immVertexFormat(), "pos", GPU_COMP_F32, 3, GPU_FETCH_FLOAT); - immBindBuiltinProgram(GPU_SHADER_3D_UNIFORM_COLOR); + float viewport[4]; + GPU_viewport_size_get_f(viewport); + GPU_blend(GPU_BLEND_ALPHA); + + immBindBuiltinProgram(GPU_SHADER_3D_POLYLINE_UNIFORM_COLOR); + immUniform2fv("viewportSize", &viewport[2]); + immUniform1f("lineWidth", U.pixelsize); + immUniformColor3ubv(col2); immBegin(GPU_PRIM_LINES, 2); immVertex3fv(pos, v1); immVertex3fv(pos, v2); ```

Harley Acheson changed title from ~~Add anti aliasing to transform constraints~~ to WIP: Add anti aliasing to transform constraints

2023-04-27 22:05:42 +02:00

Yonatan-Maor commented

2023-04-27 23:27:19 +02:00

I think that I'm getting the expected result using this code snippet (thanks @Harley )
Thing is, the lines are extremely thin, which makes them look aliased again (I tried increasing the linewidth to make sure my results look ok).

Is the required line width documented anywhere?

I think that I'm getting the expected result using this code snippet (thanks @Harley ) Thing is, the lines are extremely thin, which makes them look aliased again (I tried increasing the linewidth to make sure my results look ok). Is the required line width documented anywhere?

Harley Acheson commented

2023-04-28 00:10:39 +02:00

U.pixelsize should be consider our minimum feature size, so dots or lines shouldn't get smaller than that.

For most users you can just go to Preferences / Interface and set "Resolution Scale" to 1.0 and "Line Width" to "Default" and you should see most lines 1 pixel wide and U.pixelsize will be 1. Change to "Thick" lines and that value will be 2. A Mac user with a Retina (high DPI) display will get a U.pixelsize of 2 when scale is 1 and using default line width.

Although U.pixelsize is a float it is actually rounded to nearest int. That shader's "lineWidth" can accept any float value so you can really just eyeball it and set that to any multiplier against it that looks good to you, like U.pixelsize * 1.12f or whatever. I'd probably make it have the same fullness as the grid axes lines, but pick something you think looks good, just make sure to test at various sizes. If you pick badly we'll just let you know during review.

U.pixelsize should be consider our minimum feature size, so dots or lines shouldn't get smaller than that. For most users you can just go to Preferences / Interface and set "Resolution Scale" to 1.0 and "Line Width" to "Default" and you should see most lines 1 pixel wide and U.pixelsize will be 1. Change to "Thick" lines and that value will be 2. A Mac user with a Retina (high DPI) display will get a U.pixelsize of 2 when scale is 1 and using default line width. Although U.pixelsize is a float it is actually rounded to nearest int. That shader's "lineWidth" can accept any float value so you can really just eyeball it and set that to any multiplier against it that looks good to you, like `U.pixelsize * 1.12f` or whatever. I'd probably make it have the same fullness as the grid axes lines, but pick something you think looks good, just make sure to test at various sizes. If you pick badly we'll just let you know during review.

Yonatan-Maor commented

2023-04-28 05:54:50 +02:00

Made the requested changes.
Adding new before/after images:

Made the requested changes. Adding new before/after images:

After.png

80 KiB

Before.png

62 KiB

Yonatan-Maor requested review from Clément Foucault 2023-04-28 05:54:56 +02:00

Clément Foucault requested changes 2023-04-28 09:44:14 +02:00

Clément Foucault left a comment

This looks good! Just need to cleanup the unused line and it's good to be merged.

source/blender/editors/transform/transform_constraints.c Outdated

						
				@ -779,3 +779,3 @@

				  uint pos = GPU_vertformat_attr_add(immVertexFormat(), "pos", GPU_COMP_F32, 3, GPU_FETCH_FLOAT);

				  immBindBuiltinProgram(GPU_SHADER_3D_UNIFORM_COLOR);

				  // immBindBuiltinProgram(GPU_SHADER_3D_UNIFORM_COLOR);