Mitigate the risk of wrong shape key selection in sculpt and edit mode. #105373

Closed
opened 2023-03-02 14:47:01 +01:00 by Alexander Gavrilov · 21 comments

The Issue

A major weakness of any layered editing system is that it matters which layer is selected for editing operations, but this selection is not apparent in the actual editing space, because layer effects are by definition combined into a single result for display.

For instance, unless the single key mode is turned on, the effects of all relative shape keys are summed up into a final shape, which doesn't change no matter which one is selected.

This means that it is easy to apply editing operations to the wrong selected layer, including a possibility that the undo stack will be exhausted before the mistake is noticed.

An additional risk factor is that shape influence sliders are located in the same list, and a hurried misclick on a slider can change selection.

Proposed Solution

The industry standard way to mitigate this risk is layer locking: layers that are not currently being worked on can be locked by the user, in which case all editing operations that would affect that layer will instead abort with an error.

Blender already supports this with Vertex Groups and weight painting - even though with groups it is more obvious which one is selected, and locking has additional effects related to normalization.

I propose adding this feature for shape keys (implemented in #104463):

screenshot-shapekey-locks.png

Since it is a standard approach that is used by any advanced image editor, most users would likely be already familiar with the concept. However there are some details in its application that need discussing.

Scope Issue

The primary design question is which operations should react to layers being locked, balancing mainly implementation complexity and the effectiveness and convenience of the feature.

In implementing the prototype I decided that the main criteria should be:

  • Operations that affect vertex positions, and thus shape keys.

  • Operations where the shape key selection matters:

    For example, applying object mode transformation updates all shape keys. Thus the selection doesn't really matter, and respecting locks would inconvenience the user that is working on the scene layout, rather than individual object details.

  • Deform-only operations:

    Even though selection may affect the final vertex positions, operations that change topology necessarily change all shape keys. For that reason, topology manipulation also belongs in an earlier stage of the design pipeline than shape key creation, even though it may be necessary to do it later sometimes for various reasons. Finally, topology change is seldom not accompanied by using transformation operators that respect the locks.

Current Scope

This is the current scope of the checks in the prototype:

  • All Edit Mode transformation operations.

    Specifically, everything invoking the generic transform system initialization code.

  • All Sculpt brushes that affect vertices, and not colors or masks.

    Specifically, everything invoking the generic brush stroke initialization code.

  • Smooth and Laplacian Smooth operators in Edit Mode.

  • Blend From Shape operator in Edit Mode.

  • Propagate To All Shapes operator in Edit Mode.

    This one is a bit of a problem, because technically shape selection doesn't matter here: it snaps all shape keys to the current position. However currently I implemented a check to abort if any shape key is locked on the principle that it is a shape key operation. Maybe it should only check the main Basis, or work in some other way.

  • Mirror Shape and deleting shape keys.

  • A few other operators...

This spreadsheet containts an exact listing of which operators should and actually do check for locks.

Discussion

It is necessary to discuss whether the main criteria, or this specific list needs changing. For instance, I'm not confident that I remembered all operations that match the criteria for the checks.

Another minor design point is whether attempting to edit a locked layer should be an error (shows a message under the cursor) or a warning (appears in the status line).

Since the checks are done by operators rather than low-level code, add-ons would have to implement them too where relevant. I wonder how many would be affected, and whether some API additions to help them do that easily would be necessary.

There have also been some questions raised in #104463, like:

  • 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.
# The Issue A major weakness of any layered editing system is that it matters which layer is selected for editing operations, but this selection is not apparent in the actual editing space, because layer effects are by definition combined into a single result for display. For instance, unless the single key mode is turned on, the effects of all relative shape keys are summed up into a final shape, which doesn't change no matter which one is selected. This means that it is easy to apply editing operations to the wrong selected layer, including a possibility that the undo stack will be exhausted before the mistake is noticed. An additional risk factor is that shape influence sliders are located in the same list, and a hurried misclick on a slider can change selection. # Proposed Solution The industry standard way to mitigate this risk is layer locking: layers that are not currently being worked on can be locked by the user, in which case all editing operations that would affect that layer will instead abort with an error. Blender already supports this with Vertex Groups and weight painting - even though with groups it is more obvious which one is selected, and locking has additional effects related to normalization. I propose adding this feature for shape keys (implemented in #104463): ![screenshot-shapekey-locks.png](/attachments/683366e0-1077-4beb-8bfe-3264acfbc229) Since it is a standard approach that is used by any advanced image editor, most users would likely be already familiar with the concept. However there are some details in its application that need discussing. ## Scope Issue The primary design question is which operations should react to layers being locked, balancing mainly implementation complexity and the effectiveness and convenience of the feature. In implementing the prototype I decided that the main criteria should be: * Operations that affect vertex positions, and thus shape keys. * Operations where the shape key selection matters: For example, applying object mode transformation updates all shape keys. Thus the selection doesn't really matter, and respecting locks would inconvenience the user that is working on the scene layout, rather than individual object details. * Deform-only operations: Even though selection may affect the final vertex positions, operations that change topology necessarily change all shape keys. For that reason, topology manipulation also belongs in an earlier stage of the design pipeline than shape key creation, even though it may be necessary to do it later sometimes for various reasons. Finally, topology change is seldom not accompanied by using transformation operators that respect the locks. ### Current Scope This is the current scope of the checks in the prototype: * All Edit Mode <u>transformation</u> operations. Specifically, everything invoking the generic transform system initialization code. * All <u>Sculpt brushes</u> that affect vertices, and not colors or masks. Specifically, everything invoking the generic brush stroke initialization code. * <u>Smooth</u> and <u>Laplacian Smooth</u> operators in Edit Mode. * <u>Blend From Shape</u> operator in Edit Mode. * <u>Propagate To All Shapes</u> operator in Edit Mode. This one is a bit of a problem, because technically shape selection doesn't matter here: it snaps all shape keys to the current position. However currently I implemented a check to abort if any shape key is locked on the principle that it is a shape key operation. Maybe it should only check the main Basis, or work in some other way. * <u>Mirror Shape</u> and deleting shape keys. * A few other operators... This [spreadsheet](https://docs.google.com/spreadsheets/d/1DEm5B3847sn8fmuux_hp4WDt4hFFLB6bVXWH1Fy3rCI/edit?usp=sharing) containts an exact listing of which operators should and actually do check for locks. # Discussion It is necessary to discuss whether the main criteria, or this specific list needs changing. For instance, I'm not confident that I remembered all operations that match the criteria for the checks. Another minor design point is whether attempting to edit a locked layer should be an error (shows a message under the cursor) or a warning (appears in the status line). Since the checks are done by operators rather than low-level code, add-ons would have to implement them too where relevant. I wonder how many would be affected, and whether some API additions to help them do that easily would be necessary. There have also been some questions raised in #104463, like: > - 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.
Alexander Gavrilov added the
Type
Design
label 2023-03-02 14:47:01 +01:00
Alexander Gavrilov added this to the User Interface project 2023-03-02 14:59:20 +01:00
Author
Member

@ideasman42 @dr.sybren @Mets @mont29 @JosephEagar pinging everybody who was involved in the pull request.

@ideasman42 @dr.sybren @Mets @mont29 @JosephEagar pinging everybody who was involved in the pull request.
Member

Here's how I think about it: The behaviour of the lock should depend on whether shape keys are used in Relative mode or not.

If shape keys are relative, the user's mental model of a shape key is as a delta, so when they use the lock, they want to lock that delta. So any operation that would affect both the shape and its basis equally, should be allowed to do so, because it's not changing the delta.

If shape keys aren't relative, then I think the only thing that should affect the underlying data of locked shape keys is applying object transforms, since that already involves offsetting all vertex coordinates, and the coordinates of absolute shape keys should be treated equally to the coordinates of the base mesh.

I think operators to batch lock/unlock things on all selected objects is a good idea, but I also wish this was done with a generic operator for all mesh attributes, instead of having separate but identical operators for different pieces of data.

Here's how I think about it: The behaviour of the lock should depend on whether shape keys are used in Relative mode or not. If shape keys are relative, the user's mental model of a shape key is as a delta, so when they use the lock, they want to lock that delta. So any operation that would affect both the shape and its basis equally, should be allowed to do so, because it's not changing the delta. If shape keys aren't relative, then I think the only thing that should affect the underlying data of locked shape keys is applying object transforms, since that already involves offsetting all vertex coordinates, and the coordinates of absolute shape keys should be treated equally to the coordinates of the base mesh. I think operators to batch lock/unlock things on all selected objects is a good idea, but I also wish this was done with a generic operator for all mesh attributes, instead of having separate but identical operators for different pieces of data.

I think @Mets raises a good point about relative vs. absolute.

For example, applying object mode transformation updates all shape keys. Thus the selection doesn't really matter, and respecting locks would inconvenience the user that is working on the scene layout, rather than individual object details.

This could cause other frustration, though, where users expect shapekeys to be untouched when they are locked.

@ideasman42 @dr.sybren @Mets @mont29 @JosephEagar pinging everybody who was involved in the pull request.

I think this should be discussed by artists who actually would interact with the feature, before pulling in more developers.

I think @Mets raises a good point about relative vs. absolute. > For example, applying object mode transformation updates all shape keys. Thus the selection doesn't really matter, and respecting locks would inconvenience the user that is working on the scene layout, rather than individual object details. This could cause other frustration, though, where users expect shapekeys to be untouched when they are locked. > @ideasman42 @dr.sybren @Mets @mont29 @JosephEagar pinging everybody who was involved in the pull request. I think this should be discussed by artists who actually would interact with the feature, before pulling in more developers.
Author
Member

This could cause other frustration, though, where users expect shapekeys to be untouched when they are locked.

Define "untouched". For relative keys, like @Mets said, the important thing is the difference between them. Arguably, offsetting all absolute ones by the same amount is also OK - you'd also use the absolute ones because of the difference, it's just accessed differently. Like I phrased it, the main point is "does the active key selection matter".

I think this should be discussed by artists who actually would interact with the feature, before pulling in more developers.

You know, I begin feeling kinda annoyed that my own opinion seems to be completely discounted like I know nothing if I don't ask some abstract "artists". This feature comes from an absolutely specific experience of my own, and I fully implemented it without consulting anyone because I wanted to have it immediately, and the idea is standard.

Also, the whole reason this task was created in the first place is that someone in the discussion thread there wanted a design task. For me, I started with the pull request, and this task continues the discussion started there in a different location.

> This could cause other frustration, though, where users expect shapekeys to be untouched when they are locked. Define "untouched". For relative keys, like @Mets said, the important thing is the difference between them. Arguably, offsetting all absolute ones by the same amount is also OK - you'd also use the absolute ones because of the difference, it's just accessed differently. Like I phrased it, the main point is "does the active key selection matter". > I think this should be discussed by artists who actually would interact with the feature, before pulling in more developers. You know, I begin feeling kinda annoyed that my own opinion seems to be completely discounted like I know nothing if I don't ask some abstract "artists". This feature comes from an absolutely specific experience of my own, and I fully implemented it without consulting anyone because I wanted to have it immediately, and the idea is standard. Also, the whole reason this task was created in the first place is that someone in the discussion thread there wanted a design task. For me, I started with the pull request, and this task continues the discussion started there in a different location.

You know, I begin feeling kinda annoyed that my own opinion seems to be completely discounted like I know nothing if I don't ask some abstract "artists". This feature comes from an absolutely specific experience of my own, and I fully implemented it without consulting anyone because I wanted to have it immediately, and the idea is standard.

Your opinion and experience is not doubted at all. I'd be wanting the same thing from myself or Sergey when either of us proposes something that influences artist workflows.

A single person always has a limited point of view, that's just a fact of life and of being human.

To get more concrete, in 7728bfd4c4 your opinion was that the special meaning of Local Space for parentless Objects could be removed. We had to revert that change in a corrective release because the removal actually broke people's files. These things happen, we're all human, and that's fine. For me it was a good lesson in being more inclusive in pulling in artists and other users of Blender while discussing changes, though.

So when a design task comes in that doesn't mention whether anyone else was involved in the design, or whether this is just the personal idea of one developer, it's raising a red flag for me.

> You know, I begin feeling kinda annoyed that my own opinion seems to be completely discounted like I know nothing if I don't ask some abstract "artists". This feature comes from an absolutely specific experience of my own, and I fully implemented it without consulting anyone because I wanted to have it immediately, and the idea is standard. Your opinion and experience is not doubted at all. I'd be wanting the same thing from myself or Sergey when either of us proposes something that influences artist workflows. A single person always has a limited point of view, that's just a fact of life and of being human. To get more concrete, in 7728bfd4c45c634ba6b62e149176425ec5779945 your opinion was that the special meaning of Local Space for parentless Objects could be removed. We had to revert that change in a corrective release because the removal actually broke people's files. These things happen, we're all human, and that's fine. For me it was a good lesson in being more inclusive in pulling in artists and other users of Blender while discussing changes, though. So when a design task comes in that doesn't mention whether anyone else was involved in the design, or whether this is just the personal idea of one developer, it's raising a red flag for me.
Author
Member

Your opinion and experience is not doubted at all. I'd be wanting the same thing from myself or Sergey when either of us proposes something that influences artist workflows.

Sorry, I overreacted a bit. 🙂

Like I said, from my point of view this was not "pulling in more developers", it was pinging everybody already involved with the original PR that there is a new place for discussion.

> Your opinion and experience is not doubted at all. I'd be wanting the same thing from myself or Sergey when either of us proposes something that influences artist workflows. Sorry, I overreacted a bit. 🙂 Like I said, from my point of view this was not "pulling in more developers", it was pinging everybody already involved with the original PR that there is a new place for discussion.

Your opinion and experience is not doubted at all. I'd be wanting the same thing from myself or Sergey when either of us proposes something that influences artist workflows.

Sorry, I overreacted a bit. 🙂

Apology accepted.

Like I said, from my point of view this was not "pulling in more developers", it was pinging everybody already involved with the original PR that there is a new place for discussion.

Then my argument expands to that PR as well.

These kind of workflow changes need to be discussed with artists. Since you have a PR for a prototype already, it's trivial to add a @blender-bot package comment and provide a test build so that people can actually play around with the proposed changes.

> > Your opinion and experience is not doubted at all. I'd be wanting the same thing from myself or Sergey when either of us proposes something that influences artist workflows. > > Sorry, I overreacted a bit. 🙂 Apology accepted. > Like I said, from my point of view this was not "pulling in more developers", it was pinging everybody already involved with the original PR that there is a new place for discussion. Then my argument expands to that PR as well. These kind of workflow changes need to be discussed with artists. Since you have a PR for a prototype already, it's trivial to add a `@blender-bot package` comment and provide a test build so that people can actually play around with the proposed changes.
Author
Member

Since you have a PR for a prototype already, it's trivial to add a @blender-bot package comment and provide a test build so that people can actually play around with the proposed changes.

Now there is a build available: https://builder.blender.org/download/patch/PR104463/

> Since you have a PR for a prototype already, it's trivial to add a `@blender-bot package` comment and provide a test build so that people can actually play around with the proposed changes. Now there is a build available: https://builder.blender.org/download/patch/PR104463/
Author
Member

I think this should be discussed by artists who actually would interact with the feature, before pulling in more developers.

@dr.sybren So, before this is also closed for inactivity, who exactly are these artists who should discuss this before any progress can be made?

> I think this should be discussed by artists who actually would interact with the feature, before pulling in more developers. @dr.sybren So, before this is also closed for inactivity, who exactly are these artists who should discuss this before any progress can be made?
Member

Sorry I missed the link to the build, I'd gladly test the patch, but now the link doesn't seem to have any builds anymore? :S

Sorry I missed the link to the build, I'd gladly test the patch, but now the link doesn't seem to have any builds anymore? :S
Author
Member

I have no idea why they made it expire so fast, but now you have to go into All Archived Builds and search for the PR id.

I have no idea why they made it expire so fast, but now you have to go into All Archived Builds and search for the PR id.
Member

Alright, I gave it a whirl!

I tried transform operators (G,R,S), gizmos, operators like smooth and blend from shape, sculpt mode. All fail with a useful error message if the active shape key is locked. Applying object transforms works as I'd expect. Lock/Unlock all operators work and are nice to have.

Important bit: I found one operator that isn't getting blocked: Mirror Shape Key (bpy.ops.object.shape_key_mirror). So if that gets fixed, it's good to go as far as I'm concerned. I think I will use this feature.

Controversial idea: Don't allow removing locked shape keys, force user to unlock it first? I'm only mentioning this because earlier today I accidentally deleted some shape keys, would've been handy to have a child-lock on those, haha.

Bonus unrelated weird thing I found: Apparently sculpt mode doesn't work on absolute shape keys. 🤷

Alright, I gave it a whirl! I tried transform operators (G,R,S), gizmos, operators like smooth and blend from shape, sculpt mode. All fail with a useful error message if the active shape key is locked. Applying object transforms works as I'd expect. Lock/Unlock all operators work and are nice to have. **Important bit**: I found one operator that isn't getting blocked: Mirror Shape Key (bpy.ops.object.shape_key_mirror). So if that gets fixed, it's good to go as far as I'm concerned. I think I will use this feature. Controversial idea: Don't allow removing locked shape keys, force user to unlock it first? I'm only mentioning this because earlier today I accidentally deleted some shape keys, would've been handy to have a child-lock on those, haha. Bonus unrelated weird thing I found: Apparently sculpt mode doesn't work on absolute shape keys. 🤷
Alexander Gavrilov added the
Module
Animation & Rigging
label 2023-04-07 12:13:34 +02:00

@dr.sybren So, before this is also closed for inactivity, who exactly are these artists who should discuss this before any progress can be made?

We have quite a busy module channel on Blender Chat. I'm sure you can find some people there who are interested in helping with testing.

> @dr.sybren So, before this is also closed for inactivity, who exactly are these artists who should discuss this before any progress can be made? We have quite a busy module channel on Blender Chat. I'm sure you can find some people there who are interested in helping with testing.
Member

Is my testing not sufficient? 👀

Is my testing not sufficient? 👀

@Mets your testing is super valuable! It's just that there seem to still be open design questions, and I didn't want you to be the only one to give feedback to try and answer those.

Another minor design point is whether attempting to edit a locked layer should be an error (shows a message under the cursor) or a warning (appears in the status line).

I would say it's a warning if the operation could continue (albeit partially as locked layers were skipped), and an error when nothing could be doneat all (because all layers were locked).

Since the checks are done by operators rather than low-level code, add-ons would have to implement them too where relevant. I wonder how many would be affected, and whether some API additions to help them do that easily would be necessary.

Without having looked at the code, my gut feeling is that such checks could indeed be done in some API function(s), and those would then be used by the operators.

@Mets your testing is super valuable! It's just that there seem to still be open design questions, and I didn't want you to be the only one to give feedback to try and answer those. > Another minor design point is whether attempting to edit a locked layer should be an error (shows a message under the cursor) or a warning (appears in the status line). I would say it's a warning if the operation could continue (albeit partially as locked layers were skipped), and an error when nothing could be doneat all (because all layers were locked). > Since the checks are done by operators rather than low-level code, add-ons would have to implement them too where relevant. I wonder how many would be affected, and whether some API additions to help them do that easily would be necessary. Without having looked at the code, my gut feeling is that such checks could indeed be done in some API function(s), and those would then be used by the operators.
Member

API convenience functions are always welcome! But I wonder what exactly would be necessary, since I can only think of 2 things: Checking if the active shape key is locked, and checking if all shape keys are locked.

But I think those can already be done in a single line each?
ob.active_shape_key.use_lock/is_locked/whatever it's called
all([sk.use_lock for sk in ob.data.shape_key.key_blocks])

Are there more helper functions that might be useful that I'm not thinking of?

API convenience functions are always welcome! But I wonder what exactly would be necessary, since I can only think of 2 things: Checking if the active shape key is locked, and checking if all shape keys are locked. But I think those can already be done in a single line each? `ob.active_shape_key.use_lock/is_locked/whatever it's called` `all([sk.use_lock for sk in ob.data.shape_key.key_blocks])` Are there more helper functions that might be useful that I'm not thinking of?
Author
Member

Important bit: I found one operator that isn't getting blocked: Mirror Shape Key (bpy.ops.object.shape_key_mirror). So if that gets fixed, it's good to go as far as I'm concerned. I think I will use this feature.

Controversial idea: Don't allow removing locked shape keys, force user to unlock it first? I'm only mentioning this because earlier today I accidentally deleted some shape keys, would've been handy to have a child-lock on those, haha.

Added checks to these in the pull request.

> **Important bit**: I found one operator that isn't getting blocked: Mirror Shape Key (bpy.ops.object.shape_key_mirror). So if that gets fixed, it's good to go as far as I'm concerned. I think I will use this feature. > > Controversial idea: Don't allow removing locked shape keys, force user to unlock it first? I'm only mentioning this because earlier today I accidentally deleted some shape keys, would've been handy to have a child-lock on those, haha. Added checks to these in the pull request.
Contributor

I stumbled upon this and since there seems to be artist feedback wanted I'll give one. I am currently facial animator in small studio and I work almost exclusively with shape keys.

Locking shape keys absolutely. Not only is accidentally sculpting on basis a problem but when you have dozens of shape keys its all a chaos and accidents waiting to happen.

Unticking shape keys gave me at first impression that I was indeed locking shape keys which lead to some problems. So having this clear distinction of what lock icon doea and what untick does is super helpful.

Sculpting on locked shape keys I think should give same type of error that mesh with no applied transform gives. But I would like if I knew always what I was sculpting on. More visible text, like "Auto Key is on".

One thing I beg you to add is tree UI for shape keys. I see progress being made for GP layers and armature collections and I cant emphasize enough how much we need that same UI for shape keys (and vertex groups too). Working on dozens of shape keys is incredibly chaotic without having a way to group them and collapse them. Shape Key collections dont need to have any functionality except you can disable and/or lock them all. This PR would be especially handy if it worked on collections. For example I have made my main pose and I take it for a review, and I get feedback and I start making corrective shape keys to tweak a little. But accidentally I sculpt on main pose shape keys and if I experimented a lot I cant delete it and get my base pose back. I have to throw it out and create base from scratch. This problem would be gone if I could group main pose shape keys and lock them together.

And since shape keys (and vertex groups) are part of rigs too it would help a lot if I could have similar hierarchies in those as in armature collections deform bones, would make things very clear if I have DEFORM_ARM group in armature, vertex groups and shape keys and I always know where to find what.

If there is something specific you want me to test in the build i'm glad to.

I stumbled upon this and since there seems to be artist feedback wanted I'll give one. I am currently facial animator in small studio and I work almost exclusively with shape keys. Locking shape keys absolutely. Not only is accidentally sculpting on basis a problem but when you have dozens of shape keys its all a chaos and accidents waiting to happen. Unticking shape keys gave me at first impression that I was indeed locking shape keys which lead to some problems. So having this clear distinction of what lock icon doea and what untick does is super helpful. Sculpting on locked shape keys I think should give same type of error that mesh with no applied transform gives. But I would like if I knew always what I was sculpting on. More visible text, like "Auto Key is on". One thing I beg you to add is tree UI for shape keys. I see progress being made for GP layers and armature collections and I cant emphasize enough how much we need that same UI for shape keys (and vertex groups too). Working on dozens of shape keys is incredibly chaotic without having a way to group them and collapse them. Shape Key collections dont need to have any functionality except you can disable and/or lock them all. This PR would be especially handy if it worked on collections. For example I have made my main pose and I take it for a review, and I get feedback and I start making corrective shape keys to tweak a little. But accidentally I sculpt on main pose shape keys and if I experimented a lot I cant delete it and get my base pose back. I have to throw it out and create base from scratch. This problem would be gone if I could group main pose shape keys and lock them together. And since shape keys (and vertex groups) are part of rigs too it would help a lot if I could have similar hierarchies in those as in armature collections deform bones, would make things very clear if I have DEFORM_ARM group in armature, vertex groups and shape keys and I always know where to find what. If there is something specific you want me to test in the build i'm glad to.
Author
Member

@Mets Are there more helper functions that might be useful that I'm not thinking of?

I'm not actually sure, I think I need to try adding checks to some addons. Any suggestions of ones with relevant operators?

In the C code I basically made these, with special versions for edit mode and sculpt, because they have their own saved references to the active key and I wanted to make sure to use those rather than the generic one.

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

bool ED_object_check_active_shape_key_unlocked(Object *ob, ReportList *reports);
bool ED_object_check_all_shape_keys_unlocked(Object *ob, ReportList *reports);

bool ED_sculpt_check_shape_key_unlocked(const Object *ob, ReportList *reports);

@Nika-Kutsniashvili If there is something specific you want me to test in the build i'm glad to.

I think the main question is whether there are any operations that should/shouldn't check the locks, but currently do the opposite - either intentionally (according to the decisions outlined above), or because they were missed.

Tree stuff is out of the scope of this discussion.

> @Mets Are there more helper functions that might be useful that I'm not thinking of? I'm not actually sure, I think I need to try adding checks to some addons. Any suggestions of ones with relevant operators? In the C code I basically made these, with special versions for edit mode and sculpt, because they have their own saved references to the active key and I wanted to make sure to use those rather than the generic one. ``` /** Checks if the currently active Edit Mode on the object is targeting a locked shape key. */ bool ED_object_check_edit_shape_key_unlocked(const Object *obedit, ReportList *reports); bool ED_object_check_active_shape_key_unlocked(Object *ob, ReportList *reports); bool ED_object_check_all_shape_keys_unlocked(Object *ob, ReportList *reports); bool ED_sculpt_check_shape_key_unlocked(const Object *ob, ReportList *reports); ``` > @Nika-Kutsniashvili If there is something specific you want me to test in the build i'm glad to. I think the main question is whether there are any operations that should/shouldn't check the locks, but currently do the opposite - either intentionally (according to the decisions outlined above), or because they were missed. Tree stuff is out of the scope of this discussion.
Author
Member

@Mets Are there more helper functions that might be useful that I'm not thinking of?

So, I had to add checks to a few operators implemented in python, and this is the helper function I came up with.

Basically, the main thing isn't that the check is hard to do, but that if you need to do it in multiple places you don't want to repeat even minor details like your checks being robust to None values, or the actual error message you emit if the check fails. So, both in python and in C code I encapsulate the actual check and emitting the error into one utility function. This way the caller just has to call the utility, and then return from the operator if it fails.

Now, one question is whether it is possible and/or reasonable to allow the python code to use the C implementations, or if this amount of duplication between languages is ok.


@dr.sybren @nathanvegdahl So, like discussed in the meeting, I made a list of operators that could potentially be relevant for this checks in a google docs spreadsheet.

I started with a list of all operators in blender, deleted clearly irrelevant groups like 'sequencer', and then categorized them based on the description and in some less cases testing. Clearly irrelevant operators within the selected groups (selection related, etc) I grayed out rather than deleting. The spreadsheet then calculates whether the operator must perform the checks based on the rules outlined in the top post, and I checked that they are indeed done.

Some 'topology modifying' operators do the checks coincidentally even though not required by the rules because they transition into a transformation operator that does the check, e.g. extrude; in this case you may have to undo the non-transform part that completes successfully. I also had to add explicit checks to the view3d.edit_mesh_* operators in python, because when the C operators they call fail, it causes an ugly exception.

I also had to add checks to quite a few C operators (especially in the curve/lattice areas) because they should do them according to the rules but didn't.

> @Mets Are there more helper functions that might be useful that I'm not thinking of? So, I had to add checks to a few operators implemented in python, and this is the [helper function](https://projects.blender.org/blender/blender/pulls/104463/files#diff-627cdb328b3a23ddb1d8943616c3b39588e0b40e) I came up with. Basically, the main thing isn't that the check is hard to do, but that if you need to do it in multiple places you don't want to repeat even minor details like your checks being robust to None values, or the actual error message you emit if the check fails. So, both in python and in [C code](https://projects.blender.org/blender/blender/pulls/104463/files#diff-b38e25ef158ddecc42b67946fa6b9f998094b0ee) I encapsulate the actual check and emitting the error into one utility function. This way the caller just has to call the utility, and then return from the operator if it fails. Now, one question is whether it is possible and/or reasonable to allow the python code to use the C implementations, or if this amount of duplication between languages is ok. --------- @dr.sybren @nathanvegdahl So, like discussed in the meeting, I made a list of operators that could potentially be relevant for this checks in a [google docs spreadsheet](https://docs.google.com/spreadsheets/d/1DEm5B3847sn8fmuux_hp4WDt4hFFLB6bVXWH1Fy3rCI/edit?usp=sharing). I started with a list of all operators in blender, deleted clearly irrelevant groups like 'sequencer', and then categorized them based on the description and in some less cases testing. Clearly irrelevant operators within the selected groups (selection related, etc) I grayed out rather than deleting. The spreadsheet then calculates whether the operator must perform the checks based on the rules outlined in the top post, and I checked that they are indeed done. Some 'topology modifying' operators do the checks coincidentally even though not required by the rules because they transition into a transformation operator that does the check, e.g. extrude; in this case you may have to undo the non-transform part that completes successfully. I also had to add explicit checks to the `view3d.edit_mesh_*` operators in python, because when the C operators they call fail, it causes an ugly exception. I also had to add checks to quite a few C operators (especially in the curve/lattice areas) because they should do them according to the rules but didn't.
Author
Member

The pull request has been committed, closing.

The pull request has been committed, closing.
Blender Bot added the
Status
Archived
label 2024-01-22 21:24:00 +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
4 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#105373
No description provided.