#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
Contributor

Summary

Add overlay option for retopology, which hides the shaded mesh akin to Hidden Wire, and offsets the edit mesh overlay towards the view.

Problem

When doing retopology, you want to be able to see both the reference mesh, and the edit mesh, at the same time.
With the edit mesh being snapped to the surface of the reference mesh, the edit mesh is often hard to see.
In X-Ray mode, you are distracted by seeing the edit mesh through itself, or through thick sections of the reference mesh.

Solution

Hide the shaded mesh, as it only gets in the way (and would require adding Z bias to a lot of different shaders).
Then offset the edit mesh on the Z axis a little to show it in front of nearby reference geometry.

Alternatives

Display in front always shows the edit mesh in front of the reference mesh, but this includes back faces, and far away faces.
X-Ray mode allows you to see both edit and reference mesh, but has the same issue as in front, with even more clutter.
It can work decently well to use a shrinkwrap modifier with an offset, but that effects the final geometry, which may make it awkward to edit.
Shrinkwrap also doesn't allow you to see the reference mesh behind it, unless you set up your object display or material settings in a specific way.

Limitations

If your reference mesh has great disparity in thickness, it may be impossible to choose the perfect offset for all areas.
Faces look better viewed head on than from an angle. A bigger offset helps, but can lead to undesired results.

Interface

The Retopology Overlay option is enabled via a checkbox that replaces Hidden Wire.
The slider adjusts the distance that the edit mesh is offset towards the view.
These options are found in the 3D View Overlays popover, and only visible in Mesh Edit mode.

Notes

I previously submitted this patch here: https://archive.blender.org/developer/D17148
Some other related history in this PR: #104921

#### Summary Add overlay option for retopology, which hides the shaded mesh akin to Hidden Wire, and offsets the edit mesh overlay towards the view. #### Problem When doing retopology, you want to be able to see both the reference mesh, and the edit mesh, at the same time. With the edit mesh being snapped to the surface of the reference mesh, the edit mesh is often hard to see. In X-Ray mode, you are distracted by seeing the edit mesh through itself, or through thick sections of the reference mesh. #### Solution Hide the shaded mesh, as it only gets in the way (and would require adding Z bias to a lot of different shaders). Then offset the edit mesh on the Z axis a little to show it in front of nearby reference geometry. #### Alternatives Display in front always shows the edit mesh in front of the reference mesh, but this includes back faces, and far away faces. X-Ray mode allows you to see both edit and reference mesh, but has the same issue as in front, with even more clutter. It can work decently well to use a shrinkwrap modifier with an offset, but that effects the final geometry, which may make it awkward to edit. Shrinkwrap also doesn't allow you to see the reference mesh behind it, unless you set up your object display or material settings in a specific way. #### Limitations If your reference mesh has great disparity in thickness, it may be impossible to choose the perfect offset for all areas. Faces look better viewed head on than from an angle. A bigger offset helps, but can lead to undesired results. #### Interface The Retopology Overlay option is enabled via a checkbox that replaces Hidden Wire. The slider adjusts the distance that the edit mesh is offset towards the view. These options are found in the 3D View Overlays popover, and only visible in Mesh Edit mode. #### Notes I previously submitted this patch here: https://archive.blender.org/developer/D17148 Some other related history in this PR: #104921
Jorijn de Graaf requested review from Jeroen Bakker 2023-02-11 00:26:51 +01:00
Jorijn de Graaf requested review from Clément Foucault 2023-02-11 00:27:01 +01:00
Jorijn de Graaf requested review from Julien Kaspar 2023-02-11 00:27:13 +01:00
Jorijn de Graaf changed title from WIP: Retopology Overlay to Retopology Overlay 2023-02-11 15:14:03 +01:00
Jorijn de Graaf changed title from Retopology Overlay to WIP: Retopology Overlay 2023-02-11 15:24:34 +01:00
Author
Contributor

It works in orthographic view now, so I think this feature is pretty much complete.

The only remaining tasks are:

  • Testing with Metal.
  • Replacing Hidden Wire with Retopology in locale files.

As I don't own an Apple computer, and don't speak many languages, I don't think I can do either of those things myself.

It works in orthographic view now, so I think this feature is pretty much complete. The only remaining tasks are: * Testing with Metal. * Replacing Hidden Wire with Retopology in locale files. As I don't own an Apple computer, and don't speak many languages, I don't think I can do either of those things myself.
Member

Works great! I just have a one suggestion left.

IMO the Z-bias slider shouldn't be able to go all the way to 0 and turn the overlay off. Only the toggle should be able to disable the overlay completely. When sliding the z-bias to the lowest value, it should instead only disable the Z bias but keep the mesh overlay intact (Kinda like the old 'Hidden Wire' overlay).


Also I'll add the original to do task to the title as a reminder to close the issue once this is committed ;)

Works great! I just have a one suggestion left. IMO the Z-bias slider shouldn't be able to go all the way to 0 and turn the overlay off. Only the toggle should be able to disable the overlay completely. When sliding the z-bias to the lowest value, it should instead only disable the Z bias but keep the mesh overlay intact (Kinda like the old 'Hidden Wire' overlay). ---- Also I'll add the original to do task to the title as a reminder to close the issue once this is committed ;)
Julien Kaspar changed title from WIP: Retopology Overlay to #70267 WIP: Retopology Overlay 2023-02-11 16:12:23 +01:00
Jorijn de Graaf changed title from #70267 WIP: Retopology Overlay to #70267 Retopology Overlay 2023-02-11 19:38:40 +01:00
Author
Contributor

IMO the Z-bias slider shouldn't be able to go all the way to 0 and turn the overlay off. Only the toggle should be able to disable the overlay completely. When sliding the z-bias to the lowest value, it should instead only disable the Z bias but keep the mesh overlay intact (Kinda like the old 'Hidden Wire' overlay).

Done 👍

> IMO the Z-bias slider shouldn't be able to go all the way to 0 and turn the overlay off. Only the toggle should be able to disable the overlay completely. When sliding the z-bias to the lowest value, it should instead only disable the Z bias but keep the mesh overlay intact (Kinda like the old 'Hidden Wire' overlay). Done 👍
Author
Contributor

It's quite difficult to tell front faces from back faces in retopology mode.

