blender
/
blender
128
396
Fork 569
Code Issues 6.2k Pull Requests 755 Projects 19 Wiki Activity

DRW: Selection Occlusion #105498

Merged
Clément Foucault merged 16 commits from bonj/blender:retopology-select into main 2023-04-09 08:08:13 +02:00
Conversation 18 Commits 16 Files Changed 6 +56 -2
Jorijn de Graaf commented 2023-03-06 16:59:15 +01:00
Contributor
Copy Link

Summary

Occlude edit mode selection behind objects in object mode.

Problem

When doing retopology, you want to be able to select your edit mesh,
but only when you can see it.
Being able to select geometry behind reference objects is not
desirable.

Solution

Make it so reference objects occlude selection, while the edit mesh is
pushed towards the view using retopology offset.

Limitations

Poly Build is not supported, because it doesn't use the depth buffer.
It behaves the same as normal, unoccluded by reference meshes.

Notes

Selection occlusion is not used when xray is enabled. This is
intentional.

#### Summary Occlude edit mode selection behind objects in object mode. #### Problem When doing retopology, you want to be able to select your edit mesh, but only when you can see it. Being able to select geometry behind reference objects is not desirable. #### Solution Make it so reference objects occlude selection, while the edit mesh is pushed towards the view using retopology offset. #### Limitations Poly Build is not supported, because it doesn't use the depth buffer. It behaves the same as normal, unoccluded by reference meshes. #### Notes Selection occlusion is not used when xray is enabled. This is intentional.
blender_bvPnQ53cXR.mp4
8.5 MiB
👍 3 ❤️ 4
Jorijn de Graaf requested review from Germano Cavalcante 2023-03-08 19:57:37 +01:00
Jorijn de Graaf requested review from Clément Foucault 2023-03-08 19:57:37 +01:00
Jorijn de Graaf requested review from Julien Kaspar 2023-03-09 13:54:46 +01:00
Jorijn de Graaf requested review from Jeroen Bakker 2023-03-09 13:55:06 +01:00
Hans Goudey added this to the Modeling project 2023-03-09 23:05:03 +01:00
Jorijn de Graaf force-pushed retopology-select from c7f5073002 to dfe02741ff 2023-03-13 16:55:34 +01:00 Compare
Jorijn de Graaf requested review from Hans Goudey 2023-03-14 00:29:27 +01:00
Jorijn de Graaf changed title from WIP: Selection Occlusion to Selection Occlusion 2023-03-22 15:20:15 +01:00
Ray molenkamp commented 2023-03-23 01:40:35 +01:00
Member
Copy Link

@blender-bot build

@blender-bot build
Germano Cavalcante commented 2023-03-24 14:58:45 +01:00
Member
Copy Link

I don't think it's appropriate to create an array with all objects visible because:

  • Creates an unnecessarily large array
  • This can extrapolate the indices in the texture of indices (created by the selection engine) since there are no equal indices there, so each object has an initial offset that corresponds to the number of selectable elements (last_offset + vertice + edge + polygons))
  • The selection engine stores a data for each object. This takes up memory space and can be a bit bad for other engines that also store a data for each object. It would be nice to avoid storing unnecessary data for so many objects.
  • This array is something that can be customized by the caller. So in theory it could contain any object, so using the DRW_object_is_in_edit_mode(ob) check limits what the caller can do by forcing it to use a specific array.

A more ideal solution in my view would be to change the DRW_draw_select_id to do the same as the other draw loops and traverse all objects.

Somehow it is necessary to tag the objects that are in SELECTID_Context::objects.

I don't think it's appropriate to create an array with all objects visible because: - Creates an unnecessarily large array - This can extrapolate the indices in the texture of indices (created by the selection engine) since there are no equal indices there, so each object has an initial offset that corresponds to the number of selectable elements (last_offset + vertice + edge + polygons)) - The selection engine stores a data for each object. This takes up memory space and can be a bit bad for other engines that also store a data for each object. It would be nice to avoid storing unnecessary data for so many objects. - This array is something that can be customized by the caller. So in theory it could contain any object, so using the `DRW_object_is_in_edit_mode(ob)` check limits what the caller can do by forcing it to use a specific array. A more ideal solution in my view would be to change the `DRW_draw_select_id` to do the same as the other draw loops and traverse all objects. Somehow it is necessary to tag the objects that are in `SELECTID_Context::objects`.
Jorijn de Graaf commented 2023-03-24 15:15:10 +01:00
Author
Contributor
Copy Link

