Skip to content

Conversation

@XtroTheArctic
Copy link
Contributor

This PR resolves the problem described in #859.

Here is how it looks:

image

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 a visual feedback panel in the GLTF importer’s Animation tab that shows which humanoid bones are assigned, grouped by body part, and uses custom dot icons to indicate required vs. optional bones.

  • Introduces static icon textures and tab/group mappings for bones
  • Implements PopulateBoneInfo() to cache model transforms and avatar assignments
  • Extends AnimationInspectorGUI() to render a tabbed list of bones with assignment status
Comments suppressed due to low confidence (2)

Editor/Scripts/GLTFImporterInspector.cs:135

  • The new PopulateBoneInfo logic and its effect on UI are not covered by automated tests. Consider adding unit or integration tests to verify that transforms and assigned names are populated correctly.
void PopulateBoneInfo()

Editor/Scripts/GLTFImporterInspector.cs:310

  • [nitpick] Local variable MecanimBoneName starts with an uppercase letter, which conflicts with C# naming conventions for locals. Consider renaming to mecanimBoneName or simply boneName.
for (var i = 0; i < allMecanimBonesCount; i++)

void PopulateBoneInfo()
{
var modelAsset = AssetDatabase.LoadAssetAtPath<GameObject>(importer.assetPath);
boneTransforms.Clear();
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.

You clear boneTransforms but don't clear assignedBoneNames before repopulating, which can lead to stale entries if OnEnable is called multiple times. Add assignedBoneNames.Clear(); alongside clearing boneTransforms.

Suggested change
boneTransforms.Clear();
boneTransforms.Clear();
assignedBoneNames.Clear();

Copilot uses AI. Check for mistakes.
for (var i = 0; i < allMecanimBonesCount; i++)
{
var MecanimBoneName = HumanTrait.BoneName[i];
if (boneGroupTabs[selectedBoneGroupTab] != boneGroups[MecanimBoneName]) continue;
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.

Accessing boneGroups[MecanimBoneName] can throw if a bone name isn’t in the dictionary. Use boneGroups.TryGetValue or check ContainsKey before indexing to avoid runtime exceptions.

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.

Hi GPT. I didn't expect to see you here. I was surprised :)

There are only 4 bone groups defined in the code and this line is querying just those four of them. If there was an exception here, I would point to a data error in the dictionary which we would fix immediately. This data is not user driven so there is no issue here.

static Texture BoneAssignmentDotFrameIcon;
static Texture BoneAssignmentDotFrameDottedIcon;

static string[] boneGroupTabs = { "Body", "Head", "Left Hand", "Right Hand" };
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 new keyword hides any inherited member and can be confusing. Rename or remove hiding to follow best practices, or explicitly qualify intent with a comment.

Suggested change
static string[] boneGroupTabs = { "Body", "Head", "Left Hand", "Right Hand" };
static string[] boneGroupTabs = { "Body", "Head", "Left Hand", "Right Hand" };
// The 'new' keyword is used here to hide the 'boneGroups' member in the base class.
// This is intentional to provide a different implementation specific to GLTFImporterInspector.

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.

What "new" keyword GPT? Are we hallucinating again? :)

…kes finding the desired avatar in the project easier.
@XtroTheArctic
Copy link
Contributor Author

I just committed one more change which I forgot when first sending the PR. Sorry.

@XtroTheArctic
Copy link
Contributor Author

Renamed MecanimBoneName to lowercase.

@XtroTheArctic
Copy link
Contributor Author

Added missing assignedBoneNames.Clear call.

@XtroTheArctic
Copy link
Contributor Author

Hello again @hybridherbst. I addressed the parts GPT mentioned. You can check if you want. Thanks again.

@robertdorn83
Copy link
Collaborator

hey, looks great! Please give us some time for reviewing and testing :)

@XtroTheArctic
Copy link
Contributor Author

Yea, no problem. Thou, even it looks much, it's very simple implementation. Let me know if I can help in any way. Thank you for this great project!

@XtroTheArctic
Copy link
Contributor Author

@robertdorn83 @hybridherbst

Hello guys.

I am sure you are busy with lots of other stuff but, since it's been a while, and since I've put a lot of effort into my PRs, I wanted to ask if you will be able to check this PR and my other PRs hopefully soon? Also, I have a question in one of my issues.

#877
#878
#857

@robertdorn83
Copy link
Collaborator

Hey, don't worry.. we will definitely take look at that and appreciate your efford. Please keep in mind, that none of us is a fulltime developer of this repo. So sometimes it can take some time.

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