blender
/
blender
136
407
Fork 591
Code Issues 6.3k Pull Requests 777 Projects 19 Wiki Activity

#70267 Retopology Overlay #104599

Merged
Clément Foucault merged 30 commits from bonj/blender:retopology-overlay into main 2023-03-03 00:35:56 +01:00
Conversation 39 Commits 30 Files Changed 25 +9 -8
5 changed files with 9 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d08be75414 - Show all commits

7
source/blender/draw/engines/overlay/overlay_edit_mesh.cc
View File

@ -63,11 +63,12 @@ void OVERLAY_edit_mesh_cache_init(OVERLAY_Data *vedata)
RegionView3D *rv3d = draw_ctx->rv3d;
bool show_retopology = (v3d->overlay.edit_flag & V3D_OVERLAY_EDIT_RETOPOLOGY) != 0;
/* Add epsilon to ensure the value is never zero when the effect is enabled. */
float retopology_bias = (show_retopology) ?
bonj marked this conversation as resolved
Jeroen Bakker commented 2023-02-28 04:00:22 +01:00
Review
Copy Link

Just a small tweak.
Use max_ff(v3d->overlay.retopology_offset, FLT_EPSILON) and remove comment as this is then cleaner in the code.

Just a small tweak. Use `max_ff(v3d->overlay.retopology_offset, FLT_EPSILON)` and remove comment as this is then cleaner in the code.
Jorijn de Graaf commented 2023-02-28 04:22:46 +01:00
Review
Copy Link

Makes sense, I'll do that.

Makes sense, I'll do that.
((rv3d->is_persp) ?
(v3d->overlay.retopology_bias * v3d->clip_start) :
(v3d->overlay.retopology_bias / v3d->clip_end)) :
-1.0f; /* Negative value disables the effect. */
(v3d->overlay.retopology_bias * v3d->clip_start + FLT_EPSILON) :
(v3d->overlay.retopology_bias / v3d->clip_end + FLT_EPSILON)) :
0.0f;
pd->edit_mesh.do_faces = true;
pd->edit_mesh.do_edges = true;

2
source/blender/draw/engines/overlay/shaders/overlay_depth_only_vert.glsl
View File

@ -10,7 +10,7 @@ void main()
gl_Position = point_world_to_ndc(world_pos);
/* Offset Z position for retopology overlay. */
bonj marked this conversation as resolved
Clément Foucault commented 2023-02-14 02:14:36 +01:00
Review
Copy Link

You edit a file that is used in other part of the engine. Maybe it is better to duplicate the shader as overlay_edit_mesh_depth_vert.glsl.

You edit a file that is used in other part of the engine. Maybe it is better to duplicate the shader as `overlay_edit_mesh_depth_vert.glsl`.
Jorijn de Graaf commented 2023-02-14 14:00:14 +01:00
Review
Copy Link

Previously it was like this: overlay_uniform_color and overlay_depth_only use overlay_depth_only_vert.glsl
Now it is like this: overlay_uniform_color uses overlay_uniform_color_vert.glsl, and overlay_depth_only uses overlay_depth_only_vert.glsl
What you're proposing is like this: overlay_uniform_color uses overlay_depth_only_vert.glsl, and overlay_depth_only uses overlay_edit_mesh_depth_vert.glsl
These glsl files are only used by these shaders, not anywhere else.
I think it makes a lot more sense to name the files after the shaders that use them.

Previously it was like this: overlay_uniform_color and overlay_depth_only use overlay_depth_only_vert.glsl Now it is like this: overlay_uniform_color uses overlay_uniform_color_vert.glsl, and overlay_depth_only uses overlay_depth_only_vert.glsl What you're proposing is like this: overlay_uniform_color uses overlay_depth_only_vert.glsl, and overlay_depth_only uses overlay_edit_mesh_depth_vert.glsl These glsl files are only used by these shaders, not anywhere else. I think it makes a lot more sense to name the files after the shaders that use them.
Clément Foucault commented 2023-02-14 19:21:28 +01:00
Review
Copy Link

Yes, but I'm suggesting to rename the overlay_depth_only to overlay_edit_mesh_depth_only too.

Changing the uniform color shader is a cleanup think and should be committed separately.

Yes, but I'm suggesting to rename the `overlay_depth_only` to `overlay_edit_mesh_depth_only` too. Changing the uniform color shader is a cleanup think and should be committed separately.
Jorijn de Graaf commented 2023-02-14 19:59:59 +01:00
Review
Copy Link

Oh you mean renaming the shader itself, as well as its vertex and fragment shaders, to overlay_edit_mesh_depth_only(_vert/frag.glsl)?
And then reverting overlay_uniform_color to how it was, letting someone else deal with it another time?
I suppose I can do that yeah.
There was one change I made that I still think we shouldn't revert, which is that the clipping version of overlay_uniform_color inherited from the depth only shader, which I'm fairly certain must've been a mistake, because all other clipping variants of shaders do inherit how they should.

Oh you mean renaming the shader itself, as well as its vertex and fragment shaders, to overlay_edit_mesh_depth_only(\_vert/frag.glsl)? And then reverting overlay_uniform_color to how it was, letting someone else deal with it another time? I suppose I can do that yeah. There was one change I made that I still think we shouldn't revert, which is that the clipping version of overlay_uniform_color inherited from the depth only shader, which I'm fairly certain must've been a mistake, because all other clipping variants of shaders do inherit how they should.
Clément Foucault commented 2023-02-14 20:45:01 +01:00
Review
Copy Link

This should also be in another commit. You can create another PR for these two thing (only one PR for both).

This should also be in another commit. You can create another PR for these two thing (only one PR for both).
Jorijn de Graaf commented 2023-02-14 23:00:05 +01:00
Review
Copy Link

After thinking about it for a long time, I understand now.
The depth_only shader is used in several places I was unaware of, so I will revert it to its original state, and do the same for uniform_color.
I will then add a new shader specifically for the depth pass in mesh edit mode, which will get its own vertex shader (it can reuse the depth_only fragment shader though).

After thinking about it for a long time, I understand now. The depth_only shader is used in several places I was unaware of, so I will revert it to its original state, and do the same for uniform_color. I will then add a new shader specifically for the depth pass in mesh edit mode, which will get its own vertex shader (it can reuse the depth_only fragment shader though).
Jorijn de Graaf commented 2023-02-14 23:25:35 +01:00
Review
Copy Link

Alright I think this should be resolved now, so I'll mark it as such.

Alright I think this should be resolved now, so I'll mark it as such.
gl_Position.z -= max(0.0, retopologyBias) / abs(gl_Position.w);
gl_Position.z -= retopologyBias / abs(gl_Position.w);
bonj marked this conversation as resolved
Clément Foucault commented 2023-02-14 02:11:05 +01:00
Review
Copy Link

Move the max out of the shader. That stands for all shaders.

Move the max out of the shader. That stands for all shaders.
Jorijn de Graaf commented 2023-02-14 02:53:57 +01:00
Review
Copy Link

This is necessary because the value is -1.0 when the checkbox is disabled.
That is how the shader knows whether to use the regular face color or the retopology face color.
Alternatively I could add an extra shader constant, but I was told to get rid of that, hence the current solution.
I guess a third solution would be to make the minimum value something like 0.001, then check for 0.0 to determine face color; I would not prefer this approach though.

This is necessary because the value is -1.0 when the checkbox is disabled. That is how the shader knows whether to use the regular face color or the retopology face color. Alternatively I could add an extra shader constant, but I was told to get rid of that, hence the current solution. I guess a third solution would be to make the minimum value something like 0.001, then check for 0.0 to determine face color; I would not prefer this approach though.
Clément Foucault commented 2023-02-14 13:05:19 +01:00
Review
Copy Link

minimum value something like 0.001

You can use FLT_MIN for that. I'm sure the difference with 0.0 will not be noticeable.

> minimum value something like 0.001 You can use `FLT_MIN` for that. I'm sure the difference with 0.0 will not be noticeable.
Jorijn de Graaf commented 2023-02-14 13:43:03 +01:00
Review
Copy Link

That's a very large negative value.
I could use FLT_EPSILON though, perhaps not as minimum for the property, but that I add it before sending the value to the shader, to make sure it's never 0 when the effect is enabled.
Edit: Did it, I think it's a good solution.

That's a very large negative value. I could use FLT_EPSILON though, perhaps not as minimum for the property, but that I add it before sending the value to the shader, to make sure it's never 0 when the effect is enabled. Edit: Did it, I think it's a good solution.
view_clipping_distances(world_pos);
}

2
source/blender/draw/engines/overlay/shaders/overlay_edit_mesh_common_lib.glsl
View File

@ -54,7 +54,7 @@ vec4 EDIT_MESH_face_color(uint face_flag)
{
vec4 color = colorFace;
vec4 color_active = mix(colorFaceSelect, colorEditMeshActive, 0.5);
color = (retopologyBias != -1.0) ? colorFaceRetopology : color;
color = (retopologyBias > 0.0) ? colorFaceRetopology : color;
color = ((face_flag & FACE_FREESTYLE) != 0u) ? colorFaceFreestyle : color;
color = ((face_flag & FACE_SELECTED) != 0u) ? colorFaceSelect : color;
color = ((face_flag & FACE_ACTIVE) != 0u) ? color_active : color;

2
source/blender/draw/engines/overlay/shaders/overlay_edit_mesh_vert.glsl
View File

@ -96,7 +96,7 @@ void main()
#endif
/* Offset Z position for retopology overlay. */
gl_Position.z -= max(0.0, retopologyBias) / abs(gl_Position.w);
gl_Position.z -= retopologyBias / abs(gl_Position.w);
bonj marked this conversation as resolved
Clément Foucault commented 2023-02-14 02:20:36 +01:00
Review
Copy Link

I am pretty sure the bias is incorrect.

I think it should be somethign like gl_Position.z -= retopologyBias * abs(gl_Position.w);.

Here your bias decrease with the distance (or increases the closer you are) which is quite weird.
I will have to double check that.

I am pretty sure the bias is incorrect. I think it should be somethign like `gl_Position.z -= retopologyBias * abs(gl_Position.w);`. Here your bias decrease with the distance (or increases the closer you are) which is quite weird. I will have to double check that.
Jorijn de Graaf commented 2023-02-14 14:17:00 +01:00
Review
Copy Link

It's counter-intuitive, but dividing seems to be the right approach here.
If we multiply by W, the effective offset changes in an undesirable way when zooming in or out.
You need very different bias values to get the same result at different view distances.
When not using W at all, it's not as bad, but still has issues compared to dividing by W.
I don't really know how to describe it, so you might have to test for yourself.

It's counter-intuitive, but dividing seems to be the right approach here. If we multiply by W, the effective offset changes in an undesirable way when zooming in or out. You need very different bias values to get the same result at different view distances. When not using W at all, it's not as bad, but still has issues compared to dividing by W. I don't really know how to describe it, so you might have to test for yourself.
Jorijn de Graaf commented 2023-02-15 23:38:25 +01:00
Review
Copy Link

Perhaps it's important to note how the bias is calculated.

float retopology_bias = (show_retopology) ?
                            ((rv3d->is_persp) ?
                                 (v3d->overlay.retopology_bias * v3d->clip_start + FLT_EPSILON) :
                                 (v3d->overlay.retopology_bias / v3d->clip_end + FLT_EPSILON)) :
                            0.0f;
Perhaps it's important to note how the bias is calculated. ```cpp float retopology_bias = (show_retopology) ? ((rv3d->is_persp) ? (v3d->overlay.retopology_bias * v3d->clip_start + FLT_EPSILON) : (v3d->overlay.retopology_bias / v3d->clip_end + FLT_EPSILON)) : 0.0f; ```
Jorijn de Graaf commented 2023-02-22 00:27:42 +01:00
Review
Copy Link

I have not been able to make it look correct with multiplying by W instead of dividing.
Since it works correctly as is, I'm going to consider this resolved.

I have not been able to make it look correct with multiplying by W instead of dividing. Since it works correctly as is, I'm going to consider this resolved.
view_clipping_distances(world_pos);
}

4
source/blender/draw/engines/overlay/shaders/overlay_edit_mesh_vert_no_geom.glsl
View File

@ -144,8 +144,8 @@ void main()
#endif
/* Offset Z position for retopology overlay. */
out_pos0.z -= max(0.0, retopologyBias) / abs(out_pos0.w);
out_pos1.z -= max(0.0, retopologyBias) / abs(out_pos1.w);
out_pos0.z -= retopologyBias / abs(out_pos0.w);
out_pos1.z -= retopologyBias / abs(out_pos1.w);
// -------- GEOM SHADER ALTERNATIVE ----------- //
vec2 ss_pos[2];