blender/blender
162
514
Fork 780
Code Issues 6.8k Pull Requests 928 Projects 19 Wiki Activity

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

New Issue
Closed
opened 2023-03-02 14:47:01 +01:00 by Alexander Gavrilov · 21 comments
Alexander Gavrilov commented 2023-03-02 14:47:01 +01:00
Member
Copy Link

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.
👍 1 ❤️ 1
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
Alexander Gavrilov commented 2023-03-02 15:01:17 +01:00
Author
Member
Copy Link

@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.
Demeter Dzadik commented 2023-03-02 16:32:57 +01:00
Member
Copy Link

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.
Sybren A. Stüvel commented 2023-03-07 14:13:35 +01:00
Member
Copy Link

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.
Alexander Gavrilov commented 2023-03-07 14:38:10 +01:00
Author
Member
Copy Link

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.
Sybren A. Stüvel commented 2023-03-07 14:52:15 +01:00
Member
Copy Link

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.
Alexander Gavrilov commented 2023-03-07 15:17:36 +01:00
Author
Member
Copy Link

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.
Sybren A. Stüvel commented 2023-03-07 17:18:56 +01:00
Member
Copy Link

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.
Alexander Gavrilov commented 2023-03-20 14:40:27 +01:00
Author
Member
Copy Link

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/
Alexander Gavrilov commented 2023-03-30 15:35:48 +02:00
Author
Member
Copy Link

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?
Demeter Dzadik commented 2023-03-31 14:48:25 +02:00
Member
Copy Link

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
Alexander Gavrilov commented 2023-03-31 16:19:15 +02:00
Author
Member
Copy Link

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.
Demeter Dzadik commented 2023-03-31 20:04:27 +02:00
Member
Copy Link

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
Sybren A. Stüvel commented 2023-04-11 16:08:54 +02:00
Member
Copy Link

@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.
Demeter Dzadik commented 2023-04-13 17:40:46 +02:00
Member
Copy Link

Is my testing not sufficient? 👀

Is my testing not sufficient? 👀
Sybren A. Stüvel commented 2023-04-14 12:20:49 +02:00
Member
Copy Link

@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.
Demeter Dzadik commented 2023-05-01 15:05:00 +02:00
Member
Copy Link

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?
Alexander Gavrilov commented 2023-07-15 10:46:21 +02:00
Author
Member
Copy Link

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.
Nika Kutsniashvili commented 2023-07-15 12:37:43 +02:00
Contributor
Copy Link

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.
Alexander Gavrilov commented 2023-08-09 15:37:43 +02:00
Author
Member
Copy Link

@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.
Alexander Gavrilov commented 2023-08-23 12:40:05 +02:00
Author
Member
Copy Link

@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.
❤️ 1
Alexander Gavrilov commented 2024-01-22 21:22:53 +01:00
Author
Member
Copy Link

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 Branch/Tag Specified
Branches Tags
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset System
Asset datablocks, libraries, browser and shelf

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Code Documentation
Code comments, Python/RNA API descriptions

  
Interest
Collada

  
Interest
Compatibility
Backward and forward 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
Wayland windowing on Unix

  
Interest
Workbench

  
Interest: X11

Xorg/X11 windowing

  
Legacy
Asset Browser Project

Archived

  
Legacy
Blender 2.8 Project

Archived

  
Legacy
Milestone 1: Basic, Local Asset Browser

Archived

  
Legacy
OpenGL Error

Archived

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Related to security, see policy: https://developer.blender.org/docs/handbook/bug_reports/vulnerability_reports/

  
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

Archived

  
Type
Report

Archived

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
User Interface
Assignees
Clear assignees
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.
Delete Branch "%!s()"

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?