Proposal for new architecture for UnityGLTF library #259

blgrossMS · 2018-10-01T17:30:40Z

No description provided.

CLAassistant · 2018-10-01T17:30:47Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Blake Gross 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.}

StephenHodgson · 2018-10-01T19:01:50Z

vNext/UnityGLTFv2.cs

+    /// <param name="textureId">Texture to load from glTF object.</param>        
+    /// <returns>The created Unity object</returns>
+    public Task<Texture2D> ImportTextureAsync(
+        UnityGLTFObject unityGLTFFObject,


unityGLTFFObject has two FFs

StephenHodgson · 2018-10-01T19:02:09Z

vNext/UnityGLTFv2.cs

+    /// <param name="nodeId">Node of the glTF object to load.</param>        
+    /// <returns>The created Unity object</returns>
+    public Task<GameObject> ImportNodeAsync(
+        UnityGLTFObject unityGLTFFObject,


unityGLTFFObject has two FFs

vNext/UnityGLTFv2.cs

ryantrem · 2018-10-02T19:17:57Z

vNext/UnityGLTF_vnext_proposal.md

+
+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


This block of code is out of date... delete?

ryantrem · 2018-10-02T19:19:42Z

vNext/UnityGLTFv2.cs

+/// <summary>
+/// Handles importing object from schema into Unity and handles exporting of objects from Unity into schema
+/// </summary>
+public partial class UnityGLTFImporter


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.

Agreed. Partial classes are also kind of hard to hunt down as well.

ryantrem · 2018-10-02T19:22:11Z

vNext/UnityGLTFv2.cs

+    /// <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>
+    public Task<GameObject> ImportSceneAsync(


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.

vNext/UnityGLTFv2.cs

ryantrem · 2018-10-02T19:26:01Z

vNext/UnityGLTFv2.cs

+/// </summary>
+public class IDataLoader
+{
+    Task<Stream> LoadStream(string uri);


LoadStreamAsync, plus add a CancellationToken parameter.

vNext/UnityGLTFv2.cs

ryantrem · 2018-10-09T15:53:47Z

vNext/UnityGLTFv2.cs

 {
-    public UnityGLTFImporterConfig();
-    public UnityGLTFImporterConfig(GLTFExtensionRegistry registry, GLTFImportOptions importOptions);
+    public ImporterConfig();


Why do we need this paramterless constructor?

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

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.

ryantrem · 2018-10-09T15:57:58Z

vNext/UnityGLTFv2.cs

@@ -66,7 +73,7 @@ public class UnityGLTFImporterConfig
 /// </summary>
 public class IDataLoader


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

It's not really locating resources. It's only resolving them to a stream.

ryantrem · 2018-10-09T15:58:36Z

vNext/UnityGLTFv2.cs

@@ -66,7 +73,7 @@ public class UnityGLTFImporterConfig
 /// </summary>
 public class IDataLoader
 {
-    Task<Stream> LoadStream(string uri);
+    Task<Stream> LoadStreamAsync(string uri, CancellationToken ct = CancellationToken.None);


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

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?

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

ryantrem · 2018-10-09T16:08:38Z

vNext/UnityGLTFv2.cs

+    /// <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);


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

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

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

ryantrem · 2018-10-09T16:09:35Z

vNext/UnityGLTFv2.cs

+    /// <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);


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?

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

ryantrem · 2018-10-09T16:17:09Z

vNext/UnityGLTFv2.cs

@@ -175,7 +209,8 @@ public async void LoadGLBs()
    UnityGLTFObject boxObject = new UnityGLTFObject("http://samplemodels/box.glb");


Is this assuming that deserialization is synchronous? Seems like this would naturally lead people down a bad path.

No, this is just passing in a uri that will be resolved during the actual load

ryantrem · 2018-10-09T16:20:23Z

vNext/UnityGLTFv2.cs

-        UnityGLTFObject unityGLTFFObject,
-        int sceneId = -1
+    public virtual Task<GameObject> ImportSceneAsync(
+        UnityGLTFObject unityGLTFObject,


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.

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.

ryantrem · 2018-10-11T22:45:13Z

vNext/UnityGLTFv2.cs

+    /// <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);


Pass the CancellationToken to the gltf extension methods as well.

ryantrem · 2018-10-15T16:34:29Z

vNext/UnityGLTFv2.cs

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


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?

ryantrem · 2018-10-15T16:36:38Z

vNext/UnityGLTFv2.cs

+    Handled
+}
+
+// Example calling pattern:


No longer valid, correct?

blgrossMS · 2018-12-05T06:51:29Z

Active work has slowed down on this. We are more taking the approach now of slowly changing the existing code to more closely match the proposed architecture. Anyone who wants to help out can!

Blake Gross added 3 commits September 20, 2018 21:54

sample cs file and vNext proposal

152ffe4

added extension proposal to cs file

f94c80e

architecture update

5cd205d

StephenHodgson reviewed Oct 1, 2018

View reviewed changes

vNext/UnityGLTFv2.cs Show resolved Hide resolved

ryantrem reviewed Oct 2, 2018

View reviewed changes

stevenvergenz approved these changes Oct 2, 2018

View reviewed changes

ryantrem reviewed Oct 2, 2018

View reviewed changes

Blake Gross added 2 commits October 8, 2018 17:17

updated proposal

c068452

adding continuation behavior

57f547b

ryantrem reviewed Oct 9, 2018

View reviewed changes

ryantrem reviewed Oct 11, 2018

View reviewed changes

Blake Gross added 2 commits October 14, 2018 21:08

added example usage

7aa3e51

added data lifecycle section

2321bfc

ryantrem reviewed Oct 15, 2018

View reviewed changes

vNext/UnityGLTFv2.cs

Handled

}

// Example calling pattern:

Copy link

ryantrem Oct 15, 2018

Choose a reason for hiding this comment

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

No longer valid, correct?

blgrossMS mentioned this pull request Oct 24, 2018

Support native multi-platform non-blocking texture loading #9

Open

blgrossMS mentioned this pull request Apr 12, 2019

Add more options for the importer #391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for new architecture for UnityGLTF library #259

Proposal for new architecture for UnityGLTF library #259

blgrossMS commented Oct 1, 2018

CLAassistant commented Oct 1, 2018 •

edited

StephenHodgson Oct 1, 2018

StephenHodgson Oct 1, 2018

ryantrem Oct 2, 2018

ryantrem Oct 2, 2018

StephenHodgson Oct 2, 2018

ryantrem Oct 2, 2018

ryantrem Oct 2, 2018

ryantrem Oct 9, 2018

blgrossMS Oct 9, 2018

ryantrem Oct 15, 2018

ryantrem Oct 9, 2018

blgrossMS Oct 9, 2018

ryantrem Oct 9, 2018

blgrossMS Oct 9, 2018

stevenvergenz Oct 9, 2018

ryantrem Oct 9, 2018

stevenvergenz Oct 9, 2018

blgrossMS Oct 9, 2018

ryantrem Oct 9, 2018

blgrossMS Oct 9, 2018

ryantrem Oct 9, 2018

blgrossMS Oct 9, 2018

ryantrem Oct 9, 2018 •

edited

blgrossMS Oct 9, 2018

ryantrem Oct 11, 2018

ryantrem Oct 15, 2018

ryantrem Oct 15, 2018

blgrossMS commented Dec 5, 2018


		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

		@@ -66,7 +73,7 @@ public class UnityGLTFImporterConfig
		/// </summary>
		public class IDataLoader

		@@ -175,7 +209,8 @@ public async void LoadGLBs()
		UnityGLTFObject boxObject = new UnityGLTFObject("http://samplemodels/box.glb");

Proposal for new architecture for UnityGLTF library #259

Are you sure you want to change the base?

Proposal for new architecture for UnityGLTF library #259

Conversation

blgrossMS commented Oct 1, 2018

CLAassistant commented Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryantrem Oct 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blgrossMS commented Dec 5, 2018

CLAassistant commented Oct 1, 2018 •

edited

ryantrem Oct 9, 2018 •

edited