UI: Update default mask icon #117467

Sean Kim · 2024-01-24T03:34:37+01:00

Sean Kim commented

2024-01-24 03:34:37 +01:00

Based on the design of #102585.
This PR replaces the MOD_MASK icon with the CLIP_UVDEHLT icon to better communicate the intent of masking across many areas of Blender. It also changes the Grease Pencil invert icons to avoid conflicts with the CLIPUV_DEHLT & CLIPUV_HLT now being used to indicate mask usage for Grease Pencil layers.

Changes

Automasking

Top Bar

Grease Pencil

Image Edit

Modifier

Addresses part of #102585

Based on the design of #102585. This PR replaces the MOD_MASK icon with the CLIP_UVDEHLT icon to better communicate the intent of masking across many areas of Blender. It also changes the Grease Pencil `invert` icons to avoid conflicts with the CLIPUV_DEHLT & CLIPUV_HLT now being used to indicate mask usage for Grease Pencil layers. ## Changes ### Automasking ![automasking_before.png](/attachments/7db971cd-798c-4eae-ac28-a34d3190495e) ![automasking_after.png](/attachments/501cf2db-6fe8-42e4-a9c6-2c6e53a1f043) ### Top Bar ![top_bar_before.png](/attachments/d04afec3-b132-47ce-a79c-d4d6bc2f1e13) ![top_bar_after.png](/attachments/77e10b8b-2794-4995-aaed-52f849787655) ### Grease Pencil ![grease_pencil_before.png](/attachments/f4c48c33-0c96-4d1e-b39b-d2ad4108ec40) ![top_layer_panel.gif](/attachments/be06d51d-0d52-4c7b-b5b8-240abda95279) ![data_block_panel.gif](/attachments/082bcdaf-126b-4a8c-8dea-e73fff68f06d) ### Image Edit ![image_edit_before.png](/attachments/5582a304-f1fc-493b-ba2c-05f35808bc03) ![image_edit_after.png](/attachments/8c2f01f1-c1d0-400f-9ca5-cdcc1fe7e090) ### Modifier ![modifier_before.png](/attachments/7fe21b47-098e-430d-a410-688ca8f038ca) ![modifier_after.png](/attachments/ee51e50e-bfb9-4368-b40c-6b373f625153) Addresses part of #102585

data_block_after.png

69 KiB

image_edit_before.png

59 KiB

modifier_before.png

415 KiB

data_block_panel.gif

251 KiB

automasking_after.png

22 KiB

top_layer_panel.gif

932 KiB

automasking_before.png

16 KiB

grease_pencil_before.png

231 KiB

modifier_after.png

436 KiB

data_block_before.png

67 KiB

image_edit_after.png

68 KiB

Sean Kim requested review from Julien Kaspar 2024-01-24 03:35:48 +01:00

Sean Kim requested review from Harley Acheson 2024-01-24 03:36:06 +01:00

Julien Kaspar commented

2024-01-24 11:02:26 +01:00

So far this looks good to me.
The only unfortunate thing is that the new mask modifier icon has more weight when compared to all other modifier icons. It sticks out and breaks visual cohesion a bit.
But IMO it's a minor issue.

This PR could also get the other changes for completion of #102585.

So far this looks good to me. The only unfortunate thing is that the new mask modifier icon has more weight when compared to all other modifier icons. It sticks out and breaks visual cohesion a bit. But IMO it's a minor issue. This PR could also get the other changes for completion of #102585.

Gangneron referenced this pull request

2024-01-24 15:30:55 +01:00

Improved Mask icon with disabled state #102585

Sean Kim commented

2024-01-24 23:08:41 +01:00

So far this looks good to me.
The only unfortunate thing is that the new mask modifier icon has more weight when compared to all other modifier icons. It sticks out and breaks visual cohesion a bit.
But IMO it's a minor issue.

This PR could also get the other changes for completion of #102585.

@JulienKaspar I decided to split this work and the work to get the toggleable state for the Auto-Masking icon into separate PRs since they seemed to be different enough issues and easier to review separately - if you think it's more beneficial for them to be shipped together then I can plan on including it here instead.

> So far this looks good to me. > The only unfortunate thing is that the new mask modifier icon has more weight when compared to all other modifier icons. It sticks out and breaks visual cohesion a bit. > But IMO it's a minor issue. > > This PR could also get the other changes for completion of #102585. @JulienKaspar I decided to split this work and the work to get the toggleable state for the Auto-Masking icon into separate PRs since they seemed to be different enough issues and easier to review separately - if you think it's more beneficial for them to be shipped together then I can plan on including it here instead.

Julien Kaspar requested changes 2024-01-25 10:28:24 +01:00

Julien Kaspar left a comment

I'm fine with it being separated into two PRs.
But it's good to keep in mind that having a mask icon with an on & off state is the primary reason behind updating the icon.

In that case there are still a couple things missing in this PR:

Use both CLIPUV_DEHLT and CLIPUV_HLT for the layers.use_mask_layer toggle.
Use the select_intersect and select_subtract icons for the layers.mask_layers.invert toggle

I'm fine with it being separated into two PRs. But it's good to keep in mind that having a mask icon with an on & off state is the primary reason behind updating the icon. In that case there are still a couple things missing in this PR: - Use both `CLIPUV_DEHLT` and `CLIPUV_HLT` for the `layers.use_mask_layer` toggle. - Use the `select_intersect` and `select_subtract` icons for the `layers.mask_layers.invert` toggle

Sean Kim changed title from ~~UI: Replace MOD_MASK icon with CLIP_UVDEHLT~~ to UI: Update default mask icon

2024-01-29 22:24:40 +01:00

Sean Kim commented

2024-01-29 22:35:58 +01:00

Updated the PR with the changes to the Grease Pencil layers & mask invert toggle. Added a few gifs of the toggle into the description.

As a note, the icons being used to match the visual representation in the original task are SELECT_INTERSECT and SELECT_DIFFFERENCE

Updated the PR with the changes to the Grease Pencil layers & mask invert toggle. Added a few gifs of the toggle into the description. As a note, the icons being used to match the visual representation in the original task are `SELECT_INTERSECT` and `SELECT_DIFFFERENCE`

Sean Kim requested review from Julien Kaspar 2024-01-29 22:36:20 +01:00

Julien Kaspar approved these changes 2024-01-30 10:16:47 +01:00

Julien Kaspar left a comment

To me this PR is complete and looks great!
Especially for the grease pencil masks and layers it fits in very well and communicates the functionality much better than the original.

To me this PR is complete and looks great! Especially for the grease pencil masks and layers it fits in very well and communicates the functionality much better than the original.

Pablo Vazquez commented

2024-01-30 11:51:59 +01:00

Thanks for working on it!

Looking at the code it seems the mask icon is still called CLIPUV_DEHLT, shouldn't that be updated as part of this PR?

Thanks for working on it! Looking at the code it seems the mask icon is still called `CLIPUV_DEHLT`, shouldn't that be updated as part of this PR?

Pablo Vazquez referenced this pull request

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

UI: Add visual toggle for global automasking #117649

Sean Kim commented

2024-01-30 22:51:25 +01:00

Thanks for working on it!

Looking at the code it seems the mask icon is still called CLIPUV_DEHLT, shouldn't that be updated as part of this PR?

@pablovazquez , Had a brief talk with @Harley in the ui module chat a week or so ago about this and he recommended keeping the icon names static to avoid disruption to scripters:

just to be clear, this would involve copying the image for CLIPUV_DEHLT over the image for MOD_MASK, but not changing any names, which can be disruptive to scripters. In areas where we need enabled and disabled masks we'd just use CLIPUV_DEHLT and CLIPUV_HLT even though their names aren't quite right.

> Thanks for working on it! > > Looking at the code it seems the mask icon is still called `CLIPUV_DEHLT`, shouldn't that be updated as part of this PR? @pablovazquez , Had a brief talk with @Harley in the ui module chat a week or so ago about this and he recommended keeping the icon names static to avoid disruption to scripters: > just to be clear, this would involve copying the image for CLIPUV_DEHLT over the image for MOD_MASK, but not changing any names, which can be disruptive to scripters. In areas where we need enabled and disabled masks we'd just use CLIPUV_DEHLT and CLIPUV_HLT even though their names aren't quite right.

Sean Kim referenced this pull request

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

UI: Add visual toggle for global automasking #117649

Pablo Vazquez commented

2024-01-31 00:41:44 +01:00

I see. So is it just changing which icons are used internally?

There seem to be many (unintended?) changes:

release/datafiles/alert_icons.png is wider even
explode icon .dat ?

Is the change to blender_icons.svg even needed then?

I see. So is it just changing which icons are used internally? There seem to be many (unintended?) changes: * `release/datafiles/alert_icons.png` is wider even * explode icon .dat ? Is the change to `blender_icons.svg` even needed then?

Sean Kim commented

2024-01-31 01:05:58 +01:00