Thanks, this is exactly the info I needed!
I'll make it use the depsgraph iterator.
Perhaps that would allow it to support non-mesh solid objects (such as metaballs) too.

Thanks, this is exactly the info I needed! I'll make it use the depsgraph iterator. Perhaps that would allow it to support non-mesh solid objects (such as metaballs) too.
Jorijn de Graaf force-pushed retopology-select from 9aea413b6e to ef85581bdf 2023-03-24 21:38:42 +01:00 Compare
Jorijn de Graaf commented 2023-03-24 21:41:08 +01:00
Author
Contributor
Copy Link

The history became so messy I decided to reset and redo only the necessary changes.
This way it's easier to keep up to date with main without merge conflicts, and easier to review.
You can find the old commits here: https://projects.blender.org/bonj/blender/src/branch/retopology-select-old

The history became so messy I decided to reset and redo only the necessary changes. This way it's easier to keep up to date with main without merge conflicts, and easier to review. You can find the old commits here: https://projects.blender.org/bonj/blender/src/branch/retopology-select-old
Jorijn de Graaf commented 2023-03-24 23:00:29 +01:00
Author
Contributor
Copy Link

@mano-wii I've implemented your feedback, and as a result the code is much simpler, and works better too 🙂

@mano-wii I've implemented your feedback, and as a result the code is much simpler, and works better too 🙂
Ray molenkamp commented 2023-03-25 00:02:13 +01:00
Member
Copy Link

@blender-bot package

@blender-bot package
Blender Bot commented 2023-03-25 00:02:16 +01:00
Member
Copy Link

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105498) when ready.
Jorijn de Graaf force-pushed retopology-select from ef85581bdf to 38293cd17c 2023-03-25 16:18:28 +01:00 Compare
Jorijn de Graaf commented 2023-03-25 16:21:12 +01:00
Author
Contributor
Copy Link

My cleanup got merged before this PR so I rebased this branch to avoid merge conflicts.

My cleanup got merged before this PR so I rebased this branch to avoid merge conflicts.
Hans Goudey commented 2023-03-27 20:57:02 +02:00
Member
Copy Link

The changes look reasonable to me, but I don't have experience in this area, so I'll leave the review to others. Thanks for working on this and for simplifying it so nicely!

The changes look reasonable to me, but I don't have experience in this area, so I'll leave the review to others. Thanks for working on this and for simplifying it so nicely!
Hans Goudey refused to review 2023-03-27 20:57:07 +02:00
Germano Cavalcante approved these changes 2023-03-27 21:47:46 +02:00
Germano Cavalcante left a comment
Member
Copy Link

The change seems safe. But there are a few things that I found odd since I'm not familiar with the latest changes made to the mesh editing overlay.

The selection engine should have matrices and depths matching the edit overlay. But I noticed that get_homogenous_z_offset is only being added to the selection engine in this patch. Does this indicate that they are not currently matching?

Overall it looks OK.

The change seems safe. But there are a few things that I found odd since I'm not familiar with the latest changes made to the mesh editing overlay. The selection engine should have matrices and depths matching the edit overlay. But I noticed that `get_homogenous_z_offset` is only being added to the selection engine in this patch. Does this indicate that they are not currently matching? Overall it looks OK.
Germano Cavalcante requested changes 2023-03-27 21:53:24 +02:00
Germano Cavalcante left a comment
Member
Copy Link

It's actually not a safe change. It doesn't seem that there are guarantees that DEG_OBJECT_ITER_BEGIN (&deg_iter_settings, ob) {...} and the objects in sel_ctx->objects are in the same order.

Note that it is important to follow an order to return the correct index of the selected object.

Also testing DRW_object_is_in_edit_mode(ob) does not guarantee that we are drawing objects are the same as the array.

It's actually not a safe change. It doesn't seem that there are guarantees that `DEG_OBJECT_ITER_BEGIN (&deg_iter_settings, ob) {...}` and the objects in `sel_ctx->objects` are in the same order. Note that it is important to follow an order to return the correct index of the selected object. Also testing `DRW_object_is_in_edit_mode(ob)` does not guarantee that we are drawing objects are the same as the array.
Jorijn de Graaf commented 2023-03-27 23:16:04 +02:00
Author
Contributor
Copy Link

