[PropertyBinding] Support for '.' in node names, and unit tests. #10859
Conversation
donmccurdy
force-pushed
the
test-parsetrackname
branch
from
beba25c to
d5c89b5
Compare
|
I of course support this approach as we discussed it. I like the addition of the unit tests. :) Thanks for the contribution. |
|
@takahirox looks good? |
|
Looks good, but give me a time a bit to think if the limitation won't be any problems. |
donmccurdy
force-pushed
the
test-parsetrackname
branch
from
d5c89b5 to
7f6be9e
Compare
|
Sorry I can't make a time to check in some days. Have you confirmed all the animation examples keep working with this change? |
|
No rush. Yeah the animation examples all look good. |
|
One way to ensure that this doesn't break in the future is contribute some complex glTF example to the Three.JS project that has some tricky to parse names. |
src/animation/PropertyBinding.js
Outdated
| // Object on target node, and accessor. May contain only word characters, | ||
| // and must be a member of the supportedObjectNames whitelist. Accessor may | ||
| // contain any non-bracket characters. | ||
| var objectRe = /(?:\.([\w-]+)(?:\[(.+)\])?)?/; |
There was a problem hiding this comment.
This comment is confusing a bit because it accepts a name not in whitelist.
And this regex accepts bracket (, ), [, and so on for accessor.
There was a problem hiding this comment.
The whitelist isn't part of the regex because it needs to be enforced afterwards; updated the comment about the accessor to specify that only closing brackets are disallowed.
There was a problem hiding this comment.
I meant, it'd be helpful to write here the result is compared with the whilelist afterwards and why.
When I first read this comment, I thought the whilelist should be in the regex because
it mentions must be a member of the supportedObjectNames whitelist.
There was a problem hiding this comment.
Updated: the comment by the regex now describes only the regex, and the comment by the whitelist checking describes that.
src/animation/PropertyBinding.js
Outdated
| var matches = re.exec( trackName ); | ||
| // Target node. May contain word characters (a-zA-Z0-9_) and '.' or '-' | ||
| // characters, but must begin and end with a word character. | ||
| var nodeRe = /(\w(?:[\w-\.]*\w)?)?/; |
There was a problem hiding this comment.
but must begin and end with a word character
Why is this limitation necessary?
nodeName is compared to .name which is general strings so could begin and end with non-word character like -something-
https://github.com/donmccurdy/three.js/blob/d6bf94269463418204213dd4f7efdc417311cc24/src/animation/PropertyBinding.js#L198
https://github.com/donmccurdy/three.js/blob/d6bf94269463418204213dd4f7efdc417311cc24/src/animation/PropertyBinding.js#L229
There was a problem hiding this comment.
Ok, I thought that trailing '.' would break regex parsing but it seems fine. Fixed.
donmccurdy
force-pushed
the
test-parsetrackname
branch
2 times, most recently
from
6a7d66f to
7965393
Compare
|
@takahirox thumbs up? |
|
Yep, great work @donmccurdy ! |
|
Nice work guys! |
Fixes #10366, #10527.
Modifies the nodeName portion of the existing regex to allow
.characters, and breaks the regex into parts to improve readability.The key limitation here: only objectNames in the whitelist (material, materials, bones) are supported. The whitelist is a strict requirement if we keep using regex: Given
foo.bar.baz, we knowbazmust be a property, but there is no way to tell in advance whetherbaris an objectName or part of the nodeName.This is one of several solutions discussed in #10366, so if we're not happy with the regular expression + whitelist we can try another approach.
/cc @bhouston @richtr @takahirox