-
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
Proposal for new architecture for UnityGLTF library #259
base: main
Are you sure you want to change the base?
Changes from 3 commits
152ffe4
f94c80e
5cd205d
c068452
57f547b
7aa3e51
2321bfc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# Unity glTF vNext Proposal | ||
|
||
## The Problem with v1 | ||
|
||
UnityGLTF has been great at providing implementation of import and export of glTF objects, and is close to being at the point of supporting the standard. There are some factors holding it back from being a great library though: | ||
|
||
- Hard to read codebase | ||
- Lack of modularity in components, which affects ease of ability to multithread | ||
- Lack of proper extension support | ||
- Need for better error reporting/handling | ||
- Better test coverage | ||
- Full standards compliance | ||
|
||
Due to these issues I am proposing a breaking API change, but the goal is for it to be a stable repository after that. | ||
|
||
My proposal for the new system would also remove support for any Unity version that does not properly support async/await (which means supporting the new language features + Unity having a sync context to ensure return to main thread). | ||
|
||
## Simpler API | ||
|
||
The first change I would like to make is to simplify the API. Currently the number of constructors and configuration options is overwhelming in creating a GLTFSceneImporter. | ||
|
||
public static class UnityGLTFLoader | ||
{ | ||
/// <summary> | ||
/// Creates a Unity GameObject from a glTF structure | ||
/// </summary> | ||
public Task<GameObject> Import( | ||
UnityGLTFObject unityGLTFFObject, // object which contains information to parse | ||
IDataLoader dataLoader, // Prev ILoader, gets stream data | ||
GLTFLoadOptions loadOptions = new GLTFLoadOptions() | ||
); | ||
|
||
/// <summary>Scheduler of tasks. Can be replaced with custom app implementation so app can handle background threads </summary> | ||
public static ITaskScheduler { get; set; } | ||
} | ||
|
||
## How | ||
|
||
A branch will be created, _v_next_, which will be where we work on the refactor. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,181 @@ | ||
/// <summary> | ||
/// Handles importing object from schema into Unity and handles exporting of objects from Unity into schema | ||
/// </summary> | ||
public partial class UnityGLTFImporter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I would prefer a class named Importer in a UnityGLTF namespace. Then people can choose to either write UnityGLTF.Importer (one additional character) or using the namespace and just refer to it as Importer. I know there are many varying opinions on this though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Partial classes are also kind of hard to hunt down as well. |
||
{ | ||
public UnityGLTFImporter( | ||
IDataLoader dataLoader, | ||
blgrossMS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
UnityGLTFImporterConfig config = new UnityGLTFImporterConfig() | ||
); | ||
|
||
/// <summary> | ||
/// Creates a Unity GameObject from a glTF scene | ||
/// </summary> | ||
/// <param name="unityGLTFFObject">Object which contains information to parse</param> | ||
/// <param name="sceneId">Scene of the glTF to load</param> | ||
/// <returns>The created Unity object</returns> | ||
blgrossMS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public Task<GameObject> ImportSceneAsync( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All these *Async methods should have a CancellationToken as the last parameter. This is very standard for async methods, and gives us the option of supporting cancellation. |
||
UnityGLTFObject unityGLTFFObject, | ||
int sceneId = -1 | ||
); | ||
|
||
/// <summary> | ||
/// Creates a Unity GameObject from a glTF node | ||
/// </summary> | ||
/// <param name="unityGLTFFObject">Object which contains information to parse</param> | ||
/// <param name="nodeId">Node of the glTF object to load.</param> | ||
/// <returns>The created Unity object</returns> | ||
public Task<GameObject> ImportNodeAsync( | ||
UnityGLTFObject unityGLTFFObject, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
int nodeId | ||
); | ||
|
||
/// <summary> | ||
/// Creates a Unity Texture2D from a glTF texture | ||
/// </summary> | ||
/// <param name="unityGLTFFObject">Object which contains information to parse</param> | ||
/// <param name="textureId">Texture to load from glTF object.</param> | ||
/// <returns>The created Unity object</returns> | ||
public Task<Texture2D> ImportTextureAsync( | ||
UnityGLTFObject unityGLTFFObject, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
int textureId | ||
); | ||
} | ||
|
||
// UnityNode.cs | ||
public partial class UnityGLTFImporter | ||
{ | ||
private Task<GameObject> ConstructNode(); | ||
} | ||
|
||
// UnityTexture.cs | ||
public partial class UnityGLTFImporter | ||
{ | ||
private Task<Texture2D> ConstructTexture(); | ||
} | ||
|
||
public class UnityGLTFImporterConfig | ||
{ | ||
public UnityGLTFImporterConfig(); | ||
public UnityGLTFImporterConfig(GLTFExtensionRegistry registry, GLTFImportOptions importOptions); | ||
} | ||
|
||
/// <summary> | ||
/// Rename of ILoader. Now returns tasks instead of IEnumerator. | ||
/// Handles the reading in of data from a path | ||
/// </summary> | ||
public class IDataLoader | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. interface, not class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not really locating resources. It's only resolving them to a stream. |
||
{ | ||
Task<Stream> LoadStream(string uri); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LoadStreamAsync, plus add a CancellationToken parameter. |
||
} | ||
|
||
public class GLTFImportOptions | ||
{ | ||
/// <summary> Scheduler of tasks. Can be replaced with custom app implementation so app can handle background threads </summary> | ||
System.Threading.Tasks.TaskScheduler TaskScheduler { get; set; } | ||
|
||
/// <summary>Override for the shader to use on created materials </summary> | ||
string CustomShaderName { get; set; } | ||
|
||
/// <summary> Adds colliders to primitive objects when created </summary> | ||
ColliderType Collider { get; set; } | ||
} | ||
|
||
public partial class UnityGLTFExporter | ||
{ | ||
/// <param name="dataWriter">Interface for handling the streams of data to write out</param> | ||
/// <param name="exportConfig">Configuration of extension settings and export otpions</param> | ||
public UnityGLTFImporter( | ||
IDataWriter dataWriter, | ||
UnityGLTFExporterConfig exportConfig = new UnityGLTFExporterConfig() | ||
); | ||
|
||
/// <summary> | ||
/// Exports a Unity object to a glTF file | ||
/// </summary> | ||
/// <param name="unityObject">The object to export</param> | ||
/// <returns></returns> | ||
public Task<GLTFFObject> Export( | ||
GameObject unityObject | ||
); | ||
} | ||
|
||
/// <summary> | ||
/// Writes data for an export operation | ||
/// </summary> | ||
public class IDataWriter | ||
{ | ||
Task<bool> WriteStream(string uri, Stream stream); | ||
} | ||
|
||
public class UnityGLTFExporterConfig | ||
{ | ||
public UnityGLTFExporterConfig(); | ||
public UnityGLTFExporterConfig(GLTFExtensionRegistry registry, GLTFExportOptions exportOptions); | ||
} | ||
|
||
public class GLTFExportOptions | ||
{ | ||
/// <summary>Whether to write the object out as a GLB</summary> | ||
bool ShouldWriteGLB { get; set ; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it not make more sense for this to just be a parameter of the actual Export* method(s)? Is it not possible to have a single exporter instance that exports both gltf and glb? |
||
} | ||
|
||
/// <summary> | ||
/// Unity wrapper for glTF object schema class from GLTFSerialization | ||
/// Properly cleans up data | ||
/// </summary> | ||
public class UnityGLTFObject : IDisposable | ||
{ | ||
/// <summary> | ||
/// Constructor for already parsed glTF or GLB | ||
/// </summary> | ||
/// <param name="gltfObject">Already parsed glTF or GLB</param> | ||
public UnityGLTFObject(IGLTFObject gltfObject); | ||
|
||
/// <summary> | ||
/// Constructor for not yet parsed glTF or GLB | ||
/// The IDataReader will handle loading the data to load the file | ||
/// </summary> | ||
/// <param name="fileName">Name of file to load</param> | ||
public UnityGLTFObject(string fileName); | ||
|
||
internal AssetCache AssetCache { get; } | ||
} | ||
|
||
/// <summary> | ||
/// Unity glTF extension wrapper | ||
/// </summary> | ||
public interface IUnityGLTFExtension | ||
{ | ||
IGLTFExtension GLTFExtension { get; }; | ||
|
||
Task<ExtensionReturnObject<GameObject>> CreateSceneAsync(GLTF.Schema.GLTFScene gltfScene, int indexToLoad); | ||
Task<ExtensionReturnObject<GameObject>> CreateNodeAsync(Node gltfNode, int indexToLoad); | ||
Task<ExtensionReturnObject<MeshPrimitive>> CreateMeshPrimitiveAsync(GLTFMesh mesh, ILoaderContext loaderContext, int indexToLoad); | ||
/// etc. | ||
} | ||
|
||
public struct ExtensionReturnObject<T> | ||
{ | ||
ExtensionContinuationBehavior ContinuationBehavior; | ||
T ReturnObject; | ||
} | ||
|
||
public enum ExtensionContinuationBehavior | ||
{ | ||
NotHandled, | ||
ContinueDefaultExecution, | ||
Handled | ||
} | ||
|
||
// Example calling pattern: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer valid, correct? |
||
public async void LoadGLBs() | ||
{ | ||
UnityGLTFObject sampleObject = new UnityGLTFObject("http://samplemodels/samplemodel.glb"); | ||
UnityGLTFObject boxObject = new UnityGLTFObject("http://samplemodels/box.glb"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this assuming that deserialization is synchronous? Seems like this would naturally lead people down a bad path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is just passing in a uri that will be resolved during the actual load |
||
IDataLoader dataLoader = new WebRequestLoader(); | ||
blgrossMS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
UnityGLTFImporter gltfImporter = new UnityGLTFImporter(dataLoader); | ||
await gltfImporter.ImportSceneAsync(sampleObject); | ||
await gltfImporter.ImportSceneAsync(boxObject); | ||
} |
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.
This block of code is out of date... delete?