Expand: Menu operators need to be updated #104023

New Issue

Julien Kaspar · 2023-01-20T12:29:46+01:00

Julien Kaspar commented

2023-01-20 12:29:46 +01:00

(NOTE) This task is part of the community & sub tasks of the workboard. Anyone is free to pick up this task and contribute. For any questions or needed reviewers, please tag @JosephEagar and @JulienKaspar.

The menu operators for Expand are currently using the wrong arguments and are out of sync with the keymap entries.
These should match so the shortcuts will also show up in the menus:

So the operators need to have these properties:
Expand Mask by Topology: Invert=False, Preserve Previous=True, Auto-Create=False
Expand Mask by Normals: Invert=False, Preserve Previous=True
Expand Face Set by Topology: Invert=False, Preserve Previous=False, Modify Active=False
Expand Active Face Set: Invert=False, Preserve Previous=False, Modify Active=True

(NOTE) This task is part of the community & sub tasks of the workboard. Anyone is free to pick up this task and contribute. For any questions or needed reviewers, please tag @JosephEagar and @JulienKaspar. The menu operators for Expand are currently using the wrong arguments and are out of sync with the keymap entries. These should match so the shortcuts will also show up in the menus: | ![image.png](https://archive.blender.org/developer/F14231041/image.png) | ![image.png](https://archive.blender.org/developer/F14231043/image.png) | -- | -- | So the operators need to have these properties: **Expand Mask by Topology:** Invert=False, Preserve Previous=True, Auto-Create=False **Expand Mask by Normals:** Invert=False, Preserve Previous=True **Expand Face Set by Topology:** Invert=False, Preserve Previous=False, Modify Active=False **Expand Active Face Set:** Invert=False, Preserve Previous=False, Modify Active=True

Julien Kaspar commented

2023-01-20 12:29:46 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Julien Kaspar commented

2023-01-20 12:29:46 +01:00

Added subscribers: @JulienKaspar, @JosephEagar

Patrick Foley commented

2023-01-26 04:33:31 +01:00

Added subscriber: @Patrick-Foley

Patrick Foley commented

2023-01-27 03:24:23 +01:00

I'm looking into this as my first contribution.

From the review comments when these features were added, it seems the desired behavior for both tools is to start the mask black and flood it white, and this is their behavior currently.

Removing the invert=False parameter from "Expand Mask by Normals" lets it fall back on the same default, and has no effect. However, removing invert=True from "Expand Mask by Topology" predictably changes the tool's behavior, starting the mask white and flooding it black.

I take it the desired result of the change should be that the tools continue to operate as before? If so, it seems that some adjustment of the lower-level implementation would be required. I'm not very familiar with the tool's implementation, so I'm not sure exactly where this more hard-coded inversion behavior (when expanding mask by topology) comes from, or the ramifications of changing it to be in line with expanding by normals.

[edit: when the "[...] by Topology" and "[...] by Normals" tools are sent the same invert value (in this case invert=False), their mask-filling behavior is opposite to each other. I thought perhaps I could find a smoking gun for this deeper into the code, so I also checked the filter-cache's invert value for both tools while they were running. But they're both the same! (in this case set to false)]

I'm looking into this as my first contribution. From the [review comments](https://developer.blender.org/D5657#131022) when these features were added, it seems the desired behavior for both tools is to start the mask black and flood it white, and this is their behavior currently. Removing the `invert=False` parameter from "Expand Mask by Normals" lets it fall back on the same default, and has no effect. However, removing `invert=True` from "Expand Mask by Topology" predictably changes the tool's behavior, starting the mask white and flooding it black. I take it the desired result of the change should be that the tools continue to operate as before? If so, it seems that some adjustment of the lower-level implementation would be required. I'm not very familiar with the tool's implementation, so I'm not sure exactly where this more hard-coded inversion behavior (when expanding mask by topology) comes from, or the ramifications of changing it to be in line with expanding by normals. [edit: when the "[...] by Topology" and "[...] by Normals" tools are sent the same `invert` value (in this case `invert=False`), their mask-filling behavior is opposite to each other. I thought perhaps I could find a smoking gun for this deeper into the code, so I also checked the filter-cache's `invert` value for both tools while they were running. But they're both the same! (in this case set to `false`)]

Julien Kaspar commented

2023-01-27 13:04:00 +01:00

@Patrick-Foley
I'm sorry for the not being clear enough on the current state of the Expand operators.
@JosephEagar is current fixing some issues at the moment. I'd suggest to wait a bit until he made the needed fixes first.

But to clarify: The desired result is that when using these Mask Expand operators:

A black mask is expanded from the clicked on position
If inverted with F a white unmasked area will be expanded
The operator does not clear the rest of the mesh island, unless "Preserve Mask" is toggled with E

The operator to expand by Normals still needs a fix to not fill the mesh island and to have the inverted effect by default.

This task is specifically about making sure that the menu operators do not override the default behavior.

@Patrick-Foley I'm sorry for the not being clear enough on the current state of the Expand operators. @JosephEagar is current fixing some issues at the moment. I'd suggest to wait a bit until he made the needed fixes first. But to clarify: The desired result is that when using these Mask Expand operators: - A black mask is expanded from the clicked on position - If inverted with `F` a white unmasked area will be expanded - The operator does not clear the rest of the mesh island, unless "Preserve Mask" is toggled with `E` The operator to expand by Normals still needs a fix to not fill the mesh island and to have the inverted effect by default. This task is specifically about making sure that the menu operators do not override the default behavior.

Julien Kaspar commented

2023-02-02 10:59:45 +01:00

@Patrick-Foley The issues I mentioned are now resolved.
The best way to resolve this task properly is to match the menu operators with the same properties of the keymap entries. This way the shortcuts will also show up in the menus:

So the operators need to have these properties:
Expand Mask by Topology: Invert=False, Preserve Previous=True, Auto-Create=False
Expand Mask by Normals: Invert=False, Preserve Previous=True
Expand Face Set by Topology: Invert=False, Preserve Previous=False, Modify Active=False
Expand Active Face Set: Invert=False, Preserve Previous=False, Modify Active=True

I'll update the task description.

@Patrick-Foley The issues I mentioned are now resolved. The best way to resolve this task properly is to match the menu operators with the same properties of the keymap entries. This way the shortcuts will also show up in the menus: | ![image.png](https://archive.blender.org/developer/F14231041/image.png) | ![image.png](https://archive.blender.org/developer/F14231043/image.png) | -- | -- | So the operators need to have these properties: **Expand Mask by Topology:** Invert=False, Preserve Previous=True, Auto-Create=False **Expand Mask by Normals:** Invert=False, Preserve Previous=True **Expand Face Set by Topology:** Invert=False, Preserve Previous=False, Modify Active=False **Expand Active Face Set:** Invert=False, Preserve Previous=False, Modify Active=True I'll update the task description.

Julien Kaspar changed title from ~~Mask Expand: Menu operators should not set invert state~~ to Expand: Menu operators need to be updated

2023-02-02 11:25:02 +01:00

Patrick Foley self-assigned this 2023-02-03 02:06:08 +01:00

Patrick Foley commented

2023-02-03 02:33:26 +01:00

Alright, diff file set up here:

https://developer.blender.org/D17191

Should make for a simple enough review.
@JulienKaspar

Alright, diff file set up here: https://developer.blender.org/D17191 Should make for a simple enough review. @JulienKaspar

Julien Kaspar added this to the Sculpt, Paint & Texture project 2023-02-08 10:48:52 +01:00

Julien Kaspar commented

2023-02-09 16:36:43 +01:00

@Patrick-Foley Sorry that the review happened so close to the migration to the new site. Could you reupload the patch so we can finish the review?

Patrick Foley commented

2023-02-10 07:09:23 +01:00

@JulienKaspar

I'm having some troubles setting the new workflow up (can't seem to verify my public key), but if and when I get that sorted I'll be happy to get a push request in and link it here.

If you'd like me to literally upload the diff file here I can do that as well, just let me know.

@JulienKaspar I'm having some troubles setting the new workflow up (can't seem to verify my public key), but if and when I get that sorted I'll be happy to get a push request in and link it here. If you'd like me to literally upload the diff file here I can do that as well, just let me know.

Philipp Oeser removed the

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Expand: Menu operators need to be updated #104023