Implemented feedback feature for avatar bones.#860
Implemented feedback feature for avatar bones.#860XtroTheArctic wants to merge 8 commits intoKhronosGroup:mainfrom
Conversation
There was a problem hiding this comment.
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
PopulateBoneInfologic 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
MecanimBoneNamestarts with an uppercase letter, which conflicts with C# naming conventions for locals. Consider renaming tomecanimBoneNameor simplyboneName.
for (var i = 0; i < allMecanimBonesCount; i++)
| void PopulateBoneInfo() | ||
| { | ||
| var modelAsset = AssetDatabase.LoadAssetAtPath<GameObject>(importer.assetPath); | ||
| boneTransforms.Clear(); |
There was a problem hiding this comment.
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.
| boneTransforms.Clear(); | |
| boneTransforms.Clear(); | |
| assignedBoneNames.Clear(); |
| for (var i = 0; i < allMecanimBonesCount; i++) | ||
| { | ||
| var MecanimBoneName = HumanTrait.BoneName[i]; | ||
| if (boneGroupTabs[selectedBoneGroupTab] != boneGroups[MecanimBoneName]) continue; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" }; |
There was a problem hiding this comment.
[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.
| 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. |
There was a problem hiding this comment.
What "new" keyword GPT? Are we hallucinating again? :)
…kes finding the desired avatar in the project easier.
|
I just committed one more change which I forgot when first sending the PR. Sorry. |
|
Renamed MecanimBoneName to lowercase. |
|
Added missing assignedBoneNames.Clear call. |
|
Hello again @hybridherbst. I addressed the parts GPT mentioned. You can check if you want. Thanks again. |
|
hey, looks great! Please give us some time for reviewing and testing :) |
|
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! |
|
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. |
|
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. |
|
Hello. I am really sorry to bother you again but can I kindly ask you to check my PRs (this and few others) since they were sent more than 6 months ago? Sorry for the disruption. |
2 participants
This PR resolves the problem described in #859.
Here is how it looks: