PropertyBinding.parseTrackName can not handle non-delimiter '.' characters in track names

[richtr]

Following the merge of #10276 there is an issue with how non-delimiter '.' characters are handled in PropertyBinding.parseTrackName. There are no requirements in most formats that track names can not include non-delimiter '.' characters.

Specific discussion of this issue starts here: #10276 (comment).

Filing this as a bug so it doesn't get lost.

/cc @mrdoob @donmccurdy @takahirox @bhouston

[donmccurdy]

Ideas, mostly from the other thread:

1. Replace . characters in names with something arbitrary (e.g. __dot__), within the loader. Pro: no other changes needed. Con: feels a bit hacky, and might interfere with userland code that expects original names.
2. Use UUIDs in track path, avoiding the user-defined names. Pro: Robust against weird names, no other changes needed. Con: Animations will no longer work with cloned meshes, as UUIDs will change. Animations should ideally be more reusable.
3. Don't serialize path as a string. Example: instead of "nodeName.property[accessor]" use [nodeName, property, accessor] or {nodeName: ..., property: ..., accessor: ...}. Pro: No (de)serialization or delimiters needed. Con: I don't know if other things depend on string track paths.

[donmccurdy]

This change would be one way to fix the issue for GLTFLoader, by allowing the loader to specify track.parsedPath. The issue would remain for KeyframeTrack instances created without a parsed path.

[donmccurdy]

Another idea:

4. In PropertyBinding.findNode(), also check against some private/internal property, like node._name. This would allow us to safely alter the original node name. Or, even better for glTF, assign node._name = target.id. glTF does not require names at all: animations reference the target node using a glTF ID: glTF IDs and Names.

[bhouston]

I think this may be possible to do via regex modification:

(1) The last dot is required as a separator between the property name and the node path name.
(2) A further dot is required if one has one of the pre-defined sub-properties supported such as ".material." or a ".bone[...]."
(4) The name inside of a bone can have a dot, but no slashes as it isn't a path.
(5) Dots are not allowed inside of property names.
(6) Otherwise dots are allowed between slashes as they are node names.

The above rules appear to work for all examples here:

three.js/src/animation/PropertyBinding.js

Line 535 in 14c47ec

// matches strings in the form of:

But this one:

uuid.objectName[objectIndex].propertyName[propertyIndex]

But I am unsure how common the above is. We could just discontinue support for it. I think the above can be handled but I am not sure it is used in practice.

I think that using some form of the above, you possibly can fix this just by making the regex a more complicated and making no other changes.

My only concern then becomes is there is a way to make regexes manageable as you increase their complexity? I worry that while this can be done, we'll move towards a regex from hell that only @donmccurdy understands.

[bhouston]

I favor the regex modification because it is fixing this in a long-term way and it is a minimal change, even though it will increase the complexity of the regex.

[donmccurdy]

@bhouston I'm open to that plan, assuming the regex is manageable.

Before we make that call, here's one more consideration if we want to support glTF robustly: glTF doesn't require node names, and it would be completely valid to have a rigged model with no named nodes. The format uses string IDs to bind animation to nodes instead. IMO, it seems like solving this robustly for glTF (since that's the format that raised this bug) will require persisting those IDs in some way. Assign them as mesh.name, mesh.userData.id... I'm not sure.

EDIT: It seems that the FBX loader is dealing with some similar things.