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.

Sign up for GitHub

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

Jump to bottom

Update to support UPM packages for glTF #631

Merged
merged 16 commits into from
Feb 3, 2025
Merged

Update to support UPM packages for glTF #631

merged 16 commits into from
Feb 3, 2025

Conversation

ms-RistoRK
Copy link
Contributor

@ms-RistoRK ms-RistoRK commented Feb 2, 2024 •
edited by keveleigh
Loading

Fixes #630
Fixes #883

keveleigh
keveleigh previously requested changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@keveleigh keveleigh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

.

UnityProjects/MRTKDevTemplate/Packages/manifest.json Show resolved Hide resolved
UnityProjects/MRTKDevTemplate/Packages/manifest.json Show resolved Hide resolved
AMollis
AMollis requested changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@AMollis AMollis left a comment

Choose a reason for hiding this comment

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

I would prefer to leave the GLTF package in the sample project. To continue to validate controller model loading.f

UnityProjects/MRTKDevTemplate/Packages/manifest.json Show resolved Hide resolved
ms-RistoRK
ms-RistoRK commented Feb 3, 2024
View reviewed changes
Copy link
Contributor Author

@ms-RistoRK ms-RistoRK left a comment

Choose a reason for hiding this comment

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

I believe to have attended the requested changes, please re-review.

AMollis
AMollis previously requested changes Feb 3, 2024
View reviewed changes
Copy link
Contributor

@AMollis AMollis left a comment

Choose a reason for hiding this comment

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

I would like @keveleigh to sign off on the changes, since they have more knowledge on this area.

org.mixedrealitytoolkit.input/Editor/MRTK.Input.Editor.asmdef Show resolved Hide resolved
org.mixedrealitytoolkit.input/MRTK.Input.asmdef Show resolved Hide resolved
org.mixedrealitytoolkit.input/Editor/MRTK.Input.Editor.asmdef Outdated Show resolved Hide resolved
@ms-RistoRK @keveleigh
Attending PR feedback
adb3c67 
Update manifest.json
@ms-RistoRK ms-RistoRK marked this pull request as draft February 6, 2024 17:55
@ms-RistoRK
Copy link
Contributor Author

Abandoning this PR as we don't have the resources to exhaustively test on all devices where MRTK3 is deployed and the bandwidth issue with com.atteneder.ktx package is an accepted known issue.

@ms-RistoRK ms-RistoRK closed this Feb 6, 2024
@ms-RistoRK ms-RistoRK mentioned this pull request Feb 6, 2024
Closed
@keveleigh keveleigh reopened this Jan 9, 2025
@keveleigh keveleigh force-pushed the Bug630FixRemovingWrongPackageReferences branch from 3a7079f to 10acdc1 Compare January 9, 2025 22:44
@keveleigh keveleigh changed the title Bug630 fix - Removing wrong package references Updating sample project to use UPM packages for glTF Jan 9, 2025
@keveleigh keveleigh self-assigned this Jan 9, 2025
@keveleigh keveleigh force-pushed the Bug630FixRemovingWrongPackageReferences branch from 10acdc1 to 1c43b9b Compare January 9, 2025 22:54
@keveleigh
Update glTFast
6d16fff
@keveleigh
Update usage of KTX_PRESENT
f60229d 
Update ControllerModelLoader.cs

Update ControllerModelLoader.cs
@keveleigh keveleigh force-pushed the Bug630FixRemovingWrongPackageReferences branch 2 times, most recently from 9507104 to 8cb85ed Compare January 9, 2025 23:47
@keveleigh keveleigh marked this pull request as ready for review January 10, 2025 00:22
@keveleigh keveleigh dismissed stale reviews from AMollis and themself January 10, 2025 00:22

Updated

@keveleigh keveleigh changed the title Updating sample project to use UPM packages for glTF Update to support UPM packages for glTF Jan 10, 2025
@keveleigh keveleigh force-pushed the Bug630FixRemovingWrongPackageReferences branch from 17d0ee4 to 95eaa95 Compare January 28, 2025 18:40
@MaxPalmer-UH MaxPalmer-UH self-requested a review February 3, 2025 14:07
MaxPalmer-UH
MaxPalmer-UH approved these changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@MaxPalmer-UH MaxPalmer-UH left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seems to work well either with the template project from within the repo or adding packages to an existing project and then validating/fixing dependencies. I don't see the issues linked with this branch (#630 & #883)

Note, I did once see errors with missing meta files for part of the KTX package when opening the template project, but my assumption is that this was a project path length problem and also wasn't an MRTK issue.

image

@keveleigh
Copy link
Contributor

Note, I did once see errors with missing meta files for part of the KTX package when opening the template project, but my assumption is that this was a project path length problem and also wasn't an MRTK issue.

Ah yeah, I also saw these. Looks like an issue with the way Unity packed their package, but it shouldn't affect our implementation even when they fix it. Thank you for the review!!

@keveleigh keveleigh merged commit f0dde8f into MixedRealityToolkit:main Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keveleigh keveleigh keveleigh left review comments

@MaxPalmer-UH MaxPalmer-UH MaxPalmer-UH approved these changes

@AMollis AMollis AMollis left review comments

@marlenaklein-msft marlenaklein-msft Awaiting requested review from marlenaklein-msft

@shaynie shaynie Awaiting requested review from shaynie

Assignees

@keveleigh keveleigh

@ms-RistoRK ms-RistoRK

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ms-RistoRK @keveleigh @AMollis @MaxPalmer-UH