Shape Keys: support locking to protect from accidental editing. #104463

Merged
Alexander Gavrilov merged 2 commits from angavrilov/blender:pr-shapekey-locking into main 2024-01-18 13:17:35 +01:00

It is very common for graphical editors with layers to support
locking individual layers to protect them from accidental edits due
to misclicks. Blender itself already supports locking vertex groups.
This adds lock toggles for shape keys, with lock/unlock all operators.

The flags are checked by sculpt brushes, edit mode transform tools,
and Smooth, Propagate and Blend From Shape operators. This selection
aims to cover operations that only deform the mesh, where the shape
key selection matters.

Topology changing operations always apply to all keys, and thus
incorrect shape key selection is less impactful. Excluding them
from the new feature greatly reduces the patch size.


This PR will be pushed as 2 commits:

  • Shape Keys: replace the BKE_keyblock_from_key function with find_index.
  • Shape Keys: support locking to protect from accidental editing.

This spreadsheet shows which operators should and do check for locks.

There also is a design task with more discussion of the overall design.

It is very common for graphical editors with layers to support locking individual layers to protect them from accidental edits due to misclicks. Blender itself already supports locking vertex groups. This adds lock toggles for shape keys, with lock/unlock all operators. The flags are checked by sculpt brushes, edit mode transform tools, and Smooth, Propagate and Blend From Shape operators. This selection aims to cover operations that only deform the mesh, where the shape key selection matters. Topology changing operations always apply to all keys, and thus incorrect shape key selection is less impactful. Excluding them from the new feature greatly reduces the patch size. ------------- This PR will be pushed as 2 commits: * Shape Keys: replace the BKE_keyblock_from_key function with find_index. * Shape Keys: support locking to protect from accidental editing. ------------- This [spreadsheet](https://docs.google.com/spreadsheets/d/1DEm5B3847sn8fmuux_hp4WDt4hFFLB6bVXWH1Fy3rCI/edit?usp=sharing) shows which operators should and do check for locks. There also is a [design task](https://projects.blender.org/blender/blender/issues/105373) with more discussion of the overall design.
Alexander Gavrilov added this to the Modeling project 2023-02-08 13:05:46 +01:00
Author
Member

Here is a UI screenshot:

screenshot-shapekey-locks.png

Here is a UI screenshot: ![screenshot-shapekey-locks.png](/attachments/683366e0-1077-4beb-8bfe-3264acfbc229)
Alexander Gavrilov requested review from Campbell Barton 2023-02-08 13:49:06 +01:00
Alexander Gavrilov requested review from Joseph Eagar 2023-02-12 22:24:35 +01:00

This raises some questions that I'd be interested to hear feedback from artists in the Blender studio.

  • Firstly is this needed, is this something many users who frequently use shape-keys would find beneficial?
  • Are shape keys a special case? (is there other data we might want to lock).
  • Is the current implementation disallowing editing in all anticipated situations? There seems to be quite a few ways to transform vertices that aren't included here, applying transform, moving object origins, ... to name a couple, probably other mesh tools too. It may feel like an incomplete feature if these are left out, yet, this means we need to add check in many more places.
  • Are there ways this can be implemented in a way that doesn't frustrate users. For example, you load someone elses file, select objects and want to apply transform. If they have locked a shepe-key it could mean having to manually select and disable the the locks. Users may request a ways to disable locks on all selected objects, maybe going along in this direction is fine, but we can anticipate some of these issues and consider functionality that doesn't add unnecessary overhead.

While I'm not against this I'd like to be sure this has user approval first and any alternatives have been considered.

This raises some questions that I'd be interested to hear feedback from artists in the Blender studio. - Firstly is this needed, is this something many users who frequently use shape-keys would find beneficial? - Are shape keys a special case? (is there other data we might want to lock). - Is the current implementation disallowing editing in all anticipated situations? There seems to be quite a few ways to transform vertices that aren't included here, applying transform, moving object origins, ... to name a couple, probably other mesh tools too. It may feel like an incomplete feature if these are left out, yet, this means we need to add check in many more places. - Are there ways this can be implemented in a way that doesn't frustrate users. For example, you load someone elses file, select objects and want to apply transform. If they have locked a shepe-key it could mean having to manually select and disable the the locks. Users may request a ways to disable locks on all selected objects, maybe going along in this direction is fine, but we can anticipate some of these issues and consider functionality that doesn't add unnecessary overhead. --- While I'm not against this I'd like to be sure this has user approval first and any alternatives have been considered.
Author
Member
  • Firstly is this needed, is this something many users who frequently use shape-keys would find beneficial?

I find it extremely beneficial. This started when I was working on applying some changes to a set of MakeHuman targets (loaded into Blender as shape keys), and had to discard a bunch of work and restart from backup file because I painted on the basis key by accident, and sculpting can easily exhaust the undo stack before noticing anything.

The purpose is to reduce the chance of modifying the wrong key after a misclick on the shapekey list. The issue is exacerbated by the placement of the shapekey influence sliders in the same list, and also by problems like #104582.

After implementing this safeguard I have already hit the locks a number of times, proving that this is useful. I also always use the locks on vertex groups: on top of special weight painting features, they give a lot of peace of mind.

  • Are shape keys a special case? (is there other data we might want to lock).

Potentially any kind of data with layers, where editing operations in the viewport change their target depending on selection from a list in a different location, rather than in the viewport itself. So, I guess, any attributes that can be painted?..

  • Is the current implementation disallowing editing in all anticipated situations? There seems to be quite a few ways to transform vertices that aren't included here, applying transform, moving object origins, ... to name a few, probably other mesh tools too. It may feel like an incomplete feature if these are left out, yet, this means we need to add check in many more places.

Since the problem this tries to deal with is a mistake in selecting the correct active key, I think any operation that affects all keys does not need checking this. Apply Transform clearly is one of such cases.

I also decided that operations that change mesh topology can be likewise excluded to make things simpler to implement, because they also must affect all keys, and this is a feature for a later stage of the pipeline (mainly rigging or animation basically) than mesh topology modelling.

Edit: It may also be arguably good enough if the most common operations supported the checks, even if some rarer ones omitted them, because it would still reveal the mistake to the user soon enough that changes can be easily reverted via undo. E.g. a topology change operation is almost always followed by tweaking the vertex positions via transform operations, which already checks locks.

  • Are there ways this can be implemented in a way that doesn't frustrate users. For example, you load someone elses file, select objects and want to apply transform. If they have locked a shepe-key it could mean having to manually select and disable the the locks. Users may request a ways to disable locks on all selected objects, maybe going along in this direction is fine, but we can anticipate some of these issues and consider functionality that doesn't add unnecessary overhead.

As I said, I think operations like apply transform that affect all keys uniformly independent of selection don't need to check the locks.

> - Firstly is this needed, is this something many users who frequently use shape-keys would find beneficial? I find it extremely beneficial. This started when I was working on applying some changes to a set of MakeHuman targets (loaded into Blender as shape keys), and had to discard a bunch of work and restart from backup file because I painted on the basis key by accident, and sculpting can _easily_ exhaust the undo stack before noticing anything. The purpose is to reduce the chance of modifying the wrong key after a misclick on the shapekey list. The issue is exacerbated by the placement of the shapekey influence sliders in the same list, and also by problems like #104582. After implementing this safeguard I have already hit the locks a number of times, proving that this is useful. I also always use the locks on vertex groups: on top of special weight painting features, they give a lot of peace of mind. > - Are shape keys a special case? (is there other data we might want to lock). Potentially any kind of data with layers, where editing operations in the viewport change their target depending on selection from a list in a different location, rather than in the viewport itself. So, I guess, any attributes that can be painted?.. > - Is the current implementation disallowing editing in all anticipated situations? There seems to be quite a few ways to transform vertices that aren't included here, applying transform, moving object origins, ... to name a few, probably other mesh tools too. It may feel like an incomplete feature if these are left out, yet, this means we need to add check in many more places. Since the problem this tries to deal with is a mistake in selecting the correct active key, I think any operation that affects all keys does not need checking this. Apply Transform clearly is one of such cases. I also decided that operations that change mesh topology can be likewise excluded to make things simpler to implement, because they also must affect all keys, and this is a feature for a later stage of the pipeline (mainly rigging or animation basically) than mesh topology modelling. Edit: It may also be arguably good enough if the most common operations supported the checks, even if some rarer ones omitted them, because it would still reveal the mistake to the user soon enough that changes can be easily reverted via undo. E.g. a topology change operation is almost always followed by tweaking the vertex positions via transform operations, which already checks locks. > - Are there ways this can be implemented in a way that doesn't frustrate users. For example, you load someone elses file, select objects and want to apply transform. If they have locked a shepe-key it could mean having to manually select and disable the the locks. Users may request a ways to disable locks on all selected objects, maybe going along in this direction is fine, but we can anticipate some of these issues and consider functionality that doesn't add unnecessary overhead. As I said, I think operations like apply transform that affect all keys uniformly independent of selection don't need to check the locks.
Author
Member

However, any suggestions on what other operations should also apply these checks are welcome - I don't claim that the current coverage is exhaustive, it is just the obvious candidates that I personally remembered, because I was actively using them at the time.

However, any suggestions on what other operations should also apply these checks are welcome - I don't claim that the current coverage is exhaustive, it is just the obvious candidates that I personally remembered, because I was actively using them at the time.
Sybren A. Stüvel added the
Interest
Animation & Rigging
label 2023-02-17 14:33:53 +01:00
Member

I think this is a good idea for attributes in general in the future, and yes, I think it would be useful for shape keys specifically as well; The problem Alexander describes of accidentally sculpting on the Basis shape is fairly relatable.

That said, putting checks in every place in the code that might modify this data, and having to decide whether it should consider the lock state or not, sounds like it adds some maintenance and complexity... But I'll leave that concern for the engineers.

I think this is a good idea for attributes in general in the future, and yes, I think it would be useful for shape keys specifically as well; The problem Alexander describes of accidentally sculpting on the Basis shape is fairly relatable. That said, putting checks in every place in the code that might modify this data, and having to decide whether it should consider the lock state or not, sounds like it adds some maintenance and complexity... But I'll leave that concern for the engineers.

Can we please have a proper design task, leading to a proper design (both on UI/UX and technical aspects) signed of by the relevant team(s) first? A patch review is not the place to have these kind of discussions, definitely not with changes of that magnitude.

Can we please have a proper design task, leading to a proper design (both on UI/UX and technical aspects) signed of by the relevant team(s) first? A patch review is ***not*** the place to have these kind of discussions, definitely not with changes of that magnitude.
Author
Member

@mont29 The reason I generally don't do design tasks is that I don't want to make an abstract design for some abstract users some time in the future, I want a problem fixed here and now for myself. The whole point of open source is to enable this. After I'm satisfied with the result, I create a pull request because I already have a fully working implementation. I genuinely don't quite see how a design task would fit into the picture at that point (for real, I'm not just arguing with you for the sake of arguing). I see creating a design task as more a "there's a problem, I have no clear idea how to fix it, let's discuss" kind of thing.

@mont29 The reason I generally don't do design tasks is that I don't want to make an abstract design for some abstract users some time in the future, I want a problem fixed here and now for myself. The whole point of open source is to enable this. After I'm satisfied with the result, I create a pull request because I already have a fully working implementation. I genuinely don't quite see how a design task would fit into the picture at that point (for real, I'm not just arguing with you for the sake of arguing). I see creating a design task as more a "there's a problem, I have no clear idea how to fix it, let's discuss" kind of thing.
Author
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/PR104463) when ready.
Alexander Gavrilov force-pushed pr-shapekey-locking from 9d25ea0677 to c57ee37f68 2023-03-20 12:25:04 +01:00 Compare
Author
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/PR104463) when ready.
Author
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/PR104463) when ready.
Joseph Eagar approved these changes 2023-04-16 23:26:41 +02:00
@ -5805,2 +5806,4 @@
ss->face_sets = BKE_sculpt_face_sets_ensure(mesh);
}
else {
if (ss->shapekey_active && (ss->shapekey_active->flag & KEYBLOCK_LOCKED_SHAPE) != 0) {
Member

Why is this if here nested?

Why is this if here nested?
Author
Member

Added a comment to explain the intent that this sequence of outer ifs is branching purely on the sculpt tool type.

One alternative could be to convert this to a switch instead of using SCULPT_tool_is_mask etc.

Added a comment to explain the intent that this sequence of outer ifs is branching purely on the sculpt tool type. One alternative could be to convert this to a switch instead of using SCULPT_tool_is_mask etc.
Alexander Gavrilov force-pushed pr-shapekey-locking from c57ee37f68 to 7ce05044df 2023-07-15 10:39:57 +02:00 Compare
Alexander Gavrilov force-pushed pr-shapekey-locking from 7ce05044df to 1576a771a3 2023-07-24 15:55:39 +02:00 Compare
Alexander Gavrilov force-pushed pr-shapekey-locking from 1576a771a3 to c877f577f2 2023-08-09 15:31:08 +02:00 Compare
Author
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/PR104463) when ready.
Alexander Gavrilov force-pushed pr-shapekey-locking from c877f577f2 to ac212c675c 2023-08-22 16:57:40 +02:00 Compare
Alexander Gavrilov requested review from Sybren A. Stüvel 2023-09-07 16:27:26 +02:00
Sybren A. Stüvel requested changes 2023-11-02 11:35:55 +01:00
Sybren A. Stüvel left a comment
Member

Nice work! Overall it seems good. My biggest gripe is with the inverted naming -- see the inline notes for that.

I think this PR could be split into two parts: adding checks where locking was already possible, and adding locking where that wasn't possible before. I feel that the former is more like a bugfix, where the missing checks were really an issue, and the latter is adding a new feature.

Nice work! Overall it seems good. My biggest gripe is with the inverted naming -- see the inline notes for that. I think this PR could be split into two parts: adding checks where locking was already possible, and adding locking where that wasn't possible before. I feel that the former is more like a bugfix, where the missing checks were really an issue, and the latter is adding a new feature.
@ -266,0 +268,4 @@
def object_check_active_shape_key_unlocked(obj, operator=None):
"""
Checks that the active shape key of the specified object is not locked.

I read the function name as "there is an active shape key, and it is not locked". This is not what the code implements, though.

Avoid negated booleans. If "locked" is the modeled concept, stick to that. If you are using this function as a query "can the active shape key be edited?", use a name like object_check_active_shape_key_editable. But see below ;-)

With the negation as it is now, I'm struggling to understand the case where there is no active shape key, as in this case it returns True, but that doesn't match with how I read the function name.

And finally, I would recommend importing this function with from bpy_extras import object_utils, and then using object_utils.function_name(). That makes it clear at the call site where the function comes from. It also means that the word object can be removed from the function name, and check can be replaced with is to indicate the boolean nature. That would make the call object_utils.is_active_shape_key_editable(obj). But since all the calls are negating the returned value once again, object_utils.is_active_shape_key_locked(obj) seems like a more appropriate name (which implies negating the returned value, of course).

These comments about inverted booleans also apply to the C++ code.

I read the function name as "there is an active shape key, and it is not locked". This is not what the code implements, though. Avoid negated booleans. If "locked" is the modeled concept, stick to that. If you are using this function as a query "can the active shape key be edited?", use a name like `object_check_active_shape_key_editable`. But see below ;-) With the negation as it is now, I'm struggling to understand the case where there is no active shape key, as in this case it returns `True`, but that doesn't match with how I read the function name. And finally, I would recommend importing this function with `from bpy_extras import object_utils`, and then using `object_utils.function_name()`. That makes it clear at the call site where the function comes from. It also means that the word `object` can be removed from the function name, and `check` can be replaced with `is` to indicate the boolean nature. That would make the call `object_utils.is_active_shape_key_editable(obj)`. But since all the calls are negating the returned value once again, `object_utils.is_active_shape_key_locked(obj)` seems like a more appropriate name (which implies negating the returned value, of course). These comments about inverted booleans also apply to the C++ code.
Author
Member

The function is not a query though, its point is to encapsulate emitting the warning message to avoid having to repeat it, so that operators that need the check can contain just:

if not check(...):
   return

Rather than an 'is' query, it is more akin to 'assert', except it is not for debugging but for an actual behavior check, and uses a return code to signify the need to abort and unwind instead of throwing an exception.

If there is no active shape key, it cannot be locked (remember that unlocked is the default). That situation obviously is normal for any mesh without shape keys.

Edit: trying to make the intent clear, changed naming to "...verify...is_editable".

The function is not a query though, its point is to encapsulate emitting the warning message to avoid having to repeat it, so that operators that need the check can contain just: ``` if not check(...): return ``` Rather than an 'is' query, it is more akin to 'assert', except it is not for debugging but for an actual behavior check, and uses a return code to signify the need to abort and unwind instead of throwing an exception. If there is no active shape key, it cannot be locked (remember that unlocked is the default). That situation obviously is normal for any mesh without shape keys. Edit: trying to make the intent clear, changed naming to "...verify...is_editable".
dr.sybren marked this conversation as resolved
@ -266,0 +276,4 @@
:type operator: :class:`bpy.types.Operator`
:return: True if the object has no shape keys, or the active one is unlocked.
"""
if obj:

In which cases does it make sense to call object_check_active_shape_key_unlocked(None)? As far as I can see, if there is no object specified it's a bug, and this function shouldn't hide that.

If there is a good reason to support calling with None, document it and change the code to:

if not obj:
    return True

That way the entire function body can be unindented.

In which cases does it make sense to call `object_check_active_shape_key_unlocked(None)`? As far as I can see, if there is no object specified it's a bug, and this function shouldn't hide that. If there is a good reason to support calling with `None`, document it and change the code to: ```python if not obj: return True ``` That way the entire function body can be unindented.
Author
Member

I was thinking about reducing repetition by allowing to call the function on a null object without explicitly checking, but that's probably going too far, so removed this check.

I was thinking about reducing repetition by allowing to call the function on a null object without explicitly checking, but that's probably going too far, so removed this check.
dr.sybren marked this conversation as resolved
@ -89,3 +89,3 @@
struct KeyBlock *BKE_keyblock_add_ctime(struct Key *key, const char *name, bool do_force);
/**
* Get the appropriate #KeyBlock given an index.
* Get the appropriate #KeyBlock given an index, excluding the first (0th) key. Key may be null.

I don't understand the additional comment here. Does this mean that the index should be offset to account for the base key not counting? This should be documented better.

I don't understand the additional comment here. Does this mean that the index should be offset to account for the base key not counting? This should be documented better.
Author
Member

For whatever reason that function is designed to return null instead of the 0th (basis) key.

For whatever reason that function is designed to return null instead of the 0th (basis) key.
@ -94,0 +94,4 @@
/**
* Get the appropriate #KeyBlock given an index. Key may be null.
*/
struct KeyBlock *BKE_keyblock_find_index(struct Key *key, int index);

What differentiates this function from BKE_keyblock_from_key()? This should be documented much better.

Also depending on what sets this function apart from its sibling: does this difference really warrant having another function with a confusingly similar name? Would it be possible to rename these functions such that their difference is clear from the naming?

What differentiates this function from `BKE_keyblock_from_key()`? This should be documented much better. Also depending on what sets this function apart from its sibling: does this difference really warrant having another function with a confusingly similar name? Would it be possible to rename these functions such that their difference is clear from the naming?
Author
Member

I split changes about this function into a separate commit to make them clear, and removed from_key.

Like I said, from_key was weird in that it returned null for index 0 instead of the basis key, which makes it less generic than find_index. However this quirk is not necessary because the index can be checked for zero before the call (and in fact was checked in 2 out of 3 call sites), so the old function could be removed. I decided to still change the function name because of the subtle change in behavior.

I split changes about this function into a separate commit to make them clear, and removed `from_key`. Like I said, `from_key` was weird in that it returned null for index 0 instead of the basis key, which makes it less generic than find_index. However this quirk is not necessary because the index can be checked for zero before the call (and in fact was checked in 2 out of 3 call sites), so the old function could be removed. I decided to still change the function name because of the subtle change in behavior.
@ -1930,1 +1925,4 @@
KeyBlock *BKE_keyblock_find_index(Key *key, int index)
{
if (key) {

Write precondition checks as such:

if (!key) {
    return nullptr;
}

return static_cast<KeyBlock *>(BLI_findlink(&key->block, index));
Write precondition checks as such: ```cpp if (!key) { return nullptr; } return static_cast<KeyBlock *>(BLI_findlink(&key->block, index)); ```
angavrilov marked this conversation as resolved
@ -2892,3 +2926,3 @@
MEM_freeN(objects);
return OPERATOR_FINISHED;
return totobjects ? OPERATOR_FINISHED : OPERATOR_CANCELLED;

I like this, good addition!

I like this, good addition!
@ -2878,3 +2905,3 @@
MEM_freeN(objects);
if (tot_unselected == objects_len) {
if (tot_selected == 0 && !tot_locked) {

tot_locked is not a boolean, so I'd suggest being consistent & explicit here:

if (tot_selected == 0 && tot_locked == 0) 
`tot_locked` is not a boolean, so I'd suggest being consistent & explicit here: ```cpp if (tot_selected == 0 && tot_locked == 0) ```
@ -2881,4 +2908,3 @@
BKE_report(op->reports, RPT_WARNING, "No selected vertex");
return OPERATOR_CANCELLED;
}

I think it would be good to have a warning when all selected vertices were locked, so that you can tell from the warnings why the operator didn't do anything.

In general I think that's a good idea (letting the user know explicitly that nothing happened, and why), but here it's even more important because the original code already had such a warning in place.

Same for the other places where nothing happens because of the lock checks.

I think it would be good to have a warning when all selected vertices were locked, so that you can tell from the warnings why the operator didn't do anything. In general I think that's a good idea (letting the user know explicitly that nothing happened, and why), but here it's even more important because the original code already had such a warning in place. Same for the other places where nothing happens because of the lock checks.
Author
Member

There is no such thing as "nothing happens because of lock checks". Like I said, the whole point of those check functions not being those simple "is..." things you were suggesting is that they emit an error message if the check fails, thus avoiding the need to plaster the duplicated text of that message all over the code.

There is no such thing as "nothing happens because of lock checks". Like I said, the whole point of those check functions not being those simple "is..." things you were suggesting is that they emit an error message if the check fails, thus avoiding the need to plaster the duplicated text of that message all over the code.
@ -294,3 +297,3 @@
struct SculptGestureOperation {
/* Initial setup (data updates, special undo push...). */
/* Initial setup (data updates...) before the undo push. */

Does this PR change the way the undo push happens? If not, I think this comment change shouldn't be in here. If it is, then it should be clearly described in the PR description.

Does this PR change the way the undo push happens? If not, I think this comment change shouldn't be in here. If it is, then it should be clearly described in the PR description.
Author
Member

See sculpt_gesture_apply below. I add init that is called before the undo push and thus can completely abort the operation. Begin like before is called after the base undo push, and thus cannot abort any more.
Operators that don't need the undo push for their initialization code and do the checks are changed to use init instead.

See `sculpt_gesture_apply` below. I add `init` that is called before the undo push and thus can completely abort the operation. Begin like before is called after the base undo push, and thus cannot abort any more. Operators that don't need the undo push for their initialization code and do the checks are changed to use `init` instead.

These kind of design changes should be documented in the PR description, for sure. Currently this is not mentioned at all. I also feel that changes to the interaction with the undo system should be separated from the additional lock checks.

These kind of design changes should be documented in the PR description, for sure. Currently this is not mentioned at all. I also feel that changes to the interaction with the undo system should be separated from the additional lock checks.
@ -698,3 +706,3 @@
}
static void sculpt_gesture_apply(bContext *C, SculptGestureContext *sgcontext, wmOperator *op)
static bool sculpt_gesture_apply(bContext *C, SculptGestureContext *sgcontext, wmOperator *op)

Document what the returned boolean means.

Document what the returned boolean means.
@ -703,3 +716,3 @@
SCULPT_undo_push_begin(CTX_data_active_object(C), op);
operation->sculpt_gesture_begin(C, sgcontext);
if (operation->sculpt_gesture_begin) {

In which situations can operation->sculpt_gesture_begin be nullptr? And how does this PR change that this is now suddenly possible?

In which situations can `operation->sculpt_gesture_begin` be `nullptr`? And how does this PR change that this is now suddenly possible?
Author
Member

Because I added init and changed operators that can use it to use it instead of begin.

Because I added `init` and changed operators that can use it to use it instead of begin.
dr.sybren marked this conversation as resolved
@ -722,0 +737,4 @@
return true;
}
static int sculpt_gesture_apply_and_free(bContext *C,

This should document what it returns.

This should document what it returns.
dr.sybren marked this conversation as resolved
@ -722,0 +743,4 @@
{
const bool ok = sculpt_gesture_apply(C, sgcontext, op);
sculpt_gesture_context_free(sgcontext);

Why is it ok to free the gesture context when the 'apply' function didn't work? This is a functional change, and should be properly motivated & documented.

Why is it ok to free the gesture context when the 'apply' function didn't work? This is a functional change, and should be properly motivated & documented.
Author
Member

It is NOT a functional change. It encapsulates repeated pairs of apply + free, because now operators also need to check the boolean return code and convert it into an operator status enum value.

It is NOT a functional change. It encapsulates repeated pairs of apply + free, because now operators also need to check the boolean return code and convert it into an operator status enum value.
dr.sybren marked this conversation as resolved
@ -730,3 +757,3 @@
};
static void sculpt_gesture_face_set_begin(bContext *C, SculptGestureContext *sgcontext)
static bool sculpt_gesture_face_set_init(bContext *C, SculptGestureContext *sgcontext)

Why is this change necessary? In which case do you expect this to return false in the future? Without this info, the return true seems entirely unnecessary.

This PR changes the op.sculpt_gesture_begin to op.sculpt_gesture_init, but still plugs in some ..._begin functions here and there. Some ..._begin functions are renamed to ..._init, some are not. What is the distinction?

Why is this change necessary? In which case do you expect this to return `false` in the future? Without this info, the `return true` seems entirely unnecessary. This PR changes the `op.sculpt_gesture_begin` to `op.sculpt_gesture_init`, but still plugs in some `..._begin` functions here and there. Some `..._begin` functions are renamed to `..._init`, some are not. What is the distinction?
Author
Member

I changed the naming: init -> begin (i.e. named like original, but returns boolean); begin -> begin_post_undo.

I changed the naming: `init` -> `begin` (i.e. named like original, but returns boolean); `begin` -> `begin_post_undo`.
@ -5869,2 +5885,4 @@
ss->face_sets = BKE_sculpt_face_sets_ensure(ob);
}
else { /* Other, non-attribute sculpt tools */
if (!ED_sculpt_check_shape_key_unlocked(ob, op->reports)) {

This can be another else if and reduce one level of nesting.

This can be another `else if` and reduce one level of nesting.
Author
Member

The idea was to distinguish branching between types of sculpt tools, and the logic for the check. Basically the logic here is:

switch (sculpt_tool) {
case ...
default:
   if (!unlocked(...)) {...}
}

Edit: I think this actually can be replaced with a switch too, except it will break the encapsulation of all those is functions.

The idea was to distinguish branching between types of sculpt tools, and the logic for the check. Basically the logic here is: ``` switch (sculpt_tool) { case ... default: if (!unlocked(...)) {...} } ``` Edit: I think this actually can be replaced with a switch too, except it will break the encapsulation of all those `is` functions.
dr.sybren marked this conversation as resolved
Alexander Gavrilov force-pushed pr-shapekey-locking from ac212c675c to 92437c113e 2023-11-20 11:35:42 +01:00 Compare
-  static void sculpt_gesture_face_set_begin(bContext *C, SculptGestureContext *sgcontext)
+  static bool sculpt_gesture_face_set_init(bContext *C, SculptGestureContext *sgcontext)

Why is this change necessary? In which case do you expect this to return false in the future? Without this info, the return true seems entirely unnecessary.

This question is still open. Also from my other questions that relate to the refactor of the _begin, _init, and _begin_post_undo, it seems that that change is better to be put in a separate PR, or at the minimum as a separate commit.

That situation obviously is normal for any mesh without shape keys.

When people have questions, that is proof that things are not "obvious". When you state that it is, it comes across as rather arrogant and dismissive. Please stop replying that things are "obvious".

I split changes about this function into a separate commit to make them clear, and removed from_key.

Make sure the commit message follows the style guide (start title with 'Refactor' and include a note that this is a non-functional change). Also the documentation of the BKE_keyblock_find_index should mention what the index parameter means, as when it comes to shapekeys there are two approaches to indexing them in Blender (effectively base-0 and base-1).

I haven't been able to go over everything that changed since my last review, so this is only a partial response.

```diff - static void sculpt_gesture_face_set_begin(bContext *C, SculptGestureContext *sgcontext) + static bool sculpt_gesture_face_set_init(bContext *C, SculptGestureContext *sgcontext) ``` > Why is this change necessary? In which case do you expect this to return `false` in the future? Without this info, the `return true` seems entirely unnecessary. This question is still open. Also from my other questions that relate to the refactor of the `_begin`, `_init`, and `_begin_post_undo`, it seems that that change is better to be put in a separate PR, or at the minimum as a separate commit. > That situation obviously is normal for any mesh without shape keys. When people have questions, that is proof that things are not "obvious". When you state that it is, it comes across as rather arrogant and dismissive. Please stop replying that things are "obvious". > I split changes about this function into a separate commit to make them clear, and removed `from_key`. Make sure the commit message follows the style guide (start title with 'Refactor' and include a note that this is a non-functional change). Also the documentation of the `BKE_keyblock_find_index` should mention what the `index` parameter means, as when it comes to shapekeys there are two approaches to indexing them in Blender (effectively base-0 and base-1). I haven't been able to go over everything that changed since my last review, so this is only a partial response.
Author
Member

Why is this change necessary? In which case do you expect this to return false in the future? Without this info, the return true seems entirely unnecessary.

This question is still open.

Um, this question doesn't make much sense, because this function is used as a callback, so if the API itself is changed, the return type must be changed even if this specific callback would always return the same value.

> > Why is this change necessary? In which case do you expect this to return `false` in the future? Without this info, the `return true` seems entirely unnecessary. > > This question is still open. Um, this question doesn't make much sense, because this function is used as a callback, so if the API itself is changed, the return type must be changed even if this specific callback would always return the same value.

Um, this question doesn't make much sense

May I suggest an attitude change?

because this function is used as a callback

This is far from obvious when reading through the PR changes, especially since the code that registers this callback is not part of the changes, and thus invisible when you just read here through the web interface. Moreover it's sculpting-related code, and the person asking the question (me) is usually developing in another module; so I might be unfamiliar with this particular code. If there are any questions about a change, then I would suggest simply answering them in a neutral way.

Earlier I wrote:

Also from my other questions that relate to the refactor of the _begin, _init, and _begin_post_undo, it seems that that change is better to be put in a separate PR

This hasn't been addressed yet.

> Um, this question doesn't make much sense May I suggest an attitude change? > because this function is used as a callback This is far from obvious when reading through the PR changes, especially since the code that registers this callback is not part of the changes, and thus invisible when you just read here through the web interface. Moreover it's sculpting-related code, and the person asking the question (me) is usually developing in another module; so I might be unfamiliar with this particular code. If there are any questions about a change, then I would suggest simply answering them in a neutral way. Earlier I wrote: > Also from my other questions that relate to the refactor of the `_begin`, `_init`, and `_begin_post_undo`, it seems that that change is better to be put in a separate PR This hasn't been addressed yet.
Author
Member

This is far from obvious when reading through the PR changes, especially since the code that registers this callback is not part of the changes, and thus invisible when you just read here through the web interface. Moreover it's sculpting-related code, and the person asking the question (me) is usually developing in another module; so I might be unfamiliar with this particular code. If there are any questions about a change, then I would suggest simply answering them in a neutral way.

I obviously wasn't clear, but the part that didn't make sense to me in this case is that the second part of your question seemed to strongly imply that unless I can state some case where the function would return false in the future, the change isn't appropriate. But in this case, the reason for the change is something completely different.

Edit: basically, what I wanted to say is that I'm not going to answer the second part about return because it's irrelevant here.

Also from my other questions that relate to the refactor of the _begin, _init, and _begin_post_undo, it seems that that change is better to be put in a separate PR

This hasn't been addressed yet.

It's also not clear to me how exactly do you want to split this:

I think this PR could be split into two parts: adding checks where locking was already possible, and adding locking where that wasn't possible before. I feel that the former is more like a bugfix, where the missing checks were really an issue, and the latter is adding a new feature.

There is no "where locking was already possible", because the whole point of this whole PR is adding the locks. No locking was possible before. Unless you mean to reduce this PR to just adding locking + easily performed checks, and addressing a few of the more complex cases in subsequent PRs?

> This is far from obvious when reading through the PR changes, especially since the code that registers this callback is not part of the changes, and thus invisible when you just read here through the web interface. Moreover it's sculpting-related code, and the person asking the question (me) is usually developing in another module; so I might be unfamiliar with this particular code. If there are any questions about a change, then I would suggest simply answering them in a neutral way. I obviously wasn't clear, but the part that didn't make sense to me in this case is that the second part of your question seemed to strongly imply that unless I can state some case where the function would return false in the future, the change isn't appropriate. But in this case, the reason for the change is something completely different. Edit: basically, what I wanted to say is that I'm not going to answer the second part about return because it's irrelevant here. > > Also from my other questions that relate to the refactor of the `_begin`, `_init`, and `_begin_post_undo`, it seems that that change is better to be put in a separate PR > > This hasn't been addressed yet. It's also not clear to me how exactly do you want to split this: > I think this PR could be split into two parts: adding checks where locking was already possible, and adding locking where that wasn't possible before. I feel that the former is more like a bugfix, where the missing checks were really an issue, and the latter is adding a new feature. There is no "where locking was already possible", because the whole point of this whole PR is adding the locks. No locking was possible before. Unless you mean to reduce this PR to just adding locking + easily performed checks, and addressing a few of the more complex cases in subsequent PRs?
Alexander Gavrilov force-pushed pr-shapekey-locking from 92437c113e to 2290923f21 2023-12-25 21:35:51 +01:00 Compare
Author
Member
  • Rebased on main and fixed conflicts.
  • For now removed the changes in paint_mask.cc to be handled later: they are all just to add checks to sculpt.project_line_gesture which technically should be doing the checks according to the proposed guidelines, but the operation probably isn't that common.
* Rebased on main and fixed conflicts. * For now removed the changes in `paint_mask.cc` to be handled later: they are all just to add checks to `sculpt.project_line_gesture` which technically should be doing the checks according to the proposed guidelines, but the operation probably isn't that common.

For now removed the changes in paint_mask.cc to be handled later: they are all just to add checks to sculpt.project_line_gesture which technically should be doing the checks according to the proposed guidelines, but the operation probably isn't that common.

Thanks, that makes my work considerably simpler :)

Since Gitea is so cumbersome when working with inline notes (AFAIK they get disconnected from their place the file when you force-push) I'll just write things here in a long comment.


On Shape Keys: replace the BKE_keyblock_from_key function with find_index:

BKE_keyblock_find_index() doesn't find a keyblock's index, so I think it's better to name it BKE_keyblock_find_by_index(). My head is mostly with bone collections (as that is what I've been working on the past weeks), and those actually have a function armature_bonecoll_find_index() that finds the index of a bone collection.

Get the appropriate #KeyBlock given a 0-based index. Key may be null.; it still is unclear what the 0 index would mean. Maybe just add a \param index Index of the key to retrieve, where index 0 is the base shape. The fact that key can be null can then also be documented in a \param.

In source/blender/blenkernel/intern/DerivedMesh.cc: if (clmd && clmd->sim_parms->shapekey_rest); since key can be null, the addition of the check on clmd->sim_parms->shapekey_rest seems unnecessary. Maybe you meant to check clmd->sim_parms to avoid a null pointer dereference?


On Shape Keys: support locking to protect from accidental editing.:

There is still (IMO) an issue with the naming of the ..._is_editable() functions. These seem to return true whenever the thing that is supposed to be editable doesn't exist. For example, ED_mesh_edit_verify_shape_key_is_editable returne true when there is no shape key at all. This to me is dangerous, as "editable" implies "existence" to me.

You can see that the concepts of 'locked' and 'editable' are intermingled, in code like:

if (!ED_mesh_edit_verify_shape_key_is_editable(mesh, op->reports)) {
  tot_locked++;
  continue;
}

Here 'editable' is queried, but 'locked' is counted. As you said before that this is mostly about the reporting part, and less about the checking part, I would suggest naming the function ED_mesh_edit_report_if_shapekey_locked(...), and then documenting well what the returned bool means.

/** Checks if the currently active Edit Mode on the object is targeting a locked shape key. */
bool ED_object_edit_verify_shape_key_is_editable(const Object *obedit, ReportList *reports);

This documents the opposite of what is actually returned. Please make sure that all those functions are well documented, so that someone who isn't in the loop on the internal logic can still understand what the function does.

> For now removed the changes in `paint_mask.cc` to be handled later: they are all just to add checks to `sculpt.project_line_gesture` which technically should be doing the checks according to the proposed guidelines, but the operation probably isn't that common. Thanks, that makes my work considerably simpler :) Since Gitea is so cumbersome when working with inline notes (AFAIK they get disconnected from their place the file when you force-push) I'll just write things here in a long comment. --------- On [Shape Keys: replace the BKE_keyblock_from_key function with find_index](https://projects.blender.org/blender/blender/pulls/104463/commits/484f1fcec48f56d4f178553bb1338d92ea718316): `BKE_keyblock_find_index()` doesn't find a keyblock's index, so I think it's better to name it `BKE_keyblock_find_by_index()`. My head is mostly with bone collections (as that is what I've been working on the past weeks), and those actually have a function `armature_bonecoll_find_index()` that finds the index of a bone collection. `Get the appropriate #KeyBlock given a 0-based index. Key may be null.`; it still is unclear what the `0` index would mean. Maybe just add a `\param index Index of the key to retrieve, where index 0 is the base shape`. The fact that `key` can be `null` can then also be documented in a `\param`. In `source/blender/blenkernel/intern/DerivedMesh.cc`: `if (clmd && clmd->sim_parms->shapekey_rest)`; since `key` can be `null`, the addition of the check on `clmd->sim_parms->shapekey_rest` seems unnecessary. Maybe you meant to check `clmd->sim_parms` to avoid a null pointer dereference? ----------- On [Shape Keys: support locking to protect from accidental editing.](https://projects.blender.org/blender/blender/commit/2290923f21eb5ddb9f3f7bd406b87399cdea4ab9): There is still (IMO) an issue with the naming of the `..._is_editable()` functions. These seem to return `true` whenever the thing that is supposed to be editable doesn't exist. For example, `ED_mesh_edit_verify_shape_key_is_editable` returne `true` when there is no shape key at all. This to me is dangerous, as "editable" implies "existence" to me. You can see that the concepts of 'locked' and 'editable' are intermingled, in code like: ```cpp if (!ED_mesh_edit_verify_shape_key_is_editable(mesh, op->reports)) { tot_locked++; continue; } ``` Here 'editable' is queried, but 'locked' is counted. As you said before that this is mostly about the reporting part, and less about the checking part, I would suggest naming the function `ED_mesh_edit_report_if_shapekey_locked(...)`, and then documenting well what the returned `bool` means. ```cpp /** Checks if the currently active Edit Mode on the object is targeting a locked shape key. */ bool ED_object_edit_verify_shape_key_is_editable(const Object *obedit, ReportList *reports); ``` This documents the opposite of what is actually returned. Please make sure that all those functions are well documented, so that someone who isn't in the loop on the internal logic can still understand what the function does.
Sybren A. Stüvel requested changes 2024-01-08 12:06:30 +01:00
@ -89,0 +94,4 @@
if (key_block && (key_block->flag & KEYBLOCK_LOCKED_SHAPE) != 0) {
if (reports) {
BKE_report(reports, RPT_ERROR, "The active shape key is locked");

Since this function is called in a for-loop that iterates all objects in edit mode, there are a few usability issues:

  • The message can appear multiple times (once per object), but since it's the same every time, it doesn't provide any extra information beyond the first one.
  • There is no indication of which object the message is referred to.

I think both can be handled in one go by including the object name in the message.

Since this function is called in a `for`-loop that iterates all objects in edit mode, there are a few usability issues: - The message can appear multiple times (once per object), but since it's the same every time, it doesn't provide any extra information beyond the first one. - There is no indication of which object the message is referred to. I think both can be handled in one go by including the object name in the message.
Alexander Gavrilov force-pushed pr-shapekey-locking from 2290923f21 to fcf2083f12 2024-01-17 14:30:27 +01:00 Compare
Author
Member

BKE_keyblock_find_index() doesn't find a keyblock's index, so I think it's better to name it BKE_keyblock_find_by_index().

Renamed.

In source/blender/blenkernel/intern/DerivedMesh.cc: if (clmd && clmd->sim_parms->shapekey_rest); since key can be null, the addition of the check on clmd->sim_parms->shapekey_rest seems unnecessary. Maybe you meant to check clmd->sim_parms to avoid a null pointer dereference?

No, the point is that the original function returned null for a 0 index value, but the new one returns the basis key. Therefore the check is necessary to preserve the old logic. Call sites other than this one already check for 0, so no changes were necessary.

Here 'editable' is queried, but 'locked' is counted. As you said before that this is mostly about the reporting part, and less about the checking part, I would suggest naming the function ED_mesh_edit_report_if_shapekey_locked(...), and then documenting well what the returned bool means.

Refactored.

I think both can be handled in one go by including the object name in the message.

Changed the message.

> `BKE_keyblock_find_index()` doesn't find a keyblock's index, so I think it's better to name it `BKE_keyblock_find_by_index()`. Renamed. > In `source/blender/blenkernel/intern/DerivedMesh.cc`: `if (clmd && clmd->sim_parms->shapekey_rest)`; since `key` can be `null`, the addition of the check on `clmd->sim_parms->shapekey_rest` seems unnecessary. Maybe you meant to check `clmd->sim_parms` to avoid a null pointer dereference? No, the point is that the original function returned null for a 0 index value, but the new one returns the basis key. Therefore the check is necessary to preserve the old logic. Call sites other than this one already check for 0, so no changes were necessary. > Here 'editable' is queried, but 'locked' is counted. As you said before that this is mostly about the reporting part, and less about the checking part, I would suggest naming the function `ED_mesh_edit_report_if_shapekey_locked(...)`, and then documenting well what the returned `bool` means. Refactored. > I think both can be handled in one go by including the object name in the message. Changed the message.
Sybren A. Stüvel approved these changes 2024-01-18 12:47:34 +01:00
Sybren A. Stüvel left a comment
Member

In source/blender/blenkernel/intern/DerivedMesh.cc: if (clmd && clmd->sim_parms->shapekey_rest); since key can be null, the addition of the check on clmd->sim_parms->shapekey_rest seems unnecessary. Maybe you meant to check clmd->sim_parms to avoid a null pointer dereference?

No, the point is that the original function returned null for a 0 index value, but the new one returns the basis key. Therefore the check is necessary to preserve the old logic. Call sites other than this one already check for 0, so no changes were necessary.

Gotcha.

Here 'editable' is queried, but 'locked' is counted. As you said before that this is mostly about the reporting part, and less about the checking part, I would suggest naming the function ED_mesh_edit_report_if_shapekey_locked(...), and then documenting well what the returned bool means.

Refactored.

Thanks, I feel that the code is significantly easier to follow now :)

I think both can be handled in one go by including the object name in the message.

Changed the message.

👍

> > In `source/blender/blenkernel/intern/DerivedMesh.cc`: `if (clmd && clmd->sim_parms->shapekey_rest)`; since `key` can be `null`, the addition of the check on `clmd->sim_parms->shapekey_rest` seems unnecessary. Maybe you meant to check `clmd->sim_parms` to avoid a null pointer dereference? > > No, the point is that the original function returned null for a 0 index value, but the new one returns the basis key. Therefore the check is necessary to preserve the old logic. Call sites other than this one already check for 0, so no changes were necessary. Gotcha. > > Here 'editable' is queried, but 'locked' is counted. As you said before that this is mostly about the reporting part, and less about the checking part, I would suggest naming the function `ED_mesh_edit_report_if_shapekey_locked(...)`, and then documenting well what the returned `bool` means. > > Refactored. Thanks, I feel that the code is significantly easier to follow now :) > > I think both can be handled in one go by including the object name in the message. > > Changed the message. :+1:
Alexander Gavrilov merged commit b350d7a4c3 into main 2024-01-18 13:17:35 +01:00
Alexander Gavrilov deleted branch pr-shapekey-locking 2024-01-18 13:17:38 +01: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
Interest: X11
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
7 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#104463
No description provided.