The selection engine should have matrices and depths matching the edit overlay. But I noticed that get_homogenous_z_offset is only being added to the selection engine in this patch. Does this indicate that they are not currently matching?

These same changes are already done for the edit overlay in commit dcad51dfc3.
Though there it pushes the overlay towards the view, and here it pushes background objects away from the view, because it's simpler to implement in this case.

Note that it is important to follow an order to return the correct index of the selected object.

While it seems to work exactly as intended already, I'll take your word for it.
I suppose I could loop over the objects list first, then through the depsgraph iterator to cache any non-edit objects.
The order of background objects shouldn't matter because they only affect the depth pass.
@mano-wii What do you think of that solution? Implemented now in commit f8c26304d7.

Also testing DRW_object_is_in_edit_mode(ob) does not guarantee that we are drawing objects are the same as the array.

The objects array is populated from bases in edit mode, so checking if an object is not in edit mode at least guarantees that it's not in the list, and I think the inverse is also true.
Do you have any examples where it doesn't work this way?

> The selection engine should have matrices and depths matching the edit overlay. But I noticed that get_homogenous_z_offset is only being added to the selection engine in this patch. Does this indicate that they are not currently matching? These same changes are already done for the edit overlay in commit https://projects.blender.org/blender/blender/commit/dcad51dfc3c5ead718bfe744669effdc304f79fb. Though there it pushes the overlay towards the view, and here it pushes background objects away from the view, because it's simpler to implement in this case. > Note that it is important to follow an order to return the correct index of the selected object. While it seems to work exactly as intended already, I'll take your word for it. I suppose I could loop over the objects list first, then through the depsgraph iterator to cache any non-edit objects. The order of background objects shouldn't matter because they only affect the depth pass. @mano-wii What do you think of that solution? Implemented now in commit https://projects.blender.org/blender/blender/commit/f8c26304d7bda18c1ab6a4d410991e979b95cf66. > Also testing DRW_object_is_in_edit_mode(ob) does not guarantee that we are drawing objects are the same as the array. The objects array is populated from bases in edit mode, so checking if an object is not in edit mode at least guarantees that it's not in the list, and I think the inverse is also true. Do you have any examples where it doesn't work this way?
Jorijn de Graaf added 1 commit 2023-03-27 23:45:01 +02:00
f8c26304d7 Draw occluders separately from edit meshes 
First it draws the objects from the selection context object list like it used to, because apparently the order matters.
Then it draws non-edit objects from the depsgraph iterator; the order doesn't matter here because it's only for the depth pass.
Germano Cavalcante approved these changes 2023-03-28 14:59:02 +02:00
Germano Cavalcante left a comment
Member
Copy Link

It doesn't seem the most ideal as the sel_ctx->objects array doesn't necessarily have to be all !DRW_object_is_in_edit_mode(ob) objects, but it should work fine now.

It doesn't seem the most ideal as the `sel_ctx->objects` array doesn't necessarily have to be all `!DRW_object_is_in_edit_mode(ob)` objects, but it should work fine now.
source/blender/draw/intern/draw_manager.c Outdated
@ -2780,0 +2786,4 @@
}
}
DEG_OBJECT_ITER_END;
Germano Cavalcante commented 2023-03-28 14:52:59 +02:00
Member
Copy Link

It might be interesting to put this code inside a condition that checks if it's not X-Ray or wireframe. It may even improve readability.

Why only objects of type OB_MESH have the depth drawn? It may be good to comment on the reason.

It might be interesting to put this code inside a condition that checks if it's not X-Ray or wireframe. It may even improve readability. Why only objects of type `OB_MESH` have the depth drawn? It may be good to comment on the reason.
bonj marked this conversation as resolved
Jorijn de Graaf commented 2023-03-28 15:06:59 +02:00
Author
Contributor
Copy Link

It doesn't seem the most ideal as the sel_ctx->objects array doesn't necessarily have to be all !DRW_object_is_in_edit_mode(ob) objects, but it should work fine now.

Checking if an object is not in edit mode gives you background objects, used for occlusion.
These are not in the selection context objects array.
If the objects array did contain a non-edit object, it would crash, so I'm confident that never happens.

