-
Notifications
You must be signed in to change notification settings - Fork 492
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
Culmination of two years of fixes, especially for export #644
Culmination of two years of fixes, especially for export #644
Conversation
pfcDorn seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Added a couple more commits that have fixed issues people brought up (some of them old, some of them introduced by the PR). One bigger (kind of unintended) feature that now works is WebGL platform import support (which seems to have never been working before). So now WebGL has runtime import + runtime export, including animations, same as other platforms. |
Amazing, so gltFast is included here as well? P.s. just read more carefully, they are compatible. Still, amazing progress! |
Big update that actually brings UnityGLTF back to being state-of-the-art in some areas!
Additionally, some more fixes went in for camera/lights export and import, material export heuristics, and helpful things like ProfilerMarkers. |
UnityGLTF/Assets/UnityGLTF/Runtime/Plugins/GLTFSerialization/Schema/MeshPrimitive.cs
Outdated
Show resolved
Hide resolved
@@ -30,6 +30,7 @@ public class Skin : GLTFChildOfRootProperty | |||
|
|||
public Skin() | |||
{ | |||
Joints = new List<NodeId>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this might have unintended consequences due to assumptions in this class and others.
This class itself specifically checks Joints != null
on line 50, though it checks Joints != null && Joints.Count > 0
on line 113. Maybe line 50 just needs to be updated to also check .Count
(and potentially anywhere else in the codebase that accesses Joints
)?
…sor to cover more cases where a reimport might be necessary
…learer _OverrideSurfaceMode
… name can't be found)
…compression packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did all of these get removed? Where did they go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have been removed for now because they were
a) in the wrong place (test assets outside the Tests folder)
b) tests didn't work even when I started working on the fork.
Some recent changes to speed up certain models by 10-100x also resulted in the GLTFSerialization assembly having a dependency on Unity, which I'm not particularly happy with – but the performance improvements were worth it I think.
We are planning to bring better test capabilities back though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So no replacement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind elaborating a bit more? There are currently less test files directly in the package because it doesn’t make sense to ship test files inside a package. As in the screenshot above we’re planning to add a window that allows importing directly from the Khronos Sample Assets for tests and verification. The same mechanism is used for automated tests as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Khronos Sample Assets
Where are these located?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UnityGLTF repo has been stale for around 2 years now.
This is approximately the same time when we (prefrontal cortex) started working with glTF extensively, and thus we forked this repository and starting to improve things. There have been more than 300 commits since then, numerous features have been added, and most notably, the package has been used in many productions and battle-tested, especially for exporting glTF files from Unity. Early on, we tried to PR our changes in small chunks, but it soon became clear that the repo was abandoned.
This PR moves all of our changes over to the main repository and effectively promotes our fork, so that veryone can benefit from the changes done so far.
State of glTF in Unity
UnityGLTF hasn't received official support since early 2020. However, a number of forks have fixed issues and improved several key areas, especially animation support,export workflows, color spaces and extendibility. These forks have now been merged back into main so that everyone can benefit from then, and to enable further work.
A separate glTF implementation for Unity, glTFast, is on its path towards becoming feature complete, with import already being complete. It leverages modern Unity features such as Burst and Jobs, has better compression support (importing compressed textures and meshes), and also has wider Render Pipeline support, notably supporting URP and HDRP import (and partial export).
glTFast and UnityGLTF can coexist in the same project; you can for example use glTFast for import and UnityGLTF for export, where each of these shine. In fact, we've been contributors to both projects.
Notable changes
Notable things that haven't changed
How to test this PR
Window > Package Manager
Once the PR is approved, the package will be updated on OpenUPM as well for easier consumption (no git installation required).
Example exports
All these projects have been exported with our fork of UnityGLTF:
and many more.
Next Steps
This PR is now up for feedback. The changes that were made over the last 2 years tried to keep the repository structure the same to make eventual reviewing easier. Whoever has concerns about the general idea of promoting this fork to main: please let me know, now is the time!
Full changelog with ~35 internal releases: https://github.com/prefrontalcortex/UnityGLTF/blob/master/UnityGLTF/Assets/UnityGLTF/CHANGELOG.md
The goal is to merge this PR in ~1-2 weeks.
This PR has been made possible by prefrontal cortex and Needle.