UI: Gray out Scene Auto-Masking option if equivalent Brush option is used #102971 #106126

Henry-Chang · 2023-03-25T12:34:38+01:00

Henry-Chang commented

2023-03-25 12:34:38 +01:00

First-time contributor

Completed Part:
Graying out Scene Auto-Masking if equivalent Brush option is straightforward.

The Following Matching Criteria is applied (Scene option to Brush option that would cause Gray out):
Topology to Topology
Face Sets to Face Sets

Mesh Boundary to Mesh Boundary
Face Sets Boundary to Face Sets Boundary
Propagation Steps to both Mesh Boundary and Face Sets Boundary

Cavity, Cavity (Inverted), Create Mask, Factor, Blur, Custom Curve to either Cavity or Cavity (Inverted)

View Normal, Occlusion, Limit, Falloff to View Normal

Area Normal, Limit, Falloff to Area Normal

WIP Part:
Showing Disabled Message was less straightforward. The current implementation does indeed work as shown in the attached picture, but relies on hard-coding and is hacky. Pointers that would lead to a more elegant implementation would be appreciated.

There were two original methods to display Disabled messages:
1, Poll Function return message from operators. However, most of the automasking buttons are properties, not operators, and even for the one operator, the poll method would not fail, as there is nothing preventing its successful completion.
2, but->disabled_info. However, there isn't an obvious way to set this disabled_info from Panel class.

Thank you.

Hi, @JosephEagar and @JulienKaspar. Completed Part: Graying out Scene Auto-Masking if equivalent Brush option is straightforward. The Following Matching Criteria is applied (Scene option to Brush option that would cause Gray out): Topology to Topology Face Sets to Face Sets Mesh Boundary to Mesh Boundary Face Sets Boundary to Face Sets Boundary Propagation Steps to both Mesh Boundary and Face Sets Boundary Cavity, Cavity (Inverted), Create Mask, Factor, Blur, Custom Curve to either Cavity or Cavity (Inverted) View Normal, Occlusion, Limit, Falloff to View Normal Area Normal, Limit, Falloff to Area Normal WIP Part: Showing Disabled Message was less straightforward. The current implementation does indeed work as shown in the attached picture, but relies on hard-coding and is hacky. Pointers that would lead to a more elegant implementation would be appreciated. There were two original methods to display Disabled messages: 1, Poll Function return message from operators. However, most of the automasking buttons are properties, not operators, and even for the one operator, the poll method would not fail, as there is nothing preventing its successful completion. 2, but->disabled_info. However, there isn't an obvious way to set this disabled_info from Panel class. Thank you.

Captura de pantalla 2023-03-25 a las 2.39.00 p. m..png

2.6 MiB

Henry-Chang added 2 commits 2023-03-25 12:34:49 +01:00

UI: Gray out Scene Auto-Masking if equivalent Brush option 8d1bf196ca

WIP: Hardcoded Disabled Message for Gray out Scene Auto-masking 9eb9249468

Henry-Chang referenced this pull request

2023-03-25 12:35:25 +01:00

Wrong branch. Updated PR at #106126. #106123

Julien Kaspar added the

Module

Sculpt, Paint & Texture

label 2023-03-28 19:25:03 +02:00

Julien Kaspar commented

2023-03-28 19:49:33 +02:00

On the surface this looks great! The only issue is that once a toggle is grayed out it cannot be toggled anymore. That should ideally still be possible.

For the technical UI implementation I'd like to get another opinion on this.
Maybe @HooglyBoogly @pablovazquez or @Harley could give some advice?

On the surface this looks great! The only issue is that once a toggle is grayed out it cannot be toggled anymore. That should ideally still be possible. For the technical UI implementation I'd like to get another opinion on this. Maybe @HooglyBoogly @pablovazquez or @Harley could give some advice?

Pablo Vazquez commented

2023-03-28 21:04:34 +02:00

Nice one!

The only issue is that once a toggle is grayed out it cannot be toggled anymore. That should ideally still be possible.

To solve that @Henry-Chang should use .active instead of .enabled

