UI: Add visual toggle for global automasking #117649

Sean Kim · 2024-01-29T22:56:05+01:00

Sean Kim commented

2024-01-29 22:56:05 +01:00

This pull request adds a visual toggle for sculpt automasking in both normal and Grease Pencil modes.

If no automasking settings are applied, then the icon is CLIPUV_DEHLT, otherwise it is CLIPUV_HLT

Changes

Testing

Manual testing of changes in both modes

Addresses #102585

This pull request adds a visual toggle for sculpt automasking in both normal and Grease Pencil modes. If no automasking settings are applied, then the icon is CLIPUV_DEHLT, otherwise it is CLIPUV_HLT ### Changes ![sculpt_automasking_toggle.gif](/attachments/df847d79-fe3b-4ba9-b94a-f13b50b388ea) ![gpsculpt_automasking_toggle.gif](/attachments/50336fc5-7536-4156-b04a-7d92a738b242) ### Testing * Manual testing of changes in both modes Addresses #102585

sculpt_automasking_toggle.gif

114 KiB

gpsculpt_automasking_toggle.gif

353 KiB

❤️ 2

Sean Kim added 2 commits 2024-01-29 22:56:14 +01:00

f211f70278 UI: Add automasking icon toggling for sculpt mode

Applies changes to the icons for both 3D sculpting and Grease Pencil sculpting.

Ref \#102585

8b8e61f43e Merge branch 'main' of projects.blender.org:blender/blender into 102585-automask-toggle-icon

Sean Kim requested review from Sergey Sharybin 2024-01-29 22:56:38 +01:00

Sean Kim requested review from Hans Goudey 2024-01-29 22:56:54 +01:00

Sean Kim requested review from Julien Kaspar 2024-01-29 22:57:06 +01:00

Hans Goudey commented

2024-01-30 04:07:10 +01:00

I'm not sure about the switch of the mask icon here. Personally I find this version not as visually pleasing. I know that's being discussed elsewhere though.

Sergey Sharybin requested review from Pablo Vazquez 2024-01-30 09:40:16 +01:00

Sergey Sharybin commented

2024-01-30 09:43:16 +01:00

The icon itself is more for the UI module to review.

From the implementation side, the icon selection should not be a property, but a function in Python:

def _automasking_icon_get(context):
    # implementaiton

layout.popover(
            panel="VIEW3D_PT_sculpt_automasking",
            text="",
            icon_value=_automasking_icon_get()
        )

The UI is not a part of Python API (except of cases like collapsed state of a box, for which there is no better solution within the Blender design).

The icon itself is more for the UI module to review. From the implementation side, the icon selection should not be a property, but a function in Python: ``` def _automasking_icon_get(context): # implementaiton layout.popover( panel="VIEW3D_PT_sculpt_automasking", text="", icon_value=_automasking_icon_get() ) ``` The UI is not a part of Python API (except of cases like collapsed state of a box, for which there is no better solution within the Blender design).

Julien Kaspar approved these changes 2024-01-30 10:08:17 +01:00

Julien Kaspar left a comment

On the user side this works as expected! Waiting for technical approval then.

If there are still concerns about the icon choice we could discuss this in the chat or on the design task?

On the user side this works as expected! Waiting for technical approval then. If there are still concerns about the icon choice we could discuss this in the chat or on the design task?

Pablo Vazquez commented

2024-01-30 11:52:53 +01:00

Functionality wise is great to have, thanks! It should use proper icon names though (MASK, not CLIPUV), or would that happen after #117467 lands?

Functionality wise is great to have, thanks! It should use proper icon names though (`MASK`, not `CLIPUV`), or would that happen after #117467 lands?

Sean Kim commented

2024-01-30 22:56:56 +01:00

From the implementation side, the icon selection should not be a property, but a function in Python:

@Sergey - Makes sense on avoiding adding this into the python API, I recalled seeing it elsewhere in the code to achieve a similar effect, but keeping it entirely in UI code makes sense.

Functionality wise is great to have, thanks! It should use proper icon names though (MASK, not CLIPUV), or would that happen after #117467 lands?

@pablovazquez - Replied to this in the other PR (#117467), apologies for the split discussion happening across these two - I think the naming of the icons is best addressed there.

> From the implementation side, the icon selection should not be a property, but a function in Python: @Sergey - Makes sense on avoiding adding this into the python API, I recalled seeing it elsewhere in the code to achieve a similar effect, but keeping it entirely in UI code makes sense. > Functionality wise is great to have, thanks! It should use proper icon names though (`MASK`, not `CLIPUV`), or would that happen after #117467 lands? @pablovazquez - Replied to this in the other PR (#117467), apologies for the split discussion happening across these two - I think the naming of the icons is best addressed there.

Sean Kim added 3 commits 2024-01-31 22:43:13 +01:00

c620db0717 WIP

50e1defad6 Remove automasking RNA properties in favor of python code

187d522cc7 Merge branch 'main' of projects.blender.org:blender/blender into 102585-automask-toggle-icon

Pablo Vazquez approved these changes 2024-02-01 12:07:18 +01:00

Pablo Vazquez left a comment

Thanks! Approving design-wise.

Will leave the code review to the devs.

Thanks! Approving design-wise. Will leave the code review to the devs.

Hans Goudey approved these changes 2024-02-01 20:00:12 +01:00

Hans Goudey left a comment

Looks fine to me. I'm not up to date on which icon we should be using for each case, but I assume others have stronger opinions about that than me anyway.

Pablo Vazquez commented

2024-02-02 14:33:11 +01:00

Looks fine to me. I'm not up to date on which icon we should be using for each case

Thanks! It's been decided to use CLIPUV_HLT/DEHLT, name can be adjusted in the future as larger change but that's outside the scope of this patch.

> Looks fine to me. I'm not up to date on which icon we should be using for each case Thanks! It's been decided to use `CLIPUV_HLT/DEHLT`, name can be adjusted in the future as larger change but that's outside the scope of this patch.

Pablo Vazquez merged commit c15d1b8ccb into main

2024-02-02 14:34:56 +01:00

Pablo Vazquez referenced this issue from a commit

2024-02-02 14:34:56 +01:00

UI: Add visual indicator for automasking state

Pablo Vazquez deleted branch 102585-automask-toggle-icon

2024-02-02 14:34:59 +01:00

Pablo Vazquez referenced this pull request

2024-02-02 14:37:29 +01:00

Improved Mask icon with disabled state #102585

YimingWu referenced this issue from a commit

2024-02-05 10:56:05 +01:00