Shape Key editing: propagate updates through basis chains #105422

Closed

Alexander Gavrilov wants to merge 1 commits from angavrilov:pr-shapekey-propagate into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Alexander Gavrilov commented

2023-03-03 23:14:20 +01:00

Member

It is possible to organize shape keys into a tree through the
reference key setting. Mesh editing and sculpting a reference
key is supposed to update all its children, but this was only
done for one level of dependencies.

This adds a function that finds all dependent keys through
a breadth first search (this avoids any issues with cycles),
and updates the relevant code to use it.

The attached file can be used for testing: it has an object with three shape keys that form a dependency chain.

Without this patch editing the Basis (though sculpt or edit mode) and then enabling the third key cancels out the new changes because the modifications were not propagated.

It is possible to organize shape keys into a tree through the reference key setting. Mesh editing and sculpting a reference key is supposed to update all its children, but this was only done for one level of dependencies. This adds a function that finds all dependent keys through a breadth first search (this avoids any issues with cycles), and updates the relevant code to use it. ----------- The attached file can be used for testing: it has an object with three shape keys that form a dependency chain. Without this patch editing the Basis (though sculpt or edit mode) and then enabling the third key cancels out the new changes because the modifications were not propagated.

test-shapekey-chain-update.blend

100 KiB

Alexander Gavrilov added the

Sculpt, Paint & Texture

labels 2023-03-03 23:14:21 +01:00

Alexander Gavrilov requested review from Campbell Barton 2023-03-03 23:15:05 +01:00

Alexander Gavrilov requested review from Bastien Montagne 2023-03-03 23:15:13 +01:00

Alexander Gavrilov requested review from Joseph Eagar 2023-03-03 23:15:27 +01:00

Hans Goudey changed title from ~~Shape Key editing: propagate updates through basis chains.~~ to Shape Key editing: propagate updates through basis chains

2023-03-03 23:19:10 +01:00

Bastien Montagne refused to review 2023-03-07 16:36:14 +01:00

Bastien Montagne added this to the Modeling project 2023-03-07 16:36:22 +01:00

Bastien Montagne commented

2023-03-07 16:36:30 +01:00

Owner

Not really active in the Mesh/Modelling area nowadays, so will remove myself from here.

Also in general it's better to assign a project to a PR, than directly reviewers, unless you have already talked about the patch with them and they agreed to review it.

Not really active in the Mesh/Modelling area nowadays, so will remove myself from here. Also in general it's better to assign a project to a PR, than directly reviewers, unless you have already talked about the patch with them and they agreed to review it.

Joseph Eagar commented

2023-03-10 00:48:38 +01:00

Member

Also removing myself. Might want to ping @howardt about it.

Also removing myself. Might want to ping @howardt about it.

Joseph Eagar refused to review 2023-03-10 00:48:45 +01:00

Hans Goudey requested changes 2023-04-06 16:18:11 +02:00

Hans Goudey left a comment

Member

I have no idea about the logic of this shape key stuff, but maybe I can help get some of the typical code cleanup stuff out of the way.

I have no idea about the logic of this shape key stuff, but maybe I can help get some of the typical code cleanup stuff out of the way.

source/blender/blenkernel/BKE_key.h Outdated

						
				@ -182,2 +182,4 @@

				bool BKE_keyblock_is_basis(const struct Key *key, int index);

				/**

				 * Returns an array containing true for every key that has this one as basis.

Hans Goudey commented

2023-04-06 16:15:17 +02:00

Member

an array -> an allocated array

Would just help to write down that it's supposed to be freed

`an array` -> `an allocated array` Would just help to write down that it's supposed to be freed

angavrilov marked this conversation as resolved

source/blender/blenkernel/intern/key.cc Outdated

						
				@ -2594,2 +2594,4 @@

				  return false;

				}

				bool *BKE_keyblock_get_dependent_keys(const struct Key *key, int index)

Hans Goudey commented

2023-04-06 16:17:18 +02:00

Member

const struct Key *key, int index -> const Key *key, const int index

`const struct Key *key, int index` -> `const Key *key, const int index`

angavrilov marked this conversation as resolved

source/blender/blenkernel/intern/key.cc Outdated

						
				@ -2596,0 +2596,4 @@

				bool *BKE_keyblock_get_dependent_keys(const struct Key *key, int index)

				{

				  /* Simple checks. */

Hans Goudey commented

2023-04-06 16:13:13 +02:00

Member

I don't think this Simple checks. comment is adding anything here. Such early return condition checking is not exactly out of the ordinary, and the fact that they're "simple" is already obvious from looking at the code.

I don't think this `Simple checks.` comment is adding anything here. Such early return condition checking is not exactly out of the ordinary, and the fact that they're "simple" is already obvious from looking at the code.

angavrilov marked this conversation as resolved

source/blender/blenkernel/intern/key.cc Outdated

						
				@ -2596,0 +2608,4 @@

				  }

				  /* Seed the table with the specified key. */

				  bool *marked = static_cast<bool *>(MEM_callocN(sizeof(bool) * count, __func__));

Hans Goudey commented

2023-04-06 16:12:10 +02:00

Member

This is a C++ file, might as well use Array or BitVector here

This is a C++ file, might as well use `Array` or `BitVector` here

Alexander Gavrilov commented

2023-07-14 19:42:09 +02:00

Author

Member

It's returned to the function caller, which still includes C code.

It's returned to the function caller, which still includes C code.

source/blender/blenkernel/intern/key.cc Outdated

						
				@ -2596,0 +2621,4 @@

				    updated = 0;

				    for (i = 0, kb = static_cast<const KeyBlock *>(key->block.first); kb; i++, kb = kb->next) {

Hans Goudey commented

2023-04-06 16:14:38 +02:00

Member

Use the LISTBASE_FOREACH_INDEX macro

Use the `LISTBASE_FOREACH_INDEX` macro

angavrilov marked this conversation as resolved

source/blender/blenkernel/intern/key.cc Outdated

						
				@ -2596,0 +2634,4 @@

				    MEM_freeN(marked);

				    return nullptr;

				  }

				  else {

Hans Goudey commented

2023-04-06 16:11:48 +02:00

Member

else after return

else after return

angavrilov marked this conversation as resolved

source/blender/bmesh/intern/bmesh_mesh_convert.cc Outdated

						
				@ -852,2 +855,3 @@

				  LISTBASE_FOREACH (KeyBlock *, currkey, &key->block) {

				  int currkey_i;

Hans Goudey commented

2023-04-06 16:14:07 +02:00

Member

I think this newline between the index declaration and the loop is just wasting space, it doesn't really help readability

I think this newline between the index declaration and the loop is just wasting space, it doesn't really help readability

angavrilov marked this conversation as resolved

source/blender/bmesh/intern/bmesh_mesh_convert.cc Outdated

						
				@ -951,6 +955,7 @@ static void bm_to_mesh_shape(BMesh *bm,

				  if (ofs) {

				    MEM_freeN(ofs);

				    MEM_freeN(dependent);

Hans Goudey commented

2023-04-06 16:16:46 +02:00

Member

Are ofs and dependent always allocated together? That doesn't seem obvious looking at the code (though it totally might be the case!). A simple MEM_SAFE_FREE(dependent); at the end would be a little simpler though.

Are `ofs` and `dependent` always allocated together? That doesn't seem obvious looking at the code (though it totally might be the case!). A simple `MEM_SAFE_FREE(dependent);` at the end would be a little simpler though.

angavrilov marked this conversation as resolved

Alexander Gavrilov force-pushed pr-shapekey-propagate from 7cfa71a2ff to dca49a5bff

2023-07-14 19:39:33 +02:00

Compare

Alexander Gavrilov requested review from Hans Goudey 2023-07-14 20:33:06 +02:00

Brecht Van Lommel approved these changes 2023-07-18 14:17:19 +02:00

Brecht Van Lommel left a comment

Owner

Looks good to me.

Looks good to me.

Alexander Gavrilov closed this pull request

2023-07-18 15:32:31 +02:00

Alexander Gavrilov commented

2023-07-18 15:35:27 +02:00

Author

Member

Forgot to add the Pull Request line to d32748cdf4.

Forgot to add the Pull Request line to d32748cdf47dd20ea.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

5 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#105422

No description provided.