It might be interesting to put this code inside a condition that checks if it's not X-Ray or wireframe. It may even improve readability.

It doesn't occlude in xray so I didn't add an extra check, but I suppose I could check that, and check if retopology is enabled, then remove that check from the select engine cache function.

Why only objects of type OB_MESH have the depth drawn? It may be good to comment on the reason.

Because the depsgraph iterator contains evaluated meshes for every object, but also non-mesh versions, which are superfluous and don't have triangles to cache.

> It doesn't seem the most ideal as the `sel_ctx->objects` array doesn't necessarily have to be all `!DRW_object_is_in_edit_mode(ob)` objects, but it should work fine now. Checking if an object is *not* in edit mode gives you background objects, used for occlusion. These are *not* in the selection context objects array. If the objects array did contain a non-edit object, it would crash, so I'm confident that never happens. > It might be interesting to put this code inside a condition that checks if it's not X-Ray or wireframe. It may even improve readability. It doesn't occlude in xray so I didn't add an extra check, but I suppose I could check that, and check if retopology is enabled, then remove that check from the select engine cache function. > Why only objects of type OB_MESH have the depth drawn? It may be good to comment on the reason. Because the depsgraph iterator contains evaluated meshes for every object, but also non-mesh versions, which are superfluous and don't have triangles to cache.
Jorijn de Graaf added 1 commit 2023-03-28 18:22:33 +02:00
6424637919 Add comment explaining object type and mode checks 
Hopefully this explains well enough what the code is doing.
Jorijn de Graaf added 1 commit 2023-03-28 20:30:25 +02:00
3a85a0d907 Offset selection mesh towards view 
Instead of offsetting background objects away.
This way it perfectly matches the edit overlay, which matters when clipping is involved.
Jorijn de Graaf added 1 commit 2023-03-28 21:18:08 +02:00
a7a5f04530 Use retopology offset for selection depth group 
I forgot this one in the last commit, whoops.
Jorijn de Graaf added 1 commit 2023-03-28 21:43:19 +02:00
00cafbe04e Check if retopology is enabled for depsgraph iterator 
This is more efficient than checking it for every object, and perhaps easier to read too, because it indicates what the depsgraph iterator is for.
Jorijn de Graaf added 1 commit 2023-03-28 22:50:31 +02:00
3336c1b45e Check both retopology and xray for selection occlusion 
There's no need to use selection occlusion when xray is enabled, since it doesn't work anyway (and is a nice way of disabling selection occlusion without disabling the retopology overlay).
Since it's used in multiple places I decided to make a define, though only for v3d, not one that takes overlay because xray lives in shading.
Jorijn de Graaf added 1 commit 2023-03-28 22:54:19 +02:00
Jorijn de Graaf requested review from Germano Cavalcante 2023-03-28 23:59:48 +02:00
Jorijn de Graaf commented 2023-03-29 00:01:17 +02:00
Author
Contributor
Copy Link

Re-requesting review from @mano-wii because I made several changes.
They're all based on your feedback from the last review, and I don't expect you to find any problems, but just in case.

Re-requesting review from @mano-wii because I made several changes. They're all based on your feedback from the last review, and I don't expect you to find any problems, but just in case.
Germano Cavalcante approved these changes 2023-03-29 02:22:36 +02:00
Germano Cavalcante left a comment
Member
Copy Link

LGTM in general.

