Added the bone names which Unity's FBX importer can detect to the dictionary.#862
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds support for additional bone name mappings that Unity's FBX importer can detect.
- Changed HumanSkeletonNames from public to internal scope.
- Appended 30+ simple bone name mappings to the dictionary.
- Updated comment to mark new importer-detected bone list.
Comments suppressed due to low confidence (2)
Editor/Scripts/Internal/AvatarUtils.cs:13
- The removal of the public modifier changes the accessibility of HumanSkeletonNames, potentially breaking external code that relied on it. Consider reinstating the public keyword or confirming this change is intended.
static Dictionary<string, string> HumanSkeletonNames = new Dictionary<string, string>()
Editor/Scripts/Internal/AvatarUtils.cs:175
- Consider adding unit tests to verify that all newly supported bone names (e.g., leftthumb1, rightthumb3, jaw) are correctly detected in HumanSkeletonNames.
// Simple bone names Unity's FBX importer can detect.
| {"rightfoottoe_jnt", "RightToes"}, | ||
|
|
||
| }; | ||
| // Simple bone names Unity's FBX importer can detect. |
There was a problem hiding this comment.
[nitpick] The block of hard-coded bone mappings is quite large; consider externalizing this list (for example, in a JSON config or resource file) or grouping entries to simplify maintenance and updates.
There was a problem hiding this comment.
The purpose of this PR is to simply add some missing bone mappings. Refactoring the entire dictionary would be the subject of another task/request.
|
GPT: "The removal of the public modifier changes the accessibility of HumanSkeletonNames, potentially breaking external code that relied on it. Consider reinstating the public keyword or confirming this change is intended." The class is internal so there is no point of its members being public. I checked the usages of HumanSkeletonNames and didn't find any outside. |
|
Hello again @hybridherbst. I answered GPT's comments, if you wanna check. Thank you. |
This PR resolves the problem described in #861
Using the new feature I implemented (via this PR, you can verify that UnityGLTF can now detect only all existing 52 model bones of the given model(in the issue ticket) which Unity's FBX importer can detect by default.
Here how it looks with this fix (P.S. This model doesn't contain UpperChest, LeftEye and RightEye):