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

Proposal for new architecture for UnityGLTF library #259

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions vNext/UnityGLTF_vnext_proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# 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).

## Code Changes
See [UnityGLTFv2.cs](UnityGLTFv2.cs) for proposed interface

## How

A branch will be created, `_v_next_`, which will be where we work on the refactor.
216 changes: 216 additions & 0 deletions vNext/UnityGLTFv2.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
namespace UnityGLTF {

/// <summary>
/// Handles importing object from schema into Unity and handles exporting of objects from Unity into schema
/// </summary>
public virtual class Importer
{
public Importer(
IDataLoader dataLoader,
blgrossMS marked this conversation as resolved.
Show resolved Hide resolved
ImporterConfig config = new ImporterConfig()
);

/// <summary>
/// Creates a Unity GameObject from a glTF scene
/// </summary>
/// <param name="unityGLTFObject">Object which contains information to parse</param>
/// <param name="sceneId">Scene of the glTF to load</param>
/// <param name="progress">Progress of load completion</param>
/// <returns>The created Unity object</returns>
blgrossMS marked this conversation as resolved.
Show resolved Hide resolved
public virtual Task<GameObject> ImportSceneAsync(
UnityGLTFObject unityGLTFObject,
Copy link

@ryantrem ryantrem Oct 9, 2018

Choose a reason for hiding this comment

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

The whole UnityGLTFObject stateful wrapper still seems like a confusing part of this design to me. I think I'm now leaning more towards just passing in the raw deserialized data structure for the gltf root, and additionally (optionally) passing in an AssetCache by ref. Any problems with that approach?

Also, if we went that route, it would be very natural to have an ImportSceneAsync extension method that takes a path instead of a gltf root object, and incorporate async deserialization there.

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't enforce that asset caches and paired to a glTF object. It's just decoupling the two rather than having them stored in an object. I should update the design to have UnityGLTFObject expose the AssetCache.

int sceneId = -1,
IProgress<int> progress = null,
CancellationToken cancellationToken = CancellationToken.None
);

/// <summary>
/// Creates a Unity GameObject from a glTF node
/// </summary>
/// <param name="unityGLTFObject">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 virtual Task<GameObject> ImportNodeAsync(
UnityGLTFObject unityGLTFObject,
int nodeId,
CancellationToken cancellationToken = CancellationToken.None
);

/// <summary>
/// Creates a Unity Texture2D from a glTF texture
/// </summary>
/// <param name="unityGLTFObject">Object which contains information to parse</param>
/// <param name="textureId">Texture to load from glTF object.</param>
/// <returns>The created Unity object</returns>
public virtual Task<Texture2D> ImportTextureAsync(
UnityGLTFObject unityGLTFObject,
int textureId,
CancellationToken cancellationToken = CancellationToken.None,
);
}

// UnityNode.cs
public virtual partial class Importer
{
private virtual Task<GameObject> ConstructNode();
}

// UnityTexture.cs
public virtual partial class Importer
{
private virtual Task<Texture2D> ConstructTexture();
}

public class ImporterConfig
{
public ImporterConfig();
Copy link

Choose a reason for hiding this comment

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

Why do we need this paramterless constructor?

Copy link
Author

Choose a reason for hiding this comment

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

The default constructor is used for when there are no options that are being set

Choose a reason for hiding this comment

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

Why not just pass in null if you don't want to set options? It seems weird that the options are to either provide extensions and options, or nothing.

public ImporterConfig(List<IUnityGLTFExtension> 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
Copy link

Choose a reason for hiding this comment

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

interface, not class
Also wondering about the name of this interface... I wonder if something like IResourceLocator would be more self explanatory? Not sure...

Copy link
Author

Choose a reason for hiding this comment

The 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> LoadStreamAsync(string uri, CancellationToken ct = CancellationToken.None);
Copy link

Choose a reason for hiding this comment

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

How about Uri uri instead of string uri. Just require at the contract level that a valid Uri is passed in.

Copy link
Author

Choose a reason for hiding this comment

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

Because it's not necessarily a web uri, which seems to be what the uri class is built around. Maybe path is a better parameter name?

Copy link
Contributor

Choose a reason for hiding this comment

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

The URI documentation says that it supports the file:// protocol, which in turn supports relative URIs. Seems like that would be enough.

}

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 Exporter
{
/// <param name="dataWriter">Interface for handling the streams of data to write out</param>
/// <param name="progress">Progress of export</param>
public Exporter(
IDataWriter dataWriter,
ExporterConfig exportConfig = new ExporterConfig()
);

/// <summary>
/// Exports a Unity object to a glTF file
/// </summary>
/// <param name="unityObject">The object to export</param>
/// <param name="exportConfig">Configuration of extension settings and export options</param>
/// <returns>Strongly typed wrapper of exported object</returns>
public Task<GLTFObject> ExportAsync(
GameObject unityObject,
IProgress<int> progress = null,
CancellationToken ct = CancellationToken.None
);
}

/// <summary>
/// Writes data for an export operation
/// </summary>
public class IDataWriter
{
Task<bool> WriteStreamAsync(string uri, Stream stream, CancellationToken ct = CancellationToken.None);
}

public class ExporterConfig
{
public ExporterConfig();
public ExporterConfig(List<IUnityGLTFExtension> registry, GLTFExportOptions exportOptions);
}

public class GLTFExportOptions
{
/// <summary>Whether to write the object out as a GLB</summary>
bool ShouldWriteGLB { get; set ; }

Choose a reason for hiding this comment

The 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; };

/// <summary>
/// Creates a Unity object out of the glTF schema object
/// </summary>
/// <param name="importer">The importer that is used to load the object</param>
/// <param name="unityGLTFObject">Object that is being loaded</param>
/// <param name="sceneId">Index object which resolves to object in GLTFRoot</param>
/// <returns>The loaded glTF scene as a GameObject hierarchy</returns>
Task<ExtensionReturnObject<GameObject>> CreateSceneAsync(Importer importer, UnityGLTFObject unityGLTFObject, GLTF.Schema.SceneId sceneId);
Copy link

Choose a reason for hiding this comment

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

Why do we need the Importer passed in here? Is it to allow the method to call the default load logic for a scene or something? If so, we should probably pass in an interface with the intended subset of the functionality. Otherwise, it is confusing how you are supposed to use this from the context of an extension (unless it is really expected that you might call any method on the importer from this context). Alternatively, we could restrict this further to only having the ability to invoke the base functionality for the type in question (e.g. from this context, you could call just one method for default scene loading, and in CreateNodeAsync you could call just one method for default node loading, etc.).

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this layer of intention could be implemented as an IExtensionContext interface instance passed into the function, where Importer implements IExtensionContext?

Copy link
Author

Choose a reason for hiding this comment

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

This mirrors the functionality of Babylon. An extension may want to call any of the functions on the loader. I suppose there is a limit to what extension could want to do (an image load is not going to probably edit the animation hierarchy). I'm not sure how an interface would look different than the internal loader functions.

One thought is that the Importer interface is publicly created and there is an ImporterImpl which is not able to be publicly constructed but all methods are publicly accessible

Copy link

Choose a reason for hiding this comment

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

Why is the UnityGLTFObject passed in here? Is it so the extension can basically cache data in it? If so, then I guess this implies that the UnityGLTFObject is not opaque from the standpoint of the consumer of this API?

Copy link
Author

Choose a reason for hiding this comment

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

Yes that UnityGLTFObject has the cache of data inside which may be needed for the extension

Choose a reason for hiding this comment

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

Pass the CancellationToken to the gltf extension methods as well.



/// <summary>
/// Creates a Unity object out of the node object
/// </summary>
/// <param name="importer">The importer that is used to load the object</param>
/// <param name="unityGLTFObject">Object that is being loaded</param>
/// <param name="nodeId">Node object from GLTFRoot</param>
/// <returns>The loaded glTF node as a GameObject</returns>
Task<ExtensionReturnObject<GameObject>> CreateNodeAsync(Importer importer, UnityGLTFObject unityGLTFObject, GLTF.Schmea.NodeId nodeId);

/// <summary>
/// Creates a Mesh primitive out of a mesh primitive schema object
/// </summary>
/// <param name="importer">The importer that is used to load the object</param>
/// <param name="unityGLTFObject">Object that is being loaded</param>
/// <param name="meshId">Mesh object to load from</param>
/// <param name="primitiveIndex">Primitive to load from the mesh</param>
/// <returns>Returns the mesh primitive</returns>
Task<ExtensionReturnObject<MeshPrimitive>> CreateMeshPrimitiveAsync(Importer importer, UnityGLTFObject unityGLTFObject, MeshId meshId, int primitiveIndex);
/// etc.
}

public struct ExtensionReturnObject<T>
{
ExtensionContinuationBehavior ContinuationBehavior;
T ReturnObject;
}

public enum ExtensionContinuationBehavior
{
NotHandled,
Handled
}

// Example calling pattern:

Choose a reason for hiding this comment

The 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");
Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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

Importer gltfImporter = new Importer(dataLoader);
await gltfImporter.ImportSceneAsync(sampleObject);
await gltfImporter.ImportSceneAsync(boxObject);
}
}