Page MenuHome

Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
ClosedPublic

Authored by Alexander Gavrilov (angavrilov) on Aug 9 2018, 10:57 AM.

Details

Reviewers
Joshua Leung (aligorith)
Brecht Van Lommel (brecht)
Maniphest Tasks
T56268: B-Bone rest pose is not drawn correctly in Edit Mode
Commits
rBSaa5993f6236e: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBS5a005ddd02e0: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBS50323a02b572: Move B-Bone custom handle settings to Edit mode.
rBS7e5710d47c0d: Move B-Bone custom handle settings to Edit mode.
rBS38f3534885c7: Move B-Bone custom handle settings to Edit mode.
rBSaa6253be70df: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBS3a7e33529a19: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBS61a24c799b28: Move B-Bone custom handle settings to Edit mode.
rBS433fa2bb3e49: Move B-Bone custom handle settings to Edit mode.
rBSdb71ed89fb65: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBS471389ee5d63: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBS6f1977d9e307: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBS6eff679ec9a9: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rB61a24c799b28: Move B-Bone custom handle settings to Edit mode.
rBSdd748080e022: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBS7d61af1bfc6b: Move B-Bone custom handle settings to Edit mode.
rBS65f77ccea164: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rB65f77ccea164: Fix T56268: display the correct rest shape for B-Bones in Edit Mode.
rBSd9e201223802: Move B-Bone custom handle settings to Edit mode.
Summary

The rest shape of B-Bones is actually affected by custom handles or
the default connected parent/child mechanism. Ignoring these effects
thus leads to the edit mode shape being different from the actual
rest pose.

This splits the b_bone_spline_setup function that is used to compute
the correct rest and pose shape from pose channels into two parts,
and applies the data structure independent half to edit mode.

In order to access the custom handle settings in Edit Mode, they are
moved to Bone and EditBone, while the bPoseChannel fields are downgraded
in status to a cache for performance. Also, instead of flags, introduce
an enum to specify the handle operation modes, so that new ones could
be added later.

Diff Detail

Repository
rB Blender

Event Timeline

Rebased and resolved conflicts.

Rebased and fixed conflict.

Rebased & checked UI is now properly aligned.

Brecht Van Lommel (brecht) requested changes to this revision.Sep 26 2018, 8:18 PM

This seems generally fine, but I need to seriously test this on production files still.

Have you done testing with linked characters, proxies, some production files?

source/blender/blenkernel/BKE_armature.h
147 ↗(On Diff #11962)

Code style: start with capital and end with . for comments, here and everywhere in the C code.

source/blender/blenloader/intern/readfile.c
3592–3604
3593

Any reason to have a separate loop for this rather than doing it in the one above?

3594–3597

Can we deprecate pchan->bbone_next and pchan->bbone_prev instead of trying to do this syncing? The only reason I see to keep these around is for versioning, otherwise it's just another thing that risks getting out of sync.

3599

Don't bother using UNLIKELY in non-performance critical code like this.

source/blender/blenloader/intern/versioning_280.c
868

Use the DNA_struct_elem_find trick to avoid applying this versioning twice on already saved files.

Strictly speaking we are breaking backwards compatibility here in case an armature is used by multiple objects. I think that is rare though, and not important.

source/blender/editors/include/ED_armature.h
114

I think this is quite error prone and can get out of sync, I would rather not have this flag and always do more expensive checks.

source/blender/makesrna/intern/rna_armature.c
465–466

Code style: always use {}.

This revision now requires changes to proceed.Sep 26 2018, 8:18 PM

I only tested with the specially prepared single-file test from the bug.

source/blender/blenloader/intern/readfile.c
3593

This check requires the pchan->bone links be valid for all pchans.

3594–3597

The fields still remain in use by all code that actually evaluates the bones during animation. You can easily go from bPoseChannel to Bone, but the reverse requires a lookup by name, so channel to channel links are way faster. Channels also link to e.g. their parent, even though it is similarly redundant.

It is possible that this whole loop is just being paranoid though, and rebuilds that are triggered by preceeding checks, and during the actual blender use, would be enough to ensure everything is in sync. There is no similar check to verify that pchan->parent->bone == pchan->bone->parent after all.

source/blender/blenloader/intern/versioning_280.c
868

This is in the 'after link' function (because it needs valid pchan->bone pointers), it doesn't seem to have the fd parameter...

Unfortunatly breaking compatibility is necessary if you want the correct rest pose to display in Edit mode - making the handles a Pose mode setting was a design mistake.

source/blender/editors/include/ED_armature.h
114

Well, I suppose deleting a bone is not so frequent that O(n^2) would be a problem.

I only tested with the specially prepared single-file test from the bug.

Ok, then you need to test it with more files, because this is not a trivial change.

source/blender/blenloader/intern/readfile.c
3594–3597

Ok, that seems acceptable then.

source/blender/blenloader/intern/versioning_280.c
868

Ok, if you can't do DNA_struct_elem_find then please bump the subversion and test for that.

source/blender/editors/include/ED_armature.h
114

Right.

I verified that this has no impact on the Spring project rigs and animations, so if the few minor things I noted get fixed this should be good to go.

One thing is that I don't really understand how version checks in do_versions work with linked files (especially mix of files with different versions, in the after link processing stage), so is there any trick to handling it properly? Also, just in case, comparing ob->id.lib is the right way to check if IDs come from the same file?

One thing is that I don't really understand how version checks in do_versions work with linked files (especially mix of files with different versions, in the after link processing stage), so is there any trick to handling it properly?

First all .blend files are loaded individually and get regular versioning. Then all the datablocks are added together into a single database, and the after linking versioning is then applied on that single database.

I think what you did is fine, only copying the bbone data if the object and armature are from the same library. That way the result will be identical as if you opened all the library .blends and saved them with the latest version.

Also, just in case, comparing ob->id.lib is the right way to check if IDs come from the same file?

Yes.

Code style and other fixes.

Alexander Gavrilov (angavrilov) marked 15 inline comments as done.Sep 29 2018, 6:00 PM
Alexander Gavrilov (angavrilov) added inline comments.
source/blender/blenloader/intern/readfile.c
3549

Found an actual bug here: for some reason it wasn't rebuilding library-linked Armature objects, which in turn use Armature data from another library. You can actually crash master by exploiting this, while 2.8 survives but builds an incorrect depsgraph.

3594–3597

I decided to remove this complex rebuild check and try to rely on other causes for rebuild being sufficient. The do_versions code also tags rebuild now.

Brecht Van Lommel (brecht) requested changes to this revision.Oct 3 2018, 8:20 PM

Spring tests seem fine, but found some issues in the code.

source/blender/blenloader/intern/versioning_280.c
869

This should be 25.

905

I don't think this line is necessary, there does not exist a depsgraph at this stage. It's not valid to interact with the depsgraph in file loading in any case.

906

That function does:

pose->flag |= POSE_RECALC;
/* Depsgraph components depends on actual pose state,
 * if pose was changed depsgraph is to be updated as well.
 */
DEG_relations_tag_update(bmain);

Only setting that flag makes sense in this context.

This revision now requires changes to proceed.Oct 3 2018, 8:20 PM
Alexander Gavrilov (angavrilov) marked 3 inline comments as done.Oct 4 2018, 6:18 PM
Alexander Gavrilov (angavrilov) added inline comments.
source/blender/blenloader/intern/versioning_280.c
905

There is other read code interacting with depsgraph, including the place I originally copied this from, but fixing it is out of the scope of this patch.

This revision is now accepted and ready to land.Oct 4 2018, 6:22 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
Alexander Gavrilov (angavrilov) marked an inline comment as done.