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.

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

Use GLTFast as the primary load; fall back to the old code if it fails #278

Merged
merged 42 commits into from
Jul 23, 2023

Conversation

andybak
Copy link
Contributor

@andybak andybak commented Aug 25, 2022

Use GLTFast as the importer and fall back to the old code if GltfImport.Load() fails. Tested on most of the Kronos GLTF 2.0 samples. Apart from some more obscure examples, they all seem to import fine.

Current known bugs:

  1. If you save a scene and reload it, gltfs are missing initially. Loading the scene a second time works
  2. Importing point clouds seems to be broken
  3. API command for importing models isn't working (code was accidentally removed but added it back in and it still fails
  4. App freezes sometimes when importing a model via UI

@mikeskydev mikeskydev added the enhancement Feature added label Aug 25, 2022
Copy link
Member

@mikeskydev mikeskydev left a comment

Choose a reason for hiding this comment

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

Will do a proper review, but are the sample files and the changes to Main.unity needed? similarly, the shader includes, but I could see that being needed more. are those includes copied directly from glTFast or have they been altered somehow?

@andybak
Copy link
Contributor Author

andybak commented Aug 27, 2022

[are] the changes to Main.unity needed?

I can answer this bit. Pretty sure those are the same changes you'd see if you save Main.unity on the xr_vr branch. Just Unity being Unity.

@andybak
Copy link
Contributor Author

andybak commented Aug 29, 2022

but are the sample files [..] needed? similarly, the shader includes, but I could see that being needed more. are those includes copied directly from glTFast or have they been altered somehow?

@omn0mn0m - you're probably better placed to answer this bit. I haven't looked too closely at samples or shader files.

@omn0mn0m
Copy link

Will do a proper review, but are the sample files and the changes to Main.unity needed? similarly, the shader includes, but I could see that being needed more. are those includes copied directly from glTFast or have they been altered somehow?

The sample models were just for me testing with glb files. Sorry, forgot to delete them.

This might be my lack of unity knowledge, but I wasn't able to make .shadervariants without copying the .shader files locally. I figured there should be a way to do it, but could not find it.

@andybak andybak added this to the v2.0 milestone Aug 31, 2022
@mikeskydev mikeskydev force-pushed the feature/xr_v2 branch 2 times, most recently from 79ee667 to b9c11bd Compare September 9, 2022 15:15
@mikeskydev mikeskydev changed the base branch from feature/xr_v2 to main September 12, 2022 08:28
@mikeskydev mikeskydev changed the title Feature/gltfast openxr Feature/gltfast Sep 12, 2022
@andybak
Copy link
Contributor Author

andybak commented Sep 25, 2022

Not sure where you are with the other cleanups but I think point cloud import is now broken. I think that's a blocker as that's an existing, announced feature.

It seems to get as far as creating the gameobject but then doesn't convert into a prefab (or give an error)

@andybak
Copy link
Contributor Author

andybak commented Jun 21, 2023

Taking this out of draft mode. In my opinion the remaining API bug can be fixed after merging to main or shortly after. At the very least this is ready for review and further testing.

@andybak andybak marked this pull request as ready for review June 21, 2023 14:04
@mikeage
Copy link
Member

mikeage commented Jun 21, 2023

Are the compilation failures legitimate, or a temporary problem on the server side?

@andybak
Copy link
Contributor Author

andybak commented Jun 21, 2023

Sometimes hard to discern which errors are actually causing the failure. I think it's this part:

An error occurred while resolving packages: Project has invalid dependencies: org.nuget.google.apis: Package [[email protected]] cannot be found org.nuget.google.apis.auth: Package [[email protected]] cannot be found org.nuget.sharpziplib: Package [[email protected]] cannot be found

I am assuming they are spurious because none of that is specific to this branch.

@mikeage
Copy link
Member

mikeage commented Jun 22, 2023

Sometimes hard to discern which errors are actually causing the failure. I think it's this part:

An error occurred while resolving packages: Project has invalid dependencies: org.nuget.google.apis: Package [[email protected]] cannot be found org.nuget.google.apis.auth: Package [[email protected]] cannot be found org.nuget.sharpziplib: Package [[email protected]] cannot be found

I am assuming they are spurious because none of that is specific to this branch.

I assumed, but I saw that there were changes to the packages file and just wanted to make sure that these weren't dependencies or anything like that. In any case, looks good now!

@mikeskydev mikeskydev modified the milestones: Future Roadmap, 3.0 Jun 26, 2023
@mikeage mikeage changed the title Feature/gltfast Use GLTFast as the primary load; fall back to the old code if it fails Jul 23, 2023
@mikeage mikeage merged commit 98c7817 into icosa-foundation:main Jul 23, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants