Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand bone name possibilities. #47074

Merged
merged 1 commit into from
Mar 16, 2021
Merged

Conversation

fire
Copy link
Member

@fire fire commented Mar 16, 2021

@Calinou Calinou added this to the 4.0 milestone Mar 16, 2021
@fire fire marked this pull request as ready for review March 16, 2021 19:42
@fire fire requested a review from a team as a code owner March 16, 2021 19:42
@fire fire marked this pull request as draft March 16, 2021 19:46
@fire fire marked this pull request as ready for review March 16, 2021 19:58
@fire
Copy link
Member Author

fire commented Mar 16, 2021

Updated test project.

GltfImporterRenamesBoneseUpdated.zip

@fire fire marked this pull request as draft March 16, 2021 20:05
@fire
Copy link
Member Author

fire commented Mar 16, 2021

I noticed something weird where I lose the animation track. Doing another testing pass.

@fire
Copy link
Member Author

fire commented Mar 16, 2021

I took https://github.com/KhronosGroup/glTF-Sample-Models/blob/master/2.0/RiggedSimple/glTF-Binary/RiggedSimple.glb and renamed the bones to .:@/".

The :/ character were stripped and the animation still works.

@fire fire marked this pull request as ready for review March 16, 2021 20:27
@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release and removed cherrypick:3.2 labels Mar 16, 2021
@akien-mga
Copy link
Member

Since there are only two trivial patterns, maybe you could use String::replace() instead of RegEx and remove the RegEx dependency in this code? It's a module that can be disabled in theory, and this dependency was not properly guarded with MODULE_REGEX_ENABLED, but now we can simply remove it. (That's not critical though, just a suggestion.)

And so for bones we don't need to apply the same replaces as in String::validate_node_name()?

@fire
Copy link
Member Author

fire commented Mar 16, 2021

I'll use replace.

Also, they never used the same replaces as String::validate_node_name() so I'm not going to limit them in the revision.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please also remove the #include for the RegEx module.

@akien-mga akien-mga merged commit d06a624 into godotengine:master Mar 16, 2021
@akien-mga
Copy link
Member

Thanks!

@fire fire deleted the unlock-bone-names branch March 16, 2021 22:37
@akien-mga
Copy link
Member

Cherry-picked for 3.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants