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
Contributor

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.
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
Member

@blender-bot build

@blender-bot build

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

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

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

@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 🙂
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/PR105498) when ready.
Jorijn de Graaf force-pushed retopology-select from ef85581bdf to 38293cd17c 2023-03-25 16:18:28 +01:00 Compare
Author
Contributor

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.
Member

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

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

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

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

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.
@ -2780,0 +2786,4 @@
}
}
DEG_OBJECT_ITER_END;

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

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

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

LGTM in general.

LGTM in general.
@ -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)) {

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
@ -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))

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

@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

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

@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
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/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

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

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
@ -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)) {
Member

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

@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

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.
@ -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);

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
@ -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)) {

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.
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
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 project
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.