LGTM in general.
source/blender/draw/intern/draw_manager.c Outdated
@ -2780,0 +2786,4 @@
* It also has non-mesh objects however, which are not supported here.
*
* Only background (non-edit) objects are used for occlusion. */
if ((ob->type == OB_MESH) && !DRW_object_is_in_edit_mode(ob)) {
Germano Cavalcante commented 2023-03-29 02:22:07 +02:00
Member
Copy Link

Not that it's a problem, but another way to make comments more localized could be:

if (ob->type != OB_MESH) {
    /* Non-mesh objects are not supported here. */
    continue;
}

if (DRW_object_is_in_edit_mode(ob)) {
    /* Only background (non-edit) objects are used for occlusion. */
    continue;
}

I'm wondering if Grease Pencil and Image-type Empties couldn't contribute to the depth. But if it matches what you see in the Meshes edit overlay then it's OK.

Not that it's a problem, but another way to make comments more localized could be: ``` if (ob->type != OB_MESH) { /* Non-mesh objects are not supported here. */ continue; } if (DRW_object_is_in_edit_mode(ob)) { /* Only background (non-edit) objects are used for occlusion. */ continue; } ``` I'm wondering if Grease Pencil and Image-type Empties couldn't contribute to the depth. But if it matches what you see in the Meshes edit overlay then it's OK.
bonj marked this conversation as resolved
source/blender/editors/include/ED_view3d.h Outdated
@ -1335,6 +1335,7 @@ void ED_view3d_shade_update(struct Main *bmain, struct View3D *v3d, struct ScrAr
#define RETOPOLOGY_ENABLED(v3d) (OVERLAY_RETOPOLOGY_ENABLED((v3d)->overlay))
#define RETOPOLOGY_OFFSET(v3d) (OVERLAY_RETOPOLOGY_OFFSET((v3d)->overlay))
#define RETOPOLOGY_OCCLUSION(v3d) (RETOPOLOGY_ENABLED(v3d) && !XRAY_ENABLED(v3d))
Germano Cavalcante commented 2023-03-29 02:16:07 +02:00
Member
Copy Link

picky: I don't see much advantage in this macro. It makes it look like the retopology has a specific occlusion test, but seeing the definition, it's the same occlusion as the rest of the 3D View.
It might be more understandable to just bool use_occlusion = !XRAY_ENABLED(v3d); and use that bool in combination with RETOPOLOGY_ENABLED(v3d).

But I can be wrong.

picky: I don't see much advantage in this macro. It makes it look like the retopology has a specific occlusion test, but seeing the definition, it's the same occlusion as the rest of the 3D View. It might be more understandable to just `bool use_occlusion = !XRAY_ENABLED(v3d);` and use that bool in combination with `RETOPOLOGY_ENABLED(v3d)`. But I can be wrong.
bonj marked this conversation as resolved
Jorijn de Graaf commented 2023-03-29 02:35:39 +02:00
Author
Contributor
Copy Link

@mano-wii Replying doesn't seem to work properly so I'll just comment here.

To your first comment about localized comments and grease pencil objects:

Sure, I suppose two comments would be more readable, so I'll do it.
The depsgraph iterator has evaluated meshes for things like metaballs and surfaces.
I'm afraid I can't support empties or grease pencil objects though, because they have no triangles to cache.
I think it would require quite an overhaul to the selection engine to make this work.
Having your selection occluded by reference images actually sounds quite inconvenient anyway though.
Does make me think, I need to see how it currently behaves with translucent objects, and decide how it should behave with them.
I'll ask Julien what he thinks.
Update: Translucent meshes occlude the retopology overlay visually too, so it makes sense that they occlude selection. Basically I'm leaving this as it is.

To your second comment about the occlusion macro:

I made it mainly because it could be easy to forget the xray check.
If you use it in one place but not the other, it will crash.
I suppose since it's only used in two places though, I'll get rid of it.

@mano-wii Replying doesn't seem to work properly so I'll just comment here. To your first comment about localized comments and grease pencil objects: Sure, I suppose two comments would be more readable, so I'll do it. The depsgraph iterator has evaluated meshes for things like metaballs and surfaces. I'm afraid I can't support empties or grease pencil objects though, because they have no triangles to cache. I think it would require quite an overhaul to the selection engine to make this work. Having your selection occluded by reference images actually sounds quite inconvenient anyway though. Does make me think, I need to see how it currently behaves with translucent objects, and decide how it should behave with them. I'll ask Julien what he thinks. Update: Translucent meshes occlude the retopology overlay visually too, so it makes sense that they occlude selection. Basically I'm leaving this as it is. To your second comment about the occlusion macro: I made it mainly because it could be easy to forget the xray check. If you use it in one place but not the other, it will crash. I suppose since it's only used in two places though, I'll get rid of it.
Julien Kaspar requested changes 2023-03-29 10:36:58 +02:00
Julien Kaspar left a comment
Member
Copy Link

Overall I think this works great! This is definitely the desirable behavior imo.

The only issues I noticed are:

  • The selection is also occluded if the retopology_offset is set to 0. That should maybe not be the case.

  • Cases like when 'In Front' is enabled should be factored into the selectability of the mesh

  • There is a small difference between the selectability and the visibility as shown in the attached images.
    I did a full box selection and even though many vertices are not visually occluded, they are not selectable.

There seems to be a disconnect between all the three below

  1. visible overlay faces
  2. the visible mesh wires
  3. selectable mesh

The difference in what is selectable is also very noticable when switching between vert/edge/face selection.

I also think there needs to be a mention of this behavior in the tooltip of the overlay, but the tooltip is not very descriptive anyway right now.
Perhaps the tooltip could be rewritten to something like

Display mesh in front of other objects via an offset slider and limit selection to visible mesh if offset is larger than 0.

Overall I think this works great! This is definitely the desirable behavior imo. The only issues I noticed are: - The selection is also occluded if the `retopology_offset` is set to 0. That should maybe not be the case. - Cases like when 'In Front' is enabled should be factored into the selectability of the mesh - There is a small difference between the selectability and the visibility as shown in the attached images. I did a full box selection and even though many vertices are not visually occluded, they are not selectable. There seems to be a disconnect between all the three below 1. visible overlay faces 2. the visible mesh wires 3. selectable mesh The difference in what is selectable is also very noticable when switching between vert/edge/face selection. I also think there needs to be a mention of this behavior in the tooltip of the overlay, but the tooltip is not very descriptive anyway right now. Perhaps the tooltip could be rewritten to something like > Display mesh in front of other objects via an offset slider and limit selection to visible mesh if offset is larger than 0.
edge_selection.png
504 KiB
face_selection.png
493 KiB
vert_selection.png
492 KiB
edge_selection.png face_selection.png vert_selection.png
Jorijn de Graaf commented 2023-03-29 14:18:52 +02:00
Author
Contributor
Copy Link

@JulienKaspar Thanks for your feedback!

The selection is also occluded if the retopology_offset is set to 0. That should maybe not be the case.

I don't see why not. The slider is just for offset, if you don't want the overlay / occlusion, use the checkbox.
And if you want the overlay but no selection occlusion, use xray.

Cases like when 'In Front' is enabled should be factored into the selectability of the mesh.

That's fair, but I'm not sure how I'd go about fixing it.
This may just be a limitation to put in the documentation.
In my defense, it doesn't make any sense to use the retopology overlay and in front at the same time, as they are two different solutions to the same problem.
That's like wearing a blindfold and sunglasses at the same time.
Update: The edit selection engine doesn't understand in front, and adding that is out of scope. I wouldn't mind looking into this in a future PR though.

There is a small difference between the selectability and the visibility as shown in the attached images.
I did a full box selection and even though many vertices are not visually occluded, they are not selectable.

If you tested with the buildbot build, I've since changed the depth code, so it should be aligned perfectly with what's visible now.

I also think there needs to be a mention of this behavior in the tooltip of the overlay, but the tooltip is not very descriptive anyway right now.

Fair, I could mention the occlusion behavior in the tooltip of the checkbox.

@JulienKaspar Thanks for your feedback! > The selection is also occluded if the retopology_offset is set to 0. That should maybe not be the case. I don't see why not. The slider is just for offset, if you don't want the overlay / occlusion, use the checkbox. And if you want the overlay but no selection occlusion, use xray. > Cases like when 'In Front' is enabled should be factored into the selectability of the mesh. That's fair, but I'm not sure how I'd go about fixing it. This may just be a limitation to put in the documentation. In my defense, it doesn't make any sense to use the retopology overlay and in front at the same time, as they are two different solutions to the same problem. That's like wearing a blindfold and sunglasses at the same time. Update: The edit selection engine doesn't understand in front, and adding that is out of scope. I wouldn't mind looking into this in a future PR though. > There is a small difference between the selectability and the visibility as shown in the attached images. I did a full box selection and even though many vertices are not visually occluded, they are not selectable. If you tested with the buildbot build, I've since changed the depth code, so it should be aligned perfectly with what's visible now. > I also think there needs to be a mention of this behavior in the tooltip of the overlay, but the tooltip is not very descriptive anyway right now. Fair, I could mention the occlusion behavior in the tooltip of the checkbox.
Jorijn de Graaf added 2 commits 2023-03-29 16:59:07 +02:00
f543885071 Use continue for depsgraph iterator 
This does the same thing, but may be a bit easier to read in terms of which comment is about which piece of code.
24038c4938 Remove unnecessary RETOPOLOGY_OCCLUSION macro 
It's only used in two places and Germano didn't like it, so I'll just get rid of it.
Jorijn de Graaf added 1 commit 2023-03-29 22:45:18 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
273c4c6991 Improve retopology overlay tooltip 
As Julien mentioned, it was a pretty useless tooltip before.
It also mentions selection occlusion now.
Jorijn de Graaf requested review from Julien Kaspar 2023-03-30 14:38:08 +02:00
Ray molenkamp commented 2023-03-30 19:57:52 +02:00
Member
Copy Link

@blender-bot package

@blender-bot package
Blender Bot commented 2023-03-30 19:57:55 +02:00
Member
Copy Link

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105498) when ready.
Jorijn de Graaf added 1 commit 2023-03-30 23:59:35 +02:00
Julien Kaspar approved these changes 2023-04-02 11:51:24 +02:00
Julien Kaspar left a comment
Member
Copy Link

@bonj I've tested the latest builds and yes the selection is way more consistent with the visible mesh. There's still a small difference between the three selection modes but that's to be expected 👍

I see your point with the other notes. Let's keep it how it is.
I'll keep an eye out what people say once it's merged with main, especially about the selection occlusion when set to 0.

@bonj I've tested the latest builds and yes the selection is way more consistent with the visible mesh. There's still a small difference between the three selection modes but that's to be expected 👍 I see your point with the other notes. Let's keep it how it is. I'll keep an eye out what people say once it's merged with main, especially about the selection occlusion when set to 0.
Jorijn de Graaf commented 2023-04-02 14:09:04 +02:00
Author
Contributor
Copy Link

I'll keep an eye out what people say once it's merged with main, especially about the selection occlusion when set to 0.

It might be a good idea to make the selection occlusion optional, but I would do that with a new checkbox, rather than making it part of the slider behavior.
For now I don't want to add that because this PR has been open for long enough.
I agree let's wait for feedback after merge and go from there.

> I'll keep an eye out what people say once it's merged with main, especially about the selection occlusion when set to 0. It might be a good idea to make the selection occlusion optional, but I would do that with a new checkbox, rather than making it part of the slider behavior. For now I don't want to add that because this PR has been open for long enough. I agree let's wait for feedback after merge and go from there.
Jeroen Bakker reviewed 2023-04-03 09:04:43 +02:00
source/blender/draw/intern/draw_manager.c Outdated
@ -2774,6 +2774,25 @@ void DRW_draw_select_id(Depsgraph *depsgraph, ARegion *region, View3D *v3d, cons
drw_engines_cache_populate(obj_eval);
}
if (RETOPOLOGY_ENABLED(v3d) && !XRAY_ENABLED(v3d)) {
Jeroen Bakker commented 2023-04-03 09:04:43 +02:00
Member
Copy Link

Draw manager should be as functionality agnostic as much as possible. Adding this if-statement might be ok as we also check if bone overlay is enabled. But the if statements inside the iterator should be done by the draw engines.

Selection engine is currently being redesigned. I think @fclem @pragma37 can do a better review of this part keeping in mind the new design.

Draw manager should be as functionality agnostic as much as possible. Adding this if-statement might be ok as we also check if bone overlay is enabled. But the if statements inside the iterator should be done by the draw engines. Selection engine is currently being redesigned. I think @fclem @pragma37 can do a better review of this part keeping in mind the new design.
Jeroen Bakker requested review from Miguel Pozo 2023-04-03 09:09:11 +02:00
Jeroen Bakker refused to review 2023-04-03 09:09:15 +02:00
Jorijn de Graaf commented 2023-04-03 14:58:41 +02:00
Author
Contributor
Copy Link

@Jeroen-Bakker reply to your review.

Draw manager should be as functionality agnostic as much as possible. Adding this if-statement might be ok as we also check if bone overlay is enabled.

This one I could move if necessary, though it would be inefficient.

But the if statements inside the iterator should be done by the draw engines.

I could probably move the object type check, if that doesn't break the edit mode check.
I can't move the edit mode check, because then edit mode objects, which are also in the selection context object list, would be cached twice.

Another solution would be to get rid of the edit mode check, but not iterate over the selection context objects list at all.
That way the order would not be guaranteed though, and edit mode objects that are not in the list would also get cached.

The reason the code is the way it is, is because it needs edit mode objects from the list, then object mode objects, preferably in that order.
I suppose if the engine is changed to work differently, this aspect can change too.

Selection engine is currently being redesigned.

I was hoping this feature could make it in before that.

@Jeroen-Bakker reply to your review. > Draw manager should be as functionality agnostic as much as possible. Adding this if-statement might be ok as we also check if bone overlay is enabled. This one I could move if necessary, though it would be inefficient. > But the if statements inside the iterator should be done by the draw engines. I could probably move the object type check, if that doesn't break the edit mode check. I can't move the edit mode check, because then edit mode objects, which are also in the selection context object list, would be cached twice. Another solution would be to get rid of the edit mode check, but not iterate over the selection context objects list at all. That way the order would not be guaranteed though, and edit mode objects that are not in the list would also get cached. The reason the code is the way it is, is because it needs edit mode objects from the list, then object mode objects, preferably in that order. I suppose if the engine is changed to work differently, this aspect can change too. > Selection engine is currently being redesigned. I was hoping this feature could make it in before that.
Clément Foucault approved these changes 2023-04-08 19:05:40 +02:00
Clément Foucault left a comment
Member
Copy Link

I just see a small simple cleanup to do. Otherwise looks good to me.

I just see a small simple cleanup to do. Otherwise looks good to me.
source/blender/draw/engines/select/select_engine.c Outdated
@ -144,0 +150,4 @@
if (retopology_occlusion) {
pd->shgrp_occlude = DRW_shgroup_create(sh->select_id_uniform, psl->depth_only_pass);
DRW_shgroup_uniform_int_copy(pd->shgrp_occlude, "id", 0);
Clément Foucault commented 2023-04-08 18:55:44 +02:00
Member
Copy Link

This feels weird to me. Why is this group using id == 0 when shgrp_depth_only doesn't? I think it is better to be consistent and set it for both cases. Also I wouldn't mind adding a comment saying this id isn't actually used since the pass is written on a depth only framebuffer.

This feels weird to me. Why is this group using `id == 0` when `shgrp_depth_only` doesn't? I think it is better to be consistent and set it for both cases. Also I wouldn't mind adding a comment saying this id isn't actually used since the pass is written on a depth only framebuffer.
fclem marked this conversation as resolved
source/blender/draw/intern/draw_manager.c Outdated
@ -2774,6 +2774,25 @@ void DRW_draw_select_id(Depsgraph *depsgraph, ARegion *region, View3D *v3d, cons
drw_engines_cache_populate(obj_eval);
}
if (RETOPOLOGY_ENABLED(v3d) && !XRAY_ENABLED(v3d)) {
Clément Foucault commented 2023-04-08 19:04:12 +02:00
Member
Copy Link

Well this is extending the already existing edit mode selection path. I really don't see the problem as other part of the draw-manager are state aware of the View3D. Obviously this will need to be fitted to the new selection engine but that's not a project for the 3.6 release.

Also note that #102177 is only targeting object selection mode.

Well this is extending the already existing edit mode selection path. I really don't see the problem as other part of the draw-manager are state aware of the View3D. Obviously this will need to be fitted to the new selection engine but that's not a project for the 3.6 release. Also note that #102177 is only targeting object selection mode.
fclem marked this conversation as resolved
Jorijn de Graaf added 1 commit 2023-04-08 21:15:15 +02:00
981294301b Remove unused "id" uniform from shgrp_occlude 
This is consistent with how unused uniforms are used elsewhere (for example "sizeVertex").
Alternatively I would add the uniform for the depth only group too, and then add a comment for both explaining that it's unused, but I think that this is cleaner.
Jorijn de Graaf added 1 commit 2023-04-08 21:27:38 +02:00
Clément Foucault added 1 commit 2023-04-09 07:42:53 +02:00
Clément Foucault changed title from Selection Occlusion to DRW: Selection Occlusion 2023-04-09 08:07:34 +02:00
Clément Foucault merged commit 7a267aa000 into main 2023-04-09 08:08:13 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Germano Cavalcante
Julien Kaspar
Miguel Pozo
Clément Foucault
Labels
Clear labels   
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
This issue affects/is about backward or forward 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
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
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
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
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 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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Modeling
Assignees
Clear assignees
No Assignees
8 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#105498
No description provided.
Delete Branch "bonj/blender:retopology-select"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?