Nice one! > The only issue is that once a toggle is grayed out it cannot be toggled anymore. That should ideally still be possible. To solve that @Henry-Chang should use `.active` instead of `.enabled`

Henry-Chang added 2 commits 2023-03-29 01:05:59 +02:00

UI: Updated Gray out (with Active) Scene Auto-Masking if equivalent Brush option 5083211921

WIP: Updated Hardcoded Disabled Message for Gray out (with Active) Scene Auto-masking f69302fdd7

Henry-Chang commented

2023-03-29 02:09:07 +02:00

First-time contributor

Dear @JulienKaspar, @pablovazquez, thank you for the recommendations. The code is updated as suggested. Thank you.

Harley Acheson commented

2023-03-29 19:02:08 +02:00

@Henry-Chang - Poll Function return message from operators. However, most of the automasking buttons are properties, not operators...

Apologies in advance in this answer is a bit incomplete.

Indeed the way to disable operators is with "poll" functions, and within those we can call CTX_wm_operator_poll_msg_set to add information about why it is disabled.

For properties there is a separate mechanism. Where the properties are defined, for example in this case source\blender\makesrna\intern\rna_sculpt_paint.c, we can add a function that sets the enabled state with RNA_def_property_editable_func.

Most existing uses of that do not set a text description, but here is a great example:

In blender\source\blender\makesrna\intern\rna_space.c you will find a section of code like this:

  prop = RNA_def_property(srna, "filename", PROP_STRING, PROP_FILENAME);
  RNA_def_property_string_sdna(prop, NULL, "file");
  RNA_def_property_ui_text(prop, "File Name", "Active file in the file browser");
  RNA_def_property_editable_func(prop, "rna_FileSelectParams_filename_editable");
  RNA_def_property_update(prop, NC_SPACE | ND_SPACE_FILE_PARAMS, NULL);

If you then look for "rna_FileSelectParams_filename_editable" in that same file you will see something like this:

int rna_FileSelectParams_filename_editable(struct PointerRNA *ptr, const char **r_info)
{
  FileSelectParams *params = ptr->data;

  if (params && (params->flag & FILE_DIRSEL_ONLY)) {
    *r_info = "Only directories can be chosen for the current operation.";
    return 0;
  }

  return params ? PROP_EDITABLE : 0;
}

So returning 0 if disabled, PROP_EDITABLE (1) if not. And giving that text description on why.

A very important thing to mention is that in the above case the "ptr->data" is pointing to data that is something useful in this context: the FileSelectParams. In your case, it would be different but also useful. You'd do Sculpt *sd = (Sculpt *)ptr->data; Note that from there you can get to both the automasking_flags in there and also the ->paint->brush->automasking_flags so easy to compare them.

But... I think you will find that a reason why it might be difficult to do in your particular circumstance is because all these properties you are playing with are defined together in a loop (just search for "rna_enum_brush_automasking_flag_items") since they share the same getters, setters, etc. Fairly certainly you couldn't just define a single "rna_FileSelectParams_filename_editable" for all of them because you might not know which one in particular it is being called for. So you might have to break these up into separate definitions? Or do something special for your needs in a similar way that "BRUSH_AUTOMASKING_CAVITY_NORMAL" gets its own "set" function.

Hopefully the above all sounds interesting and not daunting and discouraging. But we can't really do the "interface_region_tooltip.cc" hack proposed here for the disabled message.

Don't hesitate to ask if you need more information.

> @Henry-Chang - Poll Function return message from operators. However, most of the automasking buttons are properties, not operators... Apologies in advance in this answer is a bit incomplete. Indeed the way to disable *operators* is with "poll" functions, and within those we can call `CTX_wm_operator_poll_msg_set` to add information about why it is disabled. For properties there is a separate mechanism. Where the properties are defined, for example in this case `source\blender\makesrna\intern\rna_sculpt_paint.c`, we can add a function that sets the enabled state with `RNA_def_property_editable_func`. Most existing uses of that do not set a text description, but here is a great example: In `blender\source\blender\makesrna\intern\rna_space.c` you will find a section of code like this: ```C prop = RNA_def_property(srna, "filename", PROP_STRING, PROP_FILENAME); RNA_def_property_string_sdna(prop, NULL, "file"); RNA_def_property_ui_text(prop, "File Name", "Active file in the file browser"); RNA_def_property_editable_func(prop, "rna_FileSelectParams_filename_editable"); RNA_def_property_update(prop, NC_SPACE | ND_SPACE_FILE_PARAMS, NULL); ``` If you then look for "rna_FileSelectParams_filename_editable" in that same file you will see something like this: ```C int rna_FileSelectParams_filename_editable(struct PointerRNA *ptr, const char **r_info) { FileSelectParams *params = ptr->data; if (params && (params->flag & FILE_DIRSEL_ONLY)) { *r_info = "Only directories can be chosen for the current operation."; return 0; } return params ? PROP_EDITABLE : 0; } ``` So returning 0 if disabled, PROP_EDITABLE (1) if not. And giving that text description on why. A very important thing to mention is that in the above case the "ptr->data" is pointing to data that is something useful in this context: the FileSelectParams. In your case, it would be different but also useful. You'd do `Sculpt *sd = (Sculpt *)ptr->data;` Note that from there you can get to both the automasking_flags in there and also the ->paint->brush->automasking_flags so easy to compare them. But... I think you will find that a reason why it might be difficult to do in your particular circumstance is because all these properties you are playing with are defined together in a loop (just search for "rna_enum_brush_automasking_flag_items") since they share the same getters, setters, etc. Fairly certainly you couldn't just define a single "rna_FileSelectParams_filename_editable" for all of them because you might not know which one in particular it is being called for. So you might have to break these up into separate definitions? Or do something special for your needs in a similar way that "BRUSH_AUTOMASKING_CAVITY_NORMAL" gets its own "set" function. Hopefully the above all sounds interesting and not daunting and discouraging. But we can't really do the "interface_region_tooltip.cc" hack proposed here for the disabled message. Don't hesitate to ask if you need more information.

Henry-Chang commented

2023-03-31 16:09:55 +02:00

First-time contributor

Dear @Harley, thank you so much for the detailed reply. No, it does not sound discouraging at all; in fact rather interesting and exciting. I am very glad the hack cannot be accepted, as I am learning more about Blender this way than I would have.

Following along with your post, I was able to quickly set Disabled messages the right way. And it was great.

However, 3 unexpected problems expressed themselves:
1, For the loop part you mentioned, I used an if else check, and a separate function for each check. It doesn't seem ideal but I looked into the StructRNA struct in the PointerRNA struct, and there doesn't seem to be any useful info that could help with this situation.

2, The Scene panel for Automasking at the top and the Brush panel in the side tool menu generate their own UIs. However, there are a few things that are shared between them, namely the Propagation Steps scroll bar, Create Mask button, Limit and Falloff scroll bars of View Normal and Area Normal. By shared between them, I mean they access the same operator or PRNA in the back end. An obvious solution would be to do like what has been done for the situation in 1, and create separate items for Brush and Scene, but I am hoping that wouldn't be necessary.

3, Even if using custom functions, operators, PRNAs for 1 and 2, this is only solving it for the .enabled setting. The .active setting seems to be much more complicated. In the function assigned to prop->editable, if one does not return 0, rna_property_editable_do would return true, which means the but->disabled_info wouldn't be set. I wanted to create an inactive attribute under PropertyRNA, and make a duplicate of RNA_def_property_editable_func, UI_but_disable, rna_property_editable_do, and RNA_property_editable_indo for the inactive attribute. But this seems to entail some non-minor changes to the RNA module, so I wanted to confirm if this makes sense.

Thank you so much.

Dear @Harley, thank you so much for the detailed reply. No, it does not sound discouraging at all; in fact rather interesting and exciting. I am very glad the hack cannot be accepted, as I am learning more about Blender this way than I would have. Following along with your post, I was able to quickly set Disabled messages the right way. And it was great. However, 3 unexpected problems expressed themselves: 1, For the loop part you mentioned, I used an if else check, and a separate function for each check. It doesn't seem ideal but I looked into the StructRNA struct in the PointerRNA struct, and there doesn't seem to be any useful info that could help with this situation. 2, The Scene panel for Automasking at the top and the Brush panel in the side tool menu generate their own UIs. However, there are a few things that are shared between them, namely the Propagation Steps scroll bar, Create Mask button, Limit and Falloff scroll bars of View Normal and Area Normal. By shared between them, I mean they access the same operator or PRNA in the back end. An obvious solution would be to do like what has been done for the situation in 1, and create separate items for Brush and Scene, but I am hoping that wouldn't be necessary. 3, Even if using custom functions, operators, PRNAs for 1 and 2, this is only solving it for the .enabled setting. The .active setting seems to be much more complicated. In the function assigned to prop->editable, if one does not return 0, rna_property_editable_do would return true, which means the but->disabled_info wouldn't be set. I wanted to create an inactive attribute under PropertyRNA, and make a duplicate of RNA_def_property_editable_func, UI_but_disable, rna_property_editable_do, and RNA_property_editable_indo for the inactive attribute. But this seems to entail some non-minor changes to the RNA module, so I wanted to confirm if this makes sense. Thank you so much.

Harley Acheson commented

2023-03-31 17:42:30 +02:00

Again, sorry if I only manage to answer parts of your questions or get some bits wrong. But I'm hoping to help push you along further and perhaps look more closely later.

I used an if else check, and a separate function for each check. It doesn't seem ideal...

No, if it gets too messy you should probably just unroll that "do" loop, defining each Boolean property separately. It might seem like you would be adding complexity but you'd be removing some. And there's only seven so no big deal.

Scene panel...Brush panel...things that are shared between them

I probably don't understand, but if they are sharing the same operators or properties then you don't have the same problem as with automasking_flags. Couldn't you just allow their use in both places as that would be harmless? If you want the two areas to do these things separately and have distinct settings then you'd have to add new fields and copy operators, etc. But feels like a separate issue best deal with separately?

only solving it for the .enabled setting. The .active setting seems to be much more complicated

If I am reading you right then you are wanting to not only not allow those items to be changed but you want to make them show the other value? So if the brush as something turned on you want to show it checked but greyed out? That sounds like you just want to add a custom "getter". That area of code has some uses of RNA_def_property_boolean_funcs that set a custom "setter". You just want to add one that would define something for the second argument of that function. As with the "editable" you would get passed a pointer to the Sculpt object and from there you can get to the Brush to get its settings. You can have the getter function return whatever Boolean value you want.

Again, sorry if I only manage to answer parts of your questions or get some bits wrong. But I'm hoping to help push you along further and perhaps look more closely later. > I used an if else check, and a separate function for each check. It doesn't seem ideal... No, if it gets too messy you should probably just unroll that "do" loop, defining each Boolean property separately. It might seem like you would be adding complexity but you'd be removing some. And there's only seven so no big deal. > Scene panel...Brush panel...things that are shared between them I probably don't understand, but if they are sharing the same operators or properties then you don't have the same problem as with automasking_flags. Couldn't you just allow their use in both places as that would be harmless? If you want the two areas to do these things separately and have distinct settings then you'd have to add new fields and copy operators, etc. But feels like a separate issue best deal with separately? > only solving it for the .enabled setting. The .active setting seems to be much more complicated If I am reading you right then you are wanting to not only not allow those items to be changed but you want to make them show the other value? So if the brush as something turned on you want to show it checked but greyed out? That sounds like you just want to add a custom "getter". That area of code has some uses of `RNA_def_property_boolean_funcs` that set a custom "setter". You just want to add one that would define something for the second argument of that function. As with the "editable" you would get passed a pointer to the Sculpt object and from there you can get to the Brush to get its settings. You can have the getter function return whatever Boolean value you want.

Henry-Chang commented

2023-04-01 04:52:38 +02:00

First-time contributor

Dear @Harley, thank you for your clarification on the 1st issue. Sorry that I wasn't clear about the details, and @JulienKaspar please correct me if I am mistaken.

