Changeset View
Standalone View
source/blender/blenkernel/intern/armature.c
| Show First 20 Lines • Show All 2,434 Lines • ▼ Show 20 Lines | else { | ||||
| if (prop_orig) { | if (prop_orig) { | ||||
| IDP_FreeProperty(prop_orig); | IDP_FreeProperty(prop_orig); | ||||
| } | } | ||||
| } | } | ||||
| } | } | ||||
| } | } | ||||
| } | } | ||||
| static int rebuild_pose_bone(bPose *pose, Bone *bone, bPoseChannel *parchan, int counter) | /** | ||||
| * \param r_last_visited_bone_p the last bone handled by the last call to this function. | |||||
sybren: The comment clarifies things. I think the naming can be improved, though; `r_last_visited_bone`… | |||||
| */ | |||||
| static int rebuild_pose_bone( | |||||
| bPose *pose, Bone *bone, bPoseChannel *parchan, int counter, Bone **r_last_visited_bone_p) | |||||
| { | { | ||||
| bPoseChannel *pchan = BKE_pose_channel_verify(pose, bone->name); /* verify checks and/or adds */ | bPoseChannel *pchan = BKE_pose_channel_verify(pose, bone->name); /* verify checks and/or adds */ | ||||
| pchan->bone = bone; | pchan->bone = bone; | ||||
| pchan->parent = parchan; | pchan->parent = parchan; | ||||
| /* We ensure the current pchan is immediately after the one we just generated/updated in the | |||||
Done Inline ActionsI don't think Doxygen will parse this, so /* should be enough. sybren: I don't think Doxygen will parse this, so `/*` should be enough. | |||||
| * previous call to `rebuild_pose_bone`. | |||||
Done Inline ActionsI think` bone_prev` can be const. sybren: I think` bone_prev` can be `const`. | |||||
| * | |||||
| * It may be either the parent, the previous sibling, or the last | |||||
| * (grand-(grand-(...)))-child (as processed by the recursive, depth-first nature of this | |||||
Done Inline Actionsdepth-first sybren: depth-first | |||||
| * function) of the previous sibling. | |||||
| * | |||||
| * Note: In most cases there is nothing to do here, but pose list may get out of order when some | |||||
Not Done Inline ActionsI 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. sybren: I think this should be its own function. That way it can also get a descriptive name, because… | |||||
Done Inline ActionsI 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. mont29: I don't agree, I do not like two/three lines functions aside from when then allow de… | |||||
| * bones are added, removed or moved in the armature data. */ | |||||
| bPoseChannel *pchan_prev = pchan->prev; | |||||
Not Done Inline Actionspchan 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? sybren: `pchan` doesn't change in the newly added bit of code. This means that `pchan->bone` is the… | |||||
Done Inline ActionsBecause 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. mont29: Because it is recursive, so in the loop below `r_prev_bone_p` will always point to the actual… | |||||
| const Bone *last_visited_bone = *r_last_visited_bone_p; | |||||
| if ((pchan_prev == NULL && last_visited_bone != NULL) || | |||||
| (pchan_prev != NULL && pchan_prev->bone != last_visited_bone)) { | |||||
| pchan_prev = last_visited_bone != NULL ? | |||||
| BKE_pose_channel_find_name(pose, last_visited_bone->name) : | |||||
| NULL; | |||||
| BLI_remlink(&pose->chanbase, pchan); | |||||
| BLI_insertlinkafter(&pose->chanbase, pchan_prev, pchan); | |||||
| } | |||||
| *r_last_visited_bone_p = pchan->bone; | |||||
| counter++; | counter++; | ||||
| for (bone = bone->childbase.first; bone; bone = bone->next) { | for (bone = bone->childbase.first; bone; bone = bone->next) { | ||||
| counter = rebuild_pose_bone(pose, bone, pchan, counter); | counter = rebuild_pose_bone(pose, bone, pchan, counter, r_last_visited_bone_p); | ||||
Not Done Inline ActionsHere 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. sybren: Here the same `r_prev_bone_p` is passed recursively, making it even harder to understand which… | |||||
Done Inline ActionsAs 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. mont29: As its name implies, `r_prev_bone_p` contains the previous bone handled by this function. Since… | |||||
| /* for quick detecting of next bone in chain, only b-bone uses it now */ | /* for quick detecting of next bone in chain, only b-bone uses it now */ | ||||
| if (bone->flag & BONE_CONNECTED) { | if (bone->flag & BONE_CONNECTED) { | ||||
| pchan->child = BKE_pose_channel_find_name(pose, bone->name); | pchan->child = BKE_pose_channel_find_name(pose, bone->name); | ||||
| } | } | ||||
| } | } | ||||
| return counter; | return counter; | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 53 Lines • ▼ Show 20 Lines | if (ob->pose == NULL) { | ||||
| animviz_settings_init(&ob->pose->avs); | animviz_settings_init(&ob->pose->avs); | ||||
| } | } | ||||
| pose = ob->pose; | pose = ob->pose; | ||||
| /* clear */ | /* clear */ | ||||
| BKE_pose_clear_pointers(pose); | BKE_pose_clear_pointers(pose); | ||||
| /* first step, check if all channels are there */ | /* first step, check if all channels are there */ | ||||
| Bone *prev_bone = NULL; | |||||
Not Done Inline ActionsT sybren: T | |||||
Not Done Inline ActionsI can't edit/delete old notes. Ignore that one. sybren: I can't edit/delete old notes. Ignore that one. | |||||
| for (bone = arm->bonebase.first; bone; bone = bone->next) { | for (bone = arm->bonebase.first; bone; bone = bone->next) { | ||||
| counter = rebuild_pose_bone(pose, bone, NULL, counter); | counter = rebuild_pose_bone(pose, bone, NULL, counter, &prev_bone); | ||||
| } | } | ||||
| /* and a check for garbage */ | /* and a check for garbage */ | ||||
| for (pchan = pose->chanbase.first; pchan; pchan = next) { | for (pchan = pose->chanbase.first; pchan; pchan = next) { | ||||
| next = pchan->next; | next = pchan->next; | ||||
| if (pchan->bone == NULL) { | if (pchan->bone == NULL) { | ||||
| BKE_pose_channel_free_ex(pchan, do_id_user); | BKE_pose_channel_free_ex(pchan, do_id_user); | ||||
| BKE_pose_channels_hash_free(pose); | BKE_pose_channels_hash_free(pose); | ||||
| ▲ Show 20 Lines • Show All 388 Lines • Show Last 20 Lines | |||||
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.