Shape Keys: support locking to protect from accidental editing. #104463
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#104463
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "angavrilov/blender:pr-shapekey-locking"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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:
This spreadsheet shows which operators should and do check for locks.
There also is a design task with more discussion of the overall design.
Here is a UI screenshot:
This raises some questions that I'd be interested to hear feedback from artists in the Blender studio.
While I'm not against this I'd like to be sure this has user approval first and any alternatives have been considered.
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.
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?..
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.
As I said, I think operations like apply transform that affect all keys uniformly independent of selection don't need to check the locks.
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.
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.
@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.
@blender-bot package
Package build started. Download here when ready.
9d25ea0677
toc57ee37f68
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@ -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) {
Why is this if here nested?
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.
c57ee37f68
to7ce05044df
7ce05044df
to1576a771a3
1576a771a3
toc877f577f2
@blender-bot package
Package build started. Download here when ready.
c877f577f2
toac212c675c
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 usingobject_utils.function_name()
. That makes it clear at the call site where the function comes from. It also means that the wordobject
can be removed from the function name, andcheck
can be replaced withis
to indicate the boolean nature. That would make the callobject_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.
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:
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".
@ -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:That way the entire function body can be unindented.
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.
@ -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.
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?
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:
@ -2892,3 +2926,3 @@
MEM_freeN(objects);
return OPERATOR_FINISHED;
return totobjects ? OPERATOR_FINISHED : OPERATOR_CANCELLED;
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:@ -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.
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.
See
sculpt_gesture_apply
below. I addinit
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.
@ -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.
@ -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
benullptr
? And how does this PR change that this is now suddenly possible?Because I added
init
and changed operators that can use it to use it instead of begin.@ -722,0 +737,4 @@
return true;
}
static int sculpt_gesture_apply_and_free(bContext *C,
This should document what it returns.
@ -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.
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.
@ -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, thereturn true
seems entirely unnecessary.This PR changes the
op.sculpt_gesture_begin
toop.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?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.The idea was to distinguish branching between types of sculpt tools, and the logic for the check. Basically the logic here is:
Edit: I think this actually can be replaced with a switch too, except it will break the encapsulation of all those
is
functions.ac212c675c
to92437c113e
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.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".
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 theindex
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.
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.
May I suggest an attitude change?
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:
This hasn't been addressed yet.
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.
It's also not clear to me how exactly do you want to split this:
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?
92437c113e
to2290923f21
paint_mask.cc
to be handled later: they are all just to add checks tosculpt.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 itBKE_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 functionarmature_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 the0
index would mean. Maybe just add a\param index Index of the key to retrieve, where index 0 is the base shape
. The fact thatkey
can benull
can then also be documented in a\param
.In
source/blender/blenkernel/intern/DerivedMesh.cc
:if (clmd && clmd->sim_parms->shapekey_rest)
; sincekey
can benull
, the addition of the check onclmd->sim_parms->shapekey_rest
seems unnecessary. Maybe you meant to checkclmd->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 returntrue
whenever the thing that is supposed to be editable doesn't exist. For example,ED_mesh_edit_verify_shape_key_is_editable
returnetrue
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:
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 returnedbool
means.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.
@ -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:I think both can be handled in one go by including the object name in the message.
2290923f21
tofcf2083f12
Renamed.
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.
Refactored.
Changed the message.
Gotcha.
Thanks, I feel that the code is significantly easier to follow now :)
👍