As I understand it, the task is about graying out the toggle, scroll bar, and button of the Scene Auto-Masking options when the corresponding toggles in the Brush panel is selected.
i.e. If Area Normal toggle is selected in Brush panel, in the Scene Panel, the Area Normal toggle and its corresponding Limit and Falloff scroll bar should be grayed out, but still editable. So, essentially, editable but grayed out. This can be done in the front end, where one can set the .active attribute instead of .enabled attribute in the Python script.

As such, I think the task essentially boils down into two parts.
1, In the Python script: Graying out the corresponding parts of the panel in the Python script but still allow them to be maneuverable. This is achieved by setting the .active attribute and can be done cleanly on the Scene Panel without affecting the Brush Panel.
2, Adding the Disabled message in the backend to items that are grayed out in the Scene Panel (though still editable). This I imagine would be achieved by calling a callback function similar to prop->editable, but only setting the r_info message and not affecting the editable state (still want them to editable). Such a callback function (maybe InactiveFunc inactive instead of EditableFunc editable), which I don't think yet exists, would solve both Problem 2 and 3.

Thank you very much.

Dear @Harley, thank you for your clarification on the 1st issue. Sorry that I wasn't clear about the details, and @JulienKaspar please correct me if I am mistaken. As I understand it, the task is about graying out the toggle, scroll bar, and button of the Scene Auto-Masking options when the corresponding toggles in the Brush panel is selected. i.e. If Area Normal toggle is selected in Brush panel, in the Scene Panel, the Area Normal toggle and its corresponding Limit and Falloff scroll bar should be grayed out, but still editable. So, essentially, editable but grayed out. This can be done in the front end, where one can set the .active attribute instead of .enabled attribute in the Python script. As such, I think the task essentially boils down into two parts. 1, In the Python script: Graying out the corresponding parts of the panel in the Python script but still allow them to be maneuverable. This is achieved by setting the .active attribute and can be done cleanly on the Scene Panel without affecting the Brush Panel. 2, Adding the Disabled message in the backend to items that are grayed out in the Scene Panel (though still editable). This I imagine would be achieved by calling a callback function similar to prop->editable, but only setting the r_info message and not affecting the editable state (still want them to editable). Such a callback function (maybe InactiveFunc inactive instead of EditableFunc editable), which I don't think yet exists, would solve both Problem 2 and 3. Thank you very much.

Harley Acheson commented

2023-04-03 17:52:38 +02:00

Dear @Harley, thank you for your clarification on the 1st issue. Sorry that I wasn't clear about the details...

You are welcome and no worries at all. Note that I was mostly giving you quite general advice and haven't looked much at your issue in particular. I don't sculpt and so haven't really thought through exactly what you are trying to do; I was mostly just telling how our code works.

and @JulienKaspar please correct me if I am mistaken.

Yes, it makes some sense to me that we might want to disable, grey-out, and/or warn users when we have conflicting options like the scene and brush Auto-Masking.

I think it would be unusual though to make options look disabled yet still editable and also give a warning message. But this could be a unique situation and Julien Kaspar is the best person to confirm this for you.

Off the top of my head I can't think of way of showing a "disabled" explanatory message for items that aren't actually disabled. Maybe you could show a message on the panel above those options (space_view3d.py) only while in this condition? So you grey them out by changing active and also show above them "The following items currently have conflicting brush settings" or similar (or better or nicer).

> Dear @Harley, thank you for your clarification on the 1st issue. Sorry that I wasn't clear about the details... You are welcome and no worries at all. Note that I was mostly giving you quite general advice and haven't looked much at your issue in particular. I don't sculpt and so haven't really thought through exactly what you are trying to do; I was mostly just telling how our code works. > and @JulienKaspar please correct me if I am mistaken. Yes, it makes some sense to me that we might want to disable, grey-out, and/or warn users when we have conflicting options like the scene and brush Auto-Masking. I think it would be unusual though to make options look disabled yet still editable and also give a warning message. But this could be a unique situation and Julien Kaspar is the best person to confirm this for you. Off the top of my head I can't think of way of showing a "disabled" explanatory message for items that aren't actually disabled. Maybe you could show a message on the panel above those options (space_view3d.py) only while in this condition? So you grey them out by changing active and also show above them "The following items currently have conflicting brush settings" or similar (or better or nicer).

Julien Kaspar commented

2023-04-05 23:01:15 +02:00

@Henry-Chang Sorry for the late response. I wasn't working over the course of March.

@JulienKaspar please correct me if I am mistaken.

You got it right.

I think it would be unusual though to make options look disabled yet still editable and also give a warning message. But this could be a unique situation and Julien Kaspar is the best person to confirm this for you.

Warning messages in this way are not common but a welcome addition IMO, because it's not clear why the options are grayed out.
Usually it's due to a main toggle being disabled right next to it (like proportional editing in the header) or the context is made clear in the tooltip (grid overlays).
But in this case the cause is somewhere completely different.

An easier alternative could be to just write it plain in the tooltips instead of making it a contextual warning.
But @Henry-Chang if you can spend the time to implement the contextual warning, I'd like to propose this to the UI module as a general addition to the tooltips that can be used elsewhere too.

@Henry-Chang Sorry for the late response. I wasn't working over the course of March. > @JulienKaspar please correct me if I am mistaken. You got it right. > I think it would be unusual though to make options look disabled yet still editable and also give a warning message. But this could be a unique situation and Julien Kaspar is the best person to confirm this for you. Warning messages in this way are not common but a welcome addition IMO, because it's not clear why the options are grayed out. Usually it's due to a main toggle being disabled right next to it (like proportional editing in the header) or the context is made clear in the tooltip (grid overlays). But in this case the cause is somewhere completely different. An easier alternative could be to just write it plain in the tooltips instead of making it a contextual warning. But @Henry-Chang if you can spend the time to implement the contextual warning, I'd like to propose this to the UI module as a general addition to the tooltips that can be used elsewhere too.

Henry-Chang added 6 commits 2023-04-06 11:45:38 +02:00

Revert all changes back to main branch state d0c42cdf63

WIP: Modifications that will allow Disabled messages to be added without setting editable to false 32b5ced13b

UI: Scene Automasking Panel Inactive with Disabled Message except 'Create Mask' Button aa6fa9badd

UI: Scene Automasking 'Create Mask' Button Disabled with Disabled Message a6777367d2

WIP: Separate 'Create Mask' Button and View Normal Limit and Falloff between Scene and Brush Panel 5b9616e1f8

WIP: Extra: Sync Values between Scene View Normal Limit, Falloff and Brush View Normal Limit, Falloff d232b8800a

Henry-Chang commented

2023-04-06 12:17:31 +02:00

First-time contributor

Dear @Harley and @JulienKaspar, thank you for your patience and guidance.

Above is my implementation, which is very close to the requirements, with one difference. The 'Create Mask' button isn't inactive, but actually disabled. The reason for that is that failing poll() function would automatically make the operator not editable. There could be some similar design as the inactive properties implemented above, where some sort of poll() function could enable "Disabled" message but keep the button editable. However, there is only one operator, and the overall difference between being able to click the 'Create Mask' button or not isn't that noticeable; therefore, I did not delve into it here, but it would also be an interesting avenue to explore further if needed.

For the modifications that would allow Disabled message to be set without forcing the button to be non-editable, I tried to make as little changes to the code as possible. However, if it would be more desirable for inactive to have its own workflow instead of borrowing parts of editable for its implementation, please let me know.

'Create Mask' Button and View Normal Limit and Falloff had to be separated between Scene and Brush Panel. Area Normal Limit and Falloff can still rely on the same PropertyRNA in the backend. However, since failing poll() function for 'Create Mask' operator would make the operator non-editable; without a different operator backend, the 'Create Mask' Button in the Brush panel would be grayed out as well. In View Normal Limit and Falloff's case, since turning on Occlusion, would make them inactive in the Brush Panel, if they rely on the same PropertyRNA backend, when they are inactive, they would show the Disabled message, which would not make sense.

For the Extra: Sync Values between View Normal Limit and Falloff between Scene and Brush Panel, I would imagine a callback like RNA_def_property_float_funcs should be used, but I'm not sure whether setting a new set function here would interfere with UI interface's ability to set the float value. It would be nice to be able to get a little guidance on this.

Also, sometimes I get the warning during make: unused function 'rna_Scene_automasking_view_normal_editable' in rna_sculpt_editable.c. It would be nice to not have to see that warning anymore. If there is also other recommendations, please let me know.

Thank you.

Dear @Harley and @JulienKaspar, thank you for your patience and guidance. Above is my implementation, which is very close to the requirements, with one difference. The 'Create Mask' button isn't inactive, but actually disabled. The reason for that is that failing poll() function would automatically make the operator not editable. There could be some similar design as the inactive properties implemented above, where some sort of poll() function could enable "Disabled" message but keep the button editable. However, there is only one operator, and the overall difference between being able to click the 'Create Mask' button or not isn't that noticeable; therefore, I did not delve into it here, but it would also be an interesting avenue to explore further if needed. For the modifications that would allow Disabled message to be set without forcing the button to be non-editable, I tried to make as little changes to the code as possible. However, if it would be more desirable for inactive to have its own workflow instead of borrowing parts of editable for its implementation, please let me know. 'Create Mask' Button and View Normal Limit and Falloff had to be separated between Scene and Brush Panel. Area Normal Limit and Falloff can still rely on the same PropertyRNA in the backend. However, since failing poll() function for 'Create Mask' operator would make the operator non-editable; without a different operator backend, the 'Create Mask' Button in the Brush panel would be grayed out as well. In View Normal Limit and Falloff's case, since turning on Occlusion, would make them inactive in the Brush Panel, if they rely on the same PropertyRNA backend, when they are inactive, they would show the Disabled message, which would not make sense. For the Extra: Sync Values between View Normal Limit and Falloff between Scene and Brush Panel, I would imagine a callback like RNA_def_property_float_funcs should be used, but I'm not sure whether setting a new set function here would interfere with UI interface's ability to set the float value. It would be nice to be able to get a little guidance on this. Also, sometimes I get the warning during make: unused function 'rna_Scene_automasking_view_normal_editable' in rna_sculpt_editable.c. It would be nice to not have to see that warning anymore. If there is also other recommendations, please let me know. Thank you.

Julien Kaspar commented

2023-05-09 12:55:35 +02:00

@Henry-Chang Sorry for getting back to you so late. I tried testing the PR but it doesn't grey out any options anymore. Perhaps there's somethign wrong and it needs an update?

Henry-Chang commented

2023-05-10 16:47:30 +02:00

First-time contributor

@JulienKaspar Hi, thank you for your reply. This is weird. I just fetched and reset to the gray-out-automasking branch. Built with make lite and was able to get the correct response as shown in the screenshot below? Would like to solve this problem but I can't reproduce the problem at my end?

Captura de pantalla 2023-05-10 a las 10.42.09 p. m..png

3.5 MiB

Julien Kaspar commented

2023-05-12 16:49:52 +02:00

Ok I got it to work on my end!
The behaviour is working great imo and doesn't need any further changes.
@Harley do you have any more notes on the implementation? Otherwise I'd say the WIP tag can be removed and the PR put up for review any time 👍

Ok I got it to work on my end! The behaviour is working great imo and doesn't need any further changes. @Harley do you have any more notes on the implementation? Otherwise I'd say the WIP tag can be removed and the PR put up for review any time 👍

Harley Acheson commented

2023-05-12 17:51:27 +02:00

Taking a quick look I see nothing obviously wrong so I might be a good time to remove "WIP", assuming it does everything it intends to. But I'd guess the review could be a longer process. This is mostly because it is not only solving a particular problem but it is introducing some new UI concepts that it needs - and that we probably need too - but needs some thought.

For example this introduces PROP_INACTIVE and UI_but_inactive, both that seem to do useful things. The state of being "inactive" seems like an obvious no-brainer but we actually (and wrongly) use the term "active" instead of "hover" all over the UI code. I know that is silly and unrelated to this but needs some thought so we don't make these confusing things worse.

"UI_but_inactive()" seems to mirror the behavior of existing "UI_but_disable()" but the latter sets the button disabled while for former does not set inactive, just sets the disabled_info text without doing the disabling. Perhaps "UI_but_inactive" should be something like "UI_but_set_disabled_hint" since that is all it does. And then "UI_but_disable" would call that after it disables the button.

Taking a quick look I see nothing obviously wrong so I might be a good time to remove "WIP", assuming it does everything it intends to. But I'd guess the review could be a longer process. This is mostly because it is not only solving a particular problem but it is introducing some new UI concepts that it needs - and that we probably need too - but needs some thought. For example this introduces PROP_INACTIVE and UI_but_inactive, both that seem to do useful things. The state of being "inactive" seems like an obvious no-brainer but we actually (and wrongly) use the term "active" instead of "hover" all over the UI code. I know that is silly and unrelated to this but needs some thought so we don't make these confusing things worse. "UI_but_inactive()" seems to mirror the behavior of existing "UI_but_disable()" but the latter sets the button disabled while for former does not set inactive, just sets the disabled_info text without doing the disabling. Perhaps "UI_but_inactive" should be something like "UI_but_set_disabled_hint" since that is all it does. And then "UI_but_disable" would call that after it disables the button.

Henry-Chang changed title from ~~WIP: UI: Gray out Scene Auto-Masking option if equivalent Brush option is used #102971~~ to UI: Gray out Scene Auto-Masking option if equivalent Brush option is used #102971

2023-05-14 05:37:16 +02:00

Henry-Chang added 1 commit 2023-05-14 06:22:26 +02:00

Fix: UI_but_inactive to UI_but_set_disabled_hint 0a80f192b6

Henry-Chang commented

2023-05-14 06:28:59 +02:00

First-time contributor

Dear @Harley and @JulienKaspar, thank you for your recommendations, confirmations, and help. I have made the changes to UI_but_set_disabled_hint and have UI_but_disable call it.
I understand that the review could take a while. That is not a problem.
Thank you.

Dear @Harley and @JulienKaspar, thank you for your recommendations, confirmations, and help. I have made the changes to UI_but_set_disabled_hint and have UI_but_disable call it. I understand that the review could take a while. That is not a problem. Thank you.

Julien Kaspar added the

 @ -2078,6 +2078,8 @@ void BKE_sculpt_toolsettings_data_ensure(Scene *scene)
     sd->automasking_start_normal_limit = 20.0f / 180.0f * M_PI;
     sd->automasking_start_normal_falloff = 0.25f;
     sd->automasking_scene_view_normal_limit = 90.0f / 180.0f * M_PI;

 @ -900,2 +900,2 @@
   /* Button is disabled, we may be able to tell user why. */
   if ((but->flag & UI_BUT_DISABLED) || extra_icon) {
   /* Button is disabled or set to inactive, we may be able to tell user why. */
   if ((but->flag & UI_BUT_DISABLED) || (but->flag & UI_BUT_INACTIVE) || extra_icon) {

 @ -4276,0 +4278,4 @@
   ToolSettings *tool_settings = CTX_data_tool_settings(C);
   int automasking_flags = tool_settings->sculpt->paint.brush->automasking_flags;
   if (SCULPT_mode_poll(C) && !(automasking_flags & BRUSH_AUTOMASKING_CAVITY_ALL)) {

 @ -1704,0 +1810,4 @@
   if (sd && (sd->paint.brush->automasking_flags & BRUSH_AUTOMASKING_VIEW_NORMAL)) {
     *r_info = "The active brush already has the same auto-masking enabled.";
     // TODO should probably utilize RNA_def_property_float_funcs to sync values between automasking_view and automasking_scene_view
     sd->automasking_scene_view_normal_limit = sd->automasking_view_normal_limit;

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

UI: Gray out Scene Auto-Masking option if equivalent Brush option is used #102971 #106126

Checkout