Import/Export Bone Inherit Scale Mode with Use Blender Profile for COLLADA
Needs ReviewPublic
Actions

Authored by Dylan Ascencio (Gamma) on May 1 2020, 7:05 AM.

Details

Reviewers

Gaia Clary (gaiaclary)

Group Reviewers

Collada

Summary

When exporting a scene as a COLLADA file, all bones lose the value of their Inherit Scale Mode field. When re-imported into Blender, the inheritance scale mode for all bones is reset to "Full", which could potentially cause issues if users choose to distribute COLLADA files rather than Blend files when sharing projects.

This patch addresses the concern by including the scale inheritance mode in the exported COLLADA file when the user selects "Use Blender Profile" in the Export dialog. I considered making this option its own checkbox under the "Armature" section. However, as the inheritance mode is specific to Blender and not part of the official COLLADA specification, I chose to include it with the other Blender-specific data output upon user request.

Including this patch may cause an undesirable inconsistency, as the states of the Inherit Rotation and Local Location fields are also lost upon exporting to COLLADA. I am willing to address this inconsistency if it is a blocking issue for this patch.

Diff Detail

Repository: rB Blender

Event Timeline

Dylan Ascencio (Gamma) requested review of this revision.May 1 2020, 7:05 AM

Dylan Ascencio (Gamma) created this revision.

Well, there are many dozens (hundreds?) of blender specific attributes which are not included in the Blender profile. So from one point of view this patch is just another arbitrary addition to the Blender export set. On the other side we have actually added attributes to the blender profile over the years 'as needed by users'. So i am not against such an extension. However if we are going to extend this, then lets do it fully. Also make sure that all related attributes are handled properly by the exporter and by the importer:

use_connect (already exists, but maybe needs change, see note below)
use_inherit_rotation
use_local_location

Note: For some reason we have used an integer for the use_connect attribute with a default value of -1 for "not specified". But i guess it could be safely changed to bool with the default == false (unconnected)

source/blender/io/collada/collada_utils.cpp
559	Here you see how using an Enumeration could be a better solution.
source/blender/io/collada/collada_utils.h
328	I am not sure but what about renaming to how the property is named in blender: 'inherit_scale' ? And wouldn't it be better to use an Enumeration here rather than a character?
358	same as above: suggest to rename to set_inherit_scale() and get_inherit_scale()

This revision now requires changes to proceed.May 1 2020, 9:05 AM

I have updated the patch to include Use Local Location. Use Connect was already implemented, so I cleaned it up with the changes that @Gaia Clary (gaiaclary) suggested.

However, I was unable to determine where the Use Inherit Rotation field is located. Use Connect and Use Local Location are bits within the bone's flag field, but Use Inherit Location has no clearly named flag. There is a flag called BONE_HINGE, described as /** No parent rotation or scale */. Is this the source of the Use Inherit Location field?

Additionally, I may have discovered a bug when reading extra tag data from a document. On L1078 in ArmatureImporter.cpp, the ExtraTags object used to read the data is initialized to null. It is only properly assigned to within the following if statement, whose condition is etit != uid_tags_map.end(). However, even before I made my changes, there were requests for data from the ExtraTags object after the if statement. If the condition is ever not met, and the if statement does not run, the reference to the ExtraTags object will remain null, thus causing undefined behavior (Access violation/segmentation fault). I wanted to consult with those who know the COLLADA codebase more in-depth before I did anything to that section in case there is a reason for this or if it does not result in the behavior that I think it does.

Regarding where the use_rotation is stored i believe that this is correct:

in rna_armature_gen.c are 2 functions named

Bone_use_inherit_rotation_get()
Bone_use_inherit_rotation_set()

Both functions refer to Bone->flag and eBone_Flag::BONE_HINGE so i believe that this Bone->flag is the correct location for the attribute.

Regarding the ExtraTags issue i do not understand the problem. et is a local variable that is not used outside the if statement. I think it would be best to move the ExtraTags *et into the if block to make this properly.

BTW: I recall there has been an ancient(?) recommendation in the blender code style guide to declare all variables always at the start of a function. But this rule was never applied fully in the Collada module, maybe because it does not apply well to c++ :) However i would not add unrelated changes into one single patch. Also cosmetic changes should be made separately so the essence of the patch remains clearly visible (my opinion).

source/blender/io/collada/ArmatureExporter.cpp
197	Why is this added only to leaf bones ? Shouldn't this go into the if block starting at line 169 ?
202	Same as previous comment
source/blender/io/collada/ArmatureImporter.cpp
181	Just curious: Why is this code moved down from its previous location?

Henrik Dick (weasel) awarded a token.Jun 19 2021, 2:42 PM

rebase on current master (basically no changes, but the patch now applies again)

This is exactly what I need currently, would be nice if this could get into Blender.
@Dylan Ascencio (Gamma) hope you don't mind my edit, since you seem to be inactive for almost a year now.

Harbormaster completed remote builds in B15319: Diff 38528.Jun 19 2021, 3:51 PM

@Henrik Dick (weasel) There seems to be something wrong with this Differential. It contains a ton of changes unrelated to this topic. Maybe something with different end of line issues ?

@Gaia Clary (gaiaclary) There was a change for connect to use boolean mixed in here (wasn't me). I can remove that in my next diff update here.

There is however a much bigger issue here. Exported animations will be corrupted! Currently the animation is converted, to work with the new imported rig which does not have this bone option. With this patch it will still do that and therefore mess up the animation. This issue can not simply be solved in the exporter since other software does not understand the blender specific transform and won't understand the animation. It can be solved in the importer though, so when importing an armature with this type data != Full it would need to calculate a corrected transform.