I have two proposed solutions:

  1. Use a different color for front and back (I was thinking blue and red, though I'm not sure what to do about selected faces for example; would they be orange on both sides?).
  2. Cull back faces entirely (though I don't know how to do it for vertices and edges).

Update: After experimenting with two colors and getting some feedback, I've decided to keep it simple and go with back face culling.

It's quite difficult to tell front faces from back faces in retopology mode. I have two proposed solutions: 1. Use a different color for front and back (I was thinking blue and red, though I'm not sure what to do about selected faces for example; would they be orange on both sides?). 2. Cull back faces entirely (though I don't know how to do it for vertices and edges). Update: After experimenting with two colors and getting some feedback, I've decided to keep it simple and go with back face culling.
Author
Contributor

Oh and another note: I think the depth only pass uses a mesh with modifiers that have "show in edit mode" enabled, while the overlay pass uses a mesh with modifiers that have "show on cage" enabled; this discrepancy can lead to weird artifacts.
Tomorrow I'll look into making it use the same mesh for both passes when the retopology overlay is on.
Update: Fixed in commit 0338a79f34 :)

Oh and another note: I think the depth only pass uses a mesh with modifiers that have "show in edit mode" enabled, while the overlay pass uses a mesh with modifiers that have "show on cage" enabled; this discrepancy can lead to weird artifacts. Tomorrow I'll look into making it use the same mesh for both passes when the retopology overlay is on. Update: Fixed in commit 0338a79f34 :)
Jorijn de Graaf changed title from #70267 Retopology Overlay to WIP: #70267 Retopology Overlay 2023-02-12 00:40:07 +01:00
Jorijn de Graaf force-pushed retopology-overlay from 0338a79f34 to cd3ec56a76 2023-02-13 13:34:14 +01:00 Compare
Author
Contributor

Sorry about the force push, there were merge conflicts and I didn't know how else to fix it.

Sorry about the force push, there were merge conflicts and I didn't know how else to fix it.
Clément Foucault requested changes 2023-02-14 02:23:42 +01:00
@ -9,5 +9,8 @@ void main()
vec3 world_pos = point_object_to_world(pos);
gl_Position = point_world_to_ndc(world_pos);
/* Offset Z position for retopology overlay. */

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`.
Author
Contributor

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.

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.
Author
Contributor

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.

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).
Author
Contributor

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).
Author
Contributor

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.
bonj marked this conversation as resolved
@ -10,2 +10,4 @@
gl_Position = point_world_to_ndc(world_pos);
/* Offset Z position for retopology overlay. */
gl_Position.z -= max(0.0, retopologyBias) / abs(gl_Position.w);

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

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

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.

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.
Author
Contributor

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.
bonj marked this conversation as resolved
@ -96,2 +96,4 @@
#endif
/* Offset Z position for retopology overlay. */
gl_Position.z -= max(0.0, retopologyBias) / abs(gl_Position.w);

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.
Author
Contributor

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.
Author
Contributor

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; ```
Author
Contributor

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.
bonj marked this conversation as resolved
@ -0,0 +1,13 @@

Did you create this file only to modify the depth_only shader?

If that's so, see my other comment and keep the depth_only for the uniform color shader.

Did you create this file only to modify the `depth_only` shader? If that's so, see my other comment and keep the depth_only for the uniform color shader.
Author
Contributor

Seeing as this and the first requested change are about the same thing, I'll mark it as resolved.

Seeing as this and the first requested change are about the same thing, I'll mark it as resolved.
bonj marked this conversation as resolved
Clément Foucault added this to the EEVEE & Viewport project 2023-02-14 13:25:00 +01:00
Jorijn de Graaf force-pushed retopology-overlay from 22b25cc13f to 8e4bd8650d 2023-02-14 19:18:16 +01:00 Compare
Author
Contributor

Another merge conflict, another force push.
This time I fixed the conflict before pushing at least.

Another merge conflict, another force push. This time I fixed the conflict before pushing at least.
Jorijn de Graaf force-pushed retopology-overlay from 06aa7c9998 to 766873fdb2 2023-02-14 22:29:56 +01:00 Compare

Just as a note, I have to check if the offset in the shader make sense mathematically.

Also would be nice to have the other reviewers opinion about usability.

Just as a note, I have to check if the offset in the shader make sense mathematically. Also would be nice to have the other reviewers opinion about usability.
Jorijn de Graaf force-pushed retopology-overlay from cda3836fd5 to a9b010c011 2023-02-15 16:02:20 +01:00 Compare
Member

I'm getting a build error so I can't test it yet.
Let me know when it's ready for me to review again 👍

I'm getting a build error so I can't test it yet. Let me know when it's ready for me to review again 👍
Author
Contributor

I'm getting a build error so I can't test it yet.
Let me know when it's ready for me to review again 👍

Are you building with Cycles and stuff? I'm not, so maybe that's why it's building for me.
Could you tell me what kind of error you're getting?
There were some new commits on main, I merged them, does this help?

> I'm getting a build error so I can't test it yet. > Let me know when it's ready for me to review again 👍 Are you building with Cycles and stuff? I'm not, so maybe that's why it's building for me. Could you tell me what kind of error you're getting? There were some new commits on main, I merged them, does this help?
Julien Kaspar approved these changes 2023-02-16 11:28:00 +01:00
Julien Kaspar left a comment
Member

Ah now it builds. All my notes are addressed. I'll keep testing it but functionality wise it works great in every aspect now 👍

Ah now it builds. All my notes are addressed. I'll keep testing it but functionality wise it works great in every aspect now 👍
Member

A bit off topic:
I tested this for a while now and the next best improvement to work on is to limit edit mode selection to only visible geometry when using this overlay.
The visualization is clearer but it causes accidental selection of culled geometry to happen much more frequently without even realizing it in the moment (because the selection isn't visible).
It happens so often that I started setting the Z-bias to 1.0 when working on more complex surrounding meshes, essentially making the Z-bias slider almost useless in most cases.

A bit off topic: I tested this for a while now and the next best improvement to work on is to limit edit mode selection to only visible geometry when using this overlay. The visualization is clearer but it causes accidental selection of culled geometry to happen much more frequently without even realizing it in the moment (because the selection isn't visible). It happens so often that I started setting the Z-bias to 1.0 when working on more complex surrounding meshes, essentially making the Z-bias slider almost useless in most cases.
Author
Contributor

I have noticed this issue, though it doesn't bother me that much because I don't rely on box/lasso/etc select.
It would be much better if you could only select visible geometry, but I have no idea how to implement that.

I have noticed this issue, though it doesn't bother me that much because I don't rely on box/lasso/etc select. It would be much better if you could only select visible geometry, but I have no idea how to implement that.
Member

It even happens regularly with vertex and edge selection on open boundaries because the click doesn't have to be precise.

But yes, this issue goes beyond this feature. Something to look at in the future.

It even happens regularly with vertex and edge selection on open boundaries because the click doesn't have to be precise. But yes, this issue goes beyond this feature. Something to look at in the future.
Author
Contributor

I would like to improve selection, but not in this pull request.
If we keep adding onto the scope it'll never get merged.

I would like to improve selection, but not in this pull request. If we keep adding onto the scope it'll never get merged.
Member

When building Blender today with this patch applied, it doesn't occlude its own faces anymore and instead also uses the Z-bias for that.
Can you preroduce this?

image

When building Blender today with this patch applied, it doesn't occlude its own faces anymore and instead also uses the Z-bias for that. Can you preroduce this? ![image](/attachments/c3ff9207-69d7-4f55-be79-9bd92272e809)
598 KiB
Jorijn de Graaf force-pushed retopology-overlay from e34468ffba to c808d9358b 2023-02-19 17:15:49 +01:00 Compare
Author
Contributor

@JulienKaspar good catch!
It took a while to figure this bug out, but it should be fixed now.
Turns out I was reusing a variable intended for another shader, so whichever was used first would be used for both of them; now it has its own variable.

@JulienKaspar good catch! It took a while to figure this bug out, but it should be fixed now. Turns out I was reusing a variable intended for another shader, so whichever was used first would be used for both of them; now it has its own variable.
Jorijn de Graaf requested review from Clément Foucault 2023-02-22 00:24:43 +01:00
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR104599) when ready.
Member

@blender-bot build

@blender-bot build
Jorijn de Graaf force-pushed retopology-overlay from 86d8bdfec9 to 79a9cea993 2023-02-23 23:23:44 +01:00 Compare
Jorijn de Graaf force-pushed retopology-overlay from 2de17c2e34 to ad07bd6931 2023-02-26 17:04:01 +01:00 Compare
Jorijn de Graaf added 1 commit 2023-02-26 18:26:12 +01:00
Author
Contributor

Now that the math is good, and there aren't any known issues, I think this PR might be ready.
The only things left to do are things I can't do myself: test with Metal, and update localization.

Now that the math is good, and there aren't any known issues, I think this PR might be ready. The only things left to do are things I can't do myself: test with Metal, and update localization.
Jorijn de Graaf changed title from WIP: #70267 Retopology Overlay to #70267 Retopology Overlay 2023-02-26 18:34:21 +01:00
Clément Foucault requested changes 2023-02-26 19:06:27 +01:00
Clément Foucault left a comment
Member

This feels nice! Only some final touch ups required in my opinion.

This feels nice! Only some final touch ups required in my opinion.
@ -102,2 +106,3 @@
/* Complementary Depth Pass */
state = DRW_STATE_WRITE_DEPTH | DRW_STATE_DEPTH_LESS_EQUAL | DRW_STATE_CULL_BACK;
state = DRW_STATE_WRITE_DEPTH | DRW_STATE_DEPTH_LESS_EQUAL |
((show_retopology) ? DRWState(0) : DRW_STATE_CULL_BACK);

Style: Prefer:

if (show_retopology) {
  /* Do not cull backfaces for retopology. This is because [insert explanation here] */
  state &= ~DRW_STATE_CULL_BACK;
}
Style: Prefer: ``` if (show_retopology) { /* Do not cull backfaces for retopology. This is because [insert explanation here] */ state &= ~DRW_STATE_CULL_BACK; }
Author
Contributor

Added an if block and explanation, as discussed in DMs.

Added an if block and explanation, as discussed in DMs.
bonj marked this conversation as resolved
@ -149,3 +156,3 @@
DRWShadingGroup **shgrp = (j == 0) ? &pd->edit_mesh_faces_grp[i] :
&pd->edit_mesh_faces_cage_grp[i];
state = state_common;
state = state_common | ((show_retopology) ? DRW_STATE_CULL_BACK : DRWState(0));

Same thing here.

Same thing here.
Author
Contributor

Added an if block and explanation, as discussed in DMs.

Added an if block and explanation, as discussed in DMs.
bonj marked this conversation as resolved
@ -258,2 +258,4 @@
}
/* From "Projection Matrix Tricks" by Eric Lengyel:
* http://www.terathon.com/gdc07_lengyel.pdf (p. 18 Depth Modification)

My bad. The equation is actually comming from page 24.

My bad. The equation is actually comming from page 24.
Author
Contributor

I thought you picked 18 because it's the start of the section about depth modification. I think it makes sense to point to the whole range of pages (18-26).

I thought you picked 18 because it's the start of the section about depth modification. I think it makes sense to point to the whole range of pages (18-26).

Yes, but here it is the reference to a specific formula. I also think this reference should be inside the function. Comment on top of function is only for documentation of the function usage.

Yes, but here it is the reference to a specific formula. I also think this reference should be inside the function. Comment on top of function is only for documentation of the function usage.
Author
Contributor

Done.

Done.
bonj marked this conversation as resolved
@ -260,0 +262,4 @@
*
* View Z is used to adjust for perspective projection.
* Homogenous W is used to convert from NDC to homogenous space. */
float get_homogenous_z_offset(float vs_z, float hs_w, float offset)

Maybe also add a note that offset is in viewspace. So positive values are closer to camera.

Maybe also add a note that offset is in viewspace. So positive values are closer to camera.
Author
Contributor

Done, now each parameter has an explanation.

Done, now each parameter has an explanation.
bonj marked this conversation as resolved
@ -260,0 +265,4 @@
float get_homogenous_z_offset(float vs_z, float hs_w, float offset)
{
if (ProjectionMatrix[3][3] == 0.0) {
return -ProjectionMatrix[3][2] * (offset / (vs_z * (vs_z + offset))) * hs_w;

Remove the negative sign here and everywhere get_homogenous_z_offset is called.
The negative sign in the equation from the slide is inside ProjectionMatrix[3][2].

Remove the negative sign here and everywhere `get_homogenous_z_offset` is called. The negative sign in the equation from the slide is inside `ProjectionMatrix[3][2]`.
Author
Contributor

Done and done.
I negated it because then you can subtract from the Z, which is what other offsets in the shader do, but yeah it was unnecessary so now they just use += for this.

Done and done. I negated it because then you can subtract from the Z, which is what other offsets in the shader do, but yeah it was unnecessary so now they just use += for this.
bonj marked this conversation as resolved
@ -4533,2 +4532,4 @@
RNA_def_property_ui_text(prop, "Retopology", "Use retopology display");
RNA_def_property_update(prop, NC_SPACE | ND_SPACE_VIEW3D | NS_VIEW3D_SHADING, NULL);
prop = RNA_def_property(srna, "retopology_bias", PROP_FLOAT, PROP_FACTOR);

Now that we fixed the bias equation, the property types should be PROP_DISTANCE.

You will loose the ability to see the blue fill bar but it will be much explicit that this value is in distance unit.

You might have to adjust the sensitivity to have kind of the same drag feeling as when it was a factor.
And bump the soft max to 2 or 10 (seems nicer to discover that this isn't clamped).
This is what control those RNA_def_property_ui_range(prop, 0.0f, 1.0f, 0.1f, 3);.

Now that we fixed the bias equation, the property types should be `PROP_DISTANCE`. You will loose the ability to see the blue fill bar but it will be much explicit that this value is in distance unit. You might have to adjust the sensitivity to have kind of the same drag feeling as when it was a factor. And bump the soft max to 2 or 10 (seems nicer to discover that this isn't clamped). This is what control those `RNA_def_property_ui_range(prop, 0.0f, 1.0f, 0.1f, 3);`.
Author
Contributor

Made it PROP_DISTANCE and upped the soft max to 10.
I think 0.1 step and 3 digits still feels good so I'll leave those the same.

Made it PROP_DISTANCE and upped the soft max to 10. I think 0.1 step and 3 digits still feels good so I'll leave those the same.
bonj marked this conversation as resolved
@ -4534,1 +4534,4 @@
prop = RNA_def_property(srna, "retopology_bias", PROP_FLOAT, PROP_FACTOR);
RNA_def_property_float_sdna(prop, NULL, "overlay.retopology_bias");
RNA_def_property_ui_text(prop, "Retopology Bias", "Z Bias used to draw edit mesh in front");

Replace description by Offset used to draw the edit mesh in front of other geometry.

Maybe @JulienKaspar can agree this is clearer? Z Bias is too technical in my opinion.

Replace description by `Offset used to draw the edit mesh in front of other geometry`. Maybe @JulienKaspar can agree this is clearer? `Z Bias` is too technical in my opinion.
Author
Contributor

I agree. I'll rename it to retopology offset everywhere.

I agree. I'll rename it to retopology offset everywhere.
bonj marked this conversation as resolved
@ -4535,0 +4535,4 @@
prop = RNA_def_property(srna, "retopology_bias", PROP_FLOAT, PROP_FACTOR);
RNA_def_property_float_sdna(prop, NULL, "overlay.retopology_bias");
RNA_def_property_ui_text(prop, "Retopology Bias", "Z Bias used to draw edit mesh in front");
RNA_def_property_range(prop, 0.0f, 100000.0f);

Max should be FLT_MAX. Arbitrary limits like these are not good.

Max should be FLT_MAX. Arbitrary limits like these are not good.
Author
Contributor

Done.

Done.
bonj marked this conversation as resolved

I think it is nice to link this comment #104921 (comment) to this PR. It has some nice related explanation and it prevents to see it forgotten. But no need to add that to the code comments.

I think it is nice to link this comment https://projects.blender.org/blender/blender/pulls/104921#issuecomment-888979 to this PR. It has some nice related explanation and it prevents to see it forgotten. But no need to add that to the code comments.
Jorijn de Graaf added 1 commit 2023-02-26 22:51:35 +01:00
2762fd7b8d Implement retopology overlay requests
The bias property is now called offset, not just in the UI but everywhere, because I like consistency.
get_homogenous_z_offset no longer negates its output, and has some more documentation.
The offset property is now of distance type, because it is a viewspace offset.
To make this more clear the soft max is increased to 10.
Because there's no point in arbitrarily limiting the values users can input, the hard max is FLT_MAX now.
I kept step the same, because it good the way it is.
Author
Contributor

@fclem I think I addressed all your requests in the last commit.
Also I added a reference to #104921 in this PR.

@fclem I think I addressed all your requests in the last commit. Also I added a reference to #104921 in this PR.
Clément Foucault approved these changes 2023-02-27 09:03:32 +01:00
Jorijn de Graaf added 1 commit 2023-02-27 17:38:57 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
c11b43952f
Merge branch 'main' into retopology-overlay
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR104599) when ready.
Jorijn de Graaf added 1 commit 2023-02-28 00:15:23 +01:00
Jorijn de Graaf added 1 commit 2023-02-28 00:49:15 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
9240dd0e13
Merge branch 'main' into retopology-overlay
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR104599) when ready.
Jeroen Bakker reviewed 2023-02-28 04:00:22 +01:00
@ -65,0 +64,4 @@
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_offset = (show_retopology) ? (v3d->overlay.retopology_offset + FLT_EPSILON) :
Member

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.
Author
Contributor

Makes sense, I'll do that.

Makes sense, I'll do that.
bonj marked this conversation as resolved
Jeroen Bakker approved these changes 2023-02-28 04:00:41 +01:00
Jorijn de Graaf added 1 commit 2023-02-28 04:22:39 +01:00
e7d2e05fa1 Use max instead of plus for retopology epsilon
Jeroen Bakker requested this, and it makes sense to me.
He also asked me to get rid of the comment.
Jorijn de Graaf added 1 commit 2023-02-28 04:27:28 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
bae2564702
Merge branch 'main' into retopology-overlay
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR104599) when ready.
Jorijn de Graaf added 1 commit 2023-02-28 13:38:56 +01:00
3a90751a0b Rename parameter for get_homogenous_z_offset
Apparently offset is already used, and you can't overwrite it on Metal, so I'm renaming it to vs_offset.
Also I'm adjusting the comment a little.
Jorijn de Graaf added 1 commit 2023-02-28 13:56:14 +01:00
40a930e13c Reuse view_pos for facing based color blend
I'm already calculating the view position earlier in the function, so might as well reuse it here instead of calculating it again.
Jorijn de Graaf changed title from #70267 Retopology Overlay to WIP: #70267 Retopology Overlay 2023-02-28 14:04:26 +01:00
Author
Contributor

It's back to WIP because there's an issue when your offset is bigger than the distance to the camera.
Update: This is now fixed by clamping the offset to half of the current Z value.

It's back to WIP because there's an issue when your offset is bigger than the distance to the camera. Update: This is now fixed by clamping the offset to half of the current Z value.
Jorijn de Graaf changed title from WIP: #70267 Retopology Overlay to #70267 Retopology Overlay 2023-02-28 18:13:55 +01:00
Jorijn de Graaf added 1 commit 2023-02-28 18:14:02 +01:00
2e92bcdae6 Clamp retopology offset for perspective projection
This way it can't offset a vertex to behind the camera, and by keeping the ratio between Z and offset large enough, it avoids floating point precision errors.
Clément Foucault added 1 commit 2023-02-28 18:17:40 +01:00
Clément Foucault added 1 commit 2023-02-28 18:32:39 +01:00
Jorijn de Graaf added 1 commit 2023-03-01 00:26:08 +01:00
113dc37272 Remove unused variable rv3d
This was necessary when the clip distances were used to calculate offset, but that's done inside the shader now, so it's no longer needed.
Clément Foucault added 1 commit 2023-03-03 00:29:52 +01:00
Clément Foucault merged commit dcad51dfc3 into main 2023-03-03 00:35:56 +01:00
First-time contributor

would be great if expose color of the overlay in the pannel :)

would be great if expose color of the overlay in the pannel :)
Author
Contributor

would be great if expose color of the overlay in the pannel :)

Might be worth considering. In the meantime you can change it in the user preferences.

> would be great if expose color of the overlay in the pannel :) Might be worth considering. In the meantime you can change it in the user preferences.
First-time contributor

@bonj If the hidden wire are also included with the inclusion of retopology, then how do I know that I have a filled polygon in front of me and not just the edges?

@bonj If the hidden wire are also included with the inclusion of retopology, then how do I know that I have a filled polygon in front of me and not just the edges?
Author
Contributor

@bonj If the hidden wire are also included with the inclusion of retopology, then how do I know that I have a filled polygon in front of me and not just the edges?

The faces are supposed to be colored (blue by default).
If this is not the case, you may need to reset your theme.
I think it defaults to transparent black if you install Blender with retopology overlay, after you already had an older version of 3.6 installed.
When creating a new userprefs file it sets the default correctly, but when reading an old one it doesn't.
Perhaps this issue is unique to the retopology overlay color, in which case I need to fix it.
Will do some more research.

> @bonj If the hidden wire are also included with the inclusion of retopology, then how do I know that I have a filled polygon in front of me and not just the edges? The faces are supposed to be colored (blue by default). If this is not the case, you may need to reset your theme. I think it defaults to transparent black if you install Blender with retopology overlay, after you already had an older version of 3.6 installed. When creating a new userprefs file it sets the default correctly, but when reading an old one it doesn't. Perhaps this issue is unique to the retopology overlay color, in which case I need to fix it. Will do some more research.
Member

there is also versioning code for the userpref. We might have forgotten this.

there is also versioning code for the userpref. We might have forgotten this.
Author
Contributor

there is also versioning code for the userpref. We might have forgotten this.

Hans Goudey in the Blender Chat informed me that I need to add it to do_versions_theme.
For this I made PR #105539 and requested your review.

> there is also versioning code for the userpref. We might have forgotten this. Hans Goudey in the Blender Chat informed me that I need to add it to `do_versions_theme`. For this I made PR #105539 and requested your review.
First-time contributor

Hello, first of all thank you very much for fixing this, I've been waiting for 3 years for blender to change this, when I have many polygons it hurts my eyes.

-When I activate with edit mode the option "infront", the retopology option does not work, the polygons on the other side are still seen, I just need the infront to work in the area that I am seeing.

You can see the ear that is on the other side, without infront it works fine but in some cases this option is needed

Hello, first of all thank you very much for fixing this, I've been waiting for 3 years for blender to change this, when I have many polygons it hurts my eyes. -When I activate with edit mode the option "infront", the retopology option does not work, the polygons on the other side are still seen, I just need the infront to work in the area that I am seeing. You can see the ear that is on the other side, without infront it works fine but in some cases this option is needed

This is not a support ticket. I would suggest to make and link a devtalk.blender.org topic about this feature to gather user feedback.

This is not a support ticket. I would suggest to make and link a devtalk.blender.org topic about this feature to gather user feedback.
Author
Contributor

@kleyco I made a thread on BlenderArtists a while ago: https://blenderartists.org/t/blender-retopology-overlay/1451351
If you repost your message there, we can continue the conversation.

@kleyco I made a thread on BlenderArtists a while ago: https://blenderartists.org/t/blender-retopology-overlay/1451351 If you repost your message there, we can continue the conversation.
First-time contributor
Please check if this issue is related: [Bsurfaces addon stopped working (probably after the new Retopology Overlay addition)](https://projects.blender.org/blender/blender-addons/issues/104484).
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
10 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#104599
No description provided.