I see. So is it just changing which icons are used internally?

There seem to be many (unintended?) changes:

release/datafiles/alert_icons.png is wider even

explode icon .dat ?

Is the change to blender_icons.svg even needed then?

An alternative could be to just change any usage of ICON_MOD_MASK to ICON_CLIPUV_HLT instead of changing the blender_icons.svg, though I assume the reasoning is to ensure the icon is consistent for future usage and to reduce confusion on naming for contributors - @JulienKaspar would have to weigh in more on that.

As for the unintended changes, that's something I didn't notice with my PR; I'll investigate this more but I'm currently clueless as to why they happened. Thanks for pointing them out.

> I see. So is it just changing which icons are used internally? > > There seem to be many (unintended?) changes: > * `release/datafiles/alert_icons.png` is wider even > * explode icon .dat ? > > Is the change to `blender_icons.svg` even needed then? An alternative could be to just change any usage of `ICON_MOD_MASK` to `ICON_CLIPUV_HLT` instead of changing the blender_icons.svg, though I assume the reasoning is to ensure the icon is consistent for future usage and to reduce confusion on naming for contributors - @JulienKaspar would have to weigh in more on that. As for the unintended changes, that's something I didn't notice with my PR; I'll investigate this more but I'm currently clueless as to why they happened. Thanks for pointing them out.

Julien Kaspar commented

2024-01-31 11:18:47 +01:00

@pablovazquez @Harley I would be fine with either way. I think you should decide on this.
Keeping the icon names for consistency on addons is good.
But I'd also be fine with renaming icons like ICON_CLIPUV_HLT to ICON_MASK_HLT for clarity.

@pablovazquez @Harley I would be fine with either way. I think you should decide on this. Keeping the icon names for consistency on addons is good. But I'd also be fine with renaming icons like `ICON_CLIPUV_HLT` to `ICON_MASK_HLT` for clarity.

Sean Kim commented

2024-01-31 21:20:36 +01:00

Updated to fix the unintended changes, I had to manually revert the icon32_mod_explode.dat and prvicons.png - still unsure at the moment as to why they change on my laptop when running make icons, but I put that into a separate issue for now (#117683).

Updated to fix the unintended changes, I had to manually revert the `icon32_mod_explode.dat` and `prvicons.png` - still unsure at the moment as to why they change on my laptop when running `make icons`, but I put that into a separate issue for now (#117683).

Pablo Vazquez commented

2024-02-01 12:04:35 +01:00

Thanks for the update!

It's good to go now, but I would like a confirmation from the Grease Pencil team that this icon change is ok with them. cc: @mendio

Thanks for the update! It's good to go now, but I would like a confirmation from the Grease Pencil team that this icon change is ok with them. cc: @mendio

Matias Mendiola commented

2024-02-01 15:18:25 +01:00

Thanks for the update!

It's good to go now, but I would like a confirmation from the Grease Pencil team that this icon change is ok with them. cc: @mendio

looks great, from our side is ok to go ahead with the icon change

> Thanks for the update! > > It's good to go now, but I would like a confirmation from the Grease Pencil team that this icon change is ok with them. cc: @mendio looks great, from our side is ok to go ahead with the icon change

👍 1

Pablo Vazquez approved these changes 2024-02-01 15:41:29 +01:00

Pablo Vazquez left a comment

Great! Design-wise this is ready.

@Harley do you see anything in the code that needs changing or would it be good to go?

Great! Design-wise this is ready. @Harley do you see anything in the code that needs changing or would it be good to go?

Harley Acheson commented

2024-02-01 16:48:19 +01:00

@pablovazquez - do you see anything in the code that needs changing or would it be good to go?

No, the code changes are fine. And glad we are not renaming any icons. We have so many with bad names but it is a burden on addon makers if the names change.

However....

I would like to be the person who merges this so I can make sure there are no versioning issues with the SVG with recent changes and some pending changes. In fact I might do a separate merge of all the icon changes together so there are less incremental changes to that file.

I'd be doing this today.

> @pablovazquez - do you see anything in the code that needs changing or would it be good to go? No, the code changes are fine. And glad we are not renaming any icons. We have so many with bad names but it is a burden on addon makers if the names change. However.... **I would like to be the person who merges this** so I can make sure there are no versioning issues with the SVG with recent changes and some pending changes. In fact I might do a separate merge of all the icon changes together so there are less incremental changes to that file. I'd be doing this today.

👍 1

Pablo Vazquez commented

2024-02-01 17:12:27 +01:00