Fix T82758: Convert Proxy to Override: Local constraints aren't saved.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Nov 25 2020, 11:25 AM.

Details

Reviewers

Sybren A. Stüvel (sybren)

Group Reviewers

Animation & Rigging

Maniphest Tasks

T82758: Convert Proxy to Override: Local constraints aren't saved

Commits

rBfbdf1af35550: Fix T82758: Convert Proxy to Override: Local constraints aren't saved.

Summary

Ensure consistent order of pose bones. Now it should always match the
one from bones in armature obdata (as exposed by e.g. RNA, i.e.
children-first).

Previously when some pose bones would need to be added or removed from a
pose due to changes in the bone armature, or if bones in armature were
re-ordered, the bones order in pose would not match anymore the one from
armature, and could even become different between e.g. a proxy and its
linked source.

This was not really nice, but not a big issue before either. But with
diffing process of override, consistent order of items between reference
linked collection and local override one is crucial.

Diff Detail

Repository: rB Blender

Event Timeline

Bastien Montagne (mont29) created this revision.Nov 25 2020, 11:25 AM

Harbormaster completed remote builds in B11417: Diff 31358.Nov 25 2020, 11:25 AM

Bastien Montagne (mont29) requested review of this revision.Nov 25 2020, 11:25 AM

Sybren A. Stüvel (sybren) requested changes to this revision.Nov 26 2020, 11:25 AM

Sybren A. Stüvel (sybren) added a subscriber: Sybren A. Stüvel (sybren).

Sybren A. Stüvel (sybren) added inline comments.

source/blender/blenkernel/intern/armature.c
2454–2461	I think this should be its own function. That way it can also get a descriptive name, because it's not immediately clear what the code does at first glance.
2455	I think` bone_prev` can be `const`.
2463	`pchan` doesn't change in the newly added bit of code. This means that `pchan->bone` is the same as `bone`. This means that this function doesn't return anything that isn't already known to the caller. Why does it need to be returned?
2478	Here the same `r_prev_bone_p` is passed recursively, making it even harder to understand which value it'll point to. My guess is that it's just assigning `*r_prev_bone_p = bone->childbase.last`. What does `r_prev_bone_p` contain? It seems to be either the bone that was given, or recursively the last child bone ever visited. This deserves a bit of a comment to explain what's going on.
2548	T

This revision now requires changes to proceed.Nov 26 2020, 11:25 AM

Sybren A. Stüvel (sybren) added inline comments.Nov 26 2020, 11:26 AM

source/blender/blenkernel/intern/armature.c
2548	I can't edit/delete old notes. Ignore that one.

Update on matser, add some comments.

source/blender/blenkernel/intern/armature.c
2454–2461	I don't agree, I do not like two/three lines functions aside from when then allow de-duplicating code... Otherwise it just scatters code apart and make it harder to follow. What it does can be made more obvious with a comment if needed.
2463	Because it is recursive, so in the loop below `r_prev_bone_p` will always point to the actual last handled bone (which might be the last grand-son of the last son of current bone e.g, and so on). So no, its value is not always known by the caller.
2478	As its name implies, `r_prev_bone_p` contains the previous bone handled by this function. Since it's recursive deep-first, this info is not easily accessible at current level of the function, hence the need to get it back from recursive call hierarchy.

Harbormaster completed remote builds in B11435: Diff 31400.Nov 27 2020, 11:50 AM

I think the patch is acceptable as it is, so feel free to use or ignore my notes how you see fit.

source/blender/blenkernel/intern/armature.c
2444	The comment clarifies things. I think the naming can be improved, though; `r_last_visited_bone` to me covers better that "last" (or "previous") refers to visiting order.
2454	I don't think Doxygen will parse this, so `/*` should be enough.
2458	depth-first