Skip to content

Conversation

@XtroTheArctic
Copy link
Contributor

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):

image

@hybridherbst hybridherbst requested a review from Copilot June 8, 2025 20:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.
Copy link

Copilot AI Jun 8, 2025

Choose a reason for hiding this comment

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

[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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@XtroTheArctic
Copy link
Contributor Author

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.

@XtroTheArctic
Copy link
Contributor Author

Hello again @hybridherbst.

I answered GPT's comments, if you wanna check. Thank you.

@hybridherbst hybridherbst merged commit d17dc8a into KhronosGroup:main Jun 12, 2025
1 check passed
@XtroTheArctic XtroTheArctic deleted the feature/861-DetectBonesWhichFBXImporterCan branch June 13, 2025 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants