Skip to content
This repository has been archived by the owner on Aug 11, 2024. It is now read-only.

Extract glTF utility to its own project. #801

Merged
merged 10 commits into from
Apr 11, 2021
Merged

Conversation

SimonDarksideJ
Copy link
Contributor

@SimonDarksideJ SimonDarksideJ commented Mar 11, 2021

XRTK - Mixed Reality Toolkit Pull Request

Overview

PR to extract the glTF utilities in to their own project.

Having a specific gLTF library within the XRTK restricted used from using their own glTF libraries and binds XRTK to tightly on a single glTF solution.

This PR simple removes the current glTF solution and moves it to it's own optional repo

Changes

Breaking Changes

  • Breaks
  • Only breaking change is that this removes the native ability to load glTF controller models directly. This feature should be refactored to be more open.

Related Submodule Changes

@SimonDarksideJ SimonDarksideJ added the Ready for review PR finished primary development, open for review label Mar 11, 2021
@SimonDarksideJ SimonDarksideJ self-assigned this Mar 11, 2021
Copy link
Contributor

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

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

See comment

@FejZa
Copy link
Contributor

FejZa commented Mar 21, 2021

My question would be, why does XRTK needs its own implementation of glTF / its own glTF module? This is something I haven't been using at all yet and thus can't really judge.

So why do we need this and what is the resolution for this PR?
@StephenHodgson "Can we at least provide a game object to instantiate?" can you explain what you mean? We are already creating a game object for each controller aren't we?

@SimonDarksideJ
Copy link
Contributor Author

My question would be, why does XRTK needs its own implementation of glTF / its own glTF module? This is something I haven't been using at all yet and thus can't really judge.

The separate implementation is simply another variant on the Khronos implementation but updated to use Unity JSON and a host of other improvements. I think it's worth preserving and maintaining in its own repo. But being separate means you can use your own if you wish instead.

So why do we need this and what is the resolution for this PR?
@StephenHodgson "Can we at least provide a game object to instantiate?" can you explain what you mean? We are already creating a game object for each controller aren't we?

That's what confused me, just hadn't had the time to circle back to this conversation. Any further thoughts @StephenHodgson ?

@StephenHodgson
Copy link
Contributor

Yes this is true, for edit time, but if we have a 3rd party glTF importer it'll likely pass a GameObject reference as either the instantiated object or an object to insatiate. I suppose that's what I'm looking for exactly. Just to link the references up (if the object has already been created) or to create it for us if it's not already in the scene.

@FejZa
Copy link
Contributor

FejZa commented Mar 24, 2021

Fair enough, now I get it.

@SimonDarksideJ
Copy link
Contributor Author

but if we have a 3rd party glTF importer it'll likely pass a GameObject reference

@StephenHodgson As commented above, you don't need a GameObject reference, just write your own Visualizer to load the glTF model and set the Visualizer type to your new visualiser for the controllers you want to configure.

@StephenHodgson
Copy link
Contributor

What is failing the build?

@SimonDarksideJ
Copy link
Contributor Author

What is failing the build?

Just checked, it's complaining about the Examples project. I'll do a quick PR to also remove the glTF samples from there

@SimonDarksideJ
Copy link
Contributor Author

SimonDarksideJ commented Mar 29, 2021

Pushed an update to change the branch for examples and the recent comments
XRTK/Examples#31

SimonDarksideJ added a commit to XRTK/com.xrtk.wmr that referenced this pull request Mar 29, 2021
@SimonDarksideJ
Copy link
Contributor Author

@StephenHodgson Issues resolved

Copy link
Contributor

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

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

Let's keep the glTF examples and just update the references

@StephenHodgson
Copy link
Contributor

@SimonDarksideJ let's be sure to create a issue in WMR for adding the visualizer later on once the glTF package is published.

@StephenHodgson StephenHodgson merged commit bfdf609 into development Apr 11, 2021
@StephenHodgson StephenHodgson deleted the Extract_glTF branch April 11, 2021 16:22
StephenHodgson added a commit to XRTK/com.xrtk.wmr that referenced this pull request Apr 11, 2021
* Remove glTF loading from WMR package

Supports: XRTK/com.xrtk.core#801

* Uplifted Core version dependency

* updated package dependencies

* updated gitignore

Co-authored-by: Stephen Hodgson <hodgson.designs@gmail.com>
XRTK-Build-Bot pushed a commit that referenced this pull request Apr 12, 2021
* PR to extract the glTF utilities in to their own project.

Resolves #800

* Version bump

* Added string extensions for analysing file names from URI paths for glTF web loading

* Addressed feedback

* Updated Examples branch

* Updated Submodules/Examples and Submodules/WindowsMixedReality to paired glTF updates

* Reverted Examples PR and switched it to use the new glTF package
Uplifted Core version dependency for Submodules

Co-authored-by: Stephen Hodgson <hodgson.designs@gmail.com>
XRTK-Build-Bot pushed a commit to XRTK/com.xrtk.wmr that referenced this pull request Apr 13, 2021
* Remove glTF loading from WMR package

Supports: XRTK/com.xrtk.core#801

* Uplifted Core version dependency

* updated package dependencies

* updated gitignore

Co-authored-by: Stephen Hodgson <hodgson.designs@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Ready for review PR finished primary development, open for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove current gLTF utility
3 participants