From 9d7f8bcc987024b4f9f0b2b097d9a92a7c80db81 Mon Sep 17 00:00:00 2001 From: Paul Du Bois Date: Thu, 13 Feb 2020 15:23:24 -0800 Subject: [PATCH] b/149241458: Don't try to open null gltf buffer URIs Manually cherry-picked from go/tbcl/118853 Change-Id: Id000b015b9ea3122d41154be31c6158428546338 --- .../TiltBrush/Scripts/Editor/Glb1Importer.cs | 3 +- .../TiltBrush/Scripts/Editor/Glb2Importer.cs | 3 +- .../TiltBrush/Scripts/Gltf/Gltf1Schema.cs | 2 +- .../TiltBrush/Scripts/Gltf/Gltf2Schema.cs | 10 ++-- .../Scripts/Gltf/GltfSchemaCommon.cs | 2 +- .../TiltBrush/Scripts/Gltf/ImportGltf.cs | 54 +++++++++++++++---- 6 files changed, 56 insertions(+), 18 deletions(-) diff --git a/UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb1Importer.cs b/UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb1Importer.cs index cd327696..c64fe6d5 100644 --- a/UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb1Importer.cs +++ b/UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb1Importer.cs @@ -39,12 +39,11 @@ public class Glb1Importer : ScriptedImporter { }; public override void OnImportAsset(AssetImportContext ctx) { - StringReader gltfStream = new StringReader(GlbParser.GetJsonChunkAsString(ctx.assetPath)); IUriLoader loader = new BufferedStreamLoader( ctx.assetPath, Path.GetDirectoryName(ctx.assetPath)); ImportGltf.GltfImportResult result = ImportGltf.Import( - GltfSchemaVersion.GLTF1, gltfStream, loader, null, kOptions); + ctx.assetPath, loader, null, kOptions); // The "identifier" param passed here is supposed to be: // - Unique to this asset diff --git a/UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb2Importer.cs b/UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb2Importer.cs index b315c4fe..b03f1b36 100644 --- a/UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb2Importer.cs +++ b/UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb2Importer.cs @@ -39,12 +39,11 @@ public class Glb2Importer : ScriptedImporter { }; public override void OnImportAsset(AssetImportContext ctx) { - StringReader gltfStream = new StringReader(GlbParser.GetJsonChunkAsString(ctx.assetPath)); IUriLoader loader = new BufferedStreamLoader( ctx.assetPath, Path.GetDirectoryName(ctx.assetPath)); ImportGltf.GltfImportResult result = ImportGltf.Import( - GltfSchemaVersion.GLTF2, gltfStream, loader, null, kOptions); + ctx.assetPath, loader, null, kOptions); // The "identifier" param passed here is supposed to be: // - Unique to this asset diff --git a/UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf1Schema.cs b/UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf1Schema.cs index 1ee5b1dc..2ed18603 100644 --- a/UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf1Schema.cs +++ b/UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf1Schema.cs @@ -86,7 +86,7 @@ protected override void Dispose(bool disposing) { } /// Map glTFid values (ie, string names) names to the objects they refer to - public override void Dereference(IUriLoader uriLoader = null) { + public override void Dereference(bool isGlb, IUriLoader uriLoader = null) { // "dereference" all the names scenePtr = scenes[scene]; foreach (var pair in buffers) { diff --git a/UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf2Schema.cs b/UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf2Schema.cs index 0d1e9dea..844a2938 100644 --- a/UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf2Schema.cs +++ b/UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf2Schema.cs @@ -87,15 +87,19 @@ protected override void Dispose(bool disposing) { } /// Map gltfIndex values (ie, int indices) names to the objects they refer to - public override void Dereference(IUriLoader uriLoader = null) { + public override void Dereference(bool isGlb, IUriLoader uriLoader = null) { // "dereference" all the indices scenePtr = scenes[scene]; for (int i = 0; i < buffers.Count; i++) { Gltf2Buffer buffer = buffers[i]; buffer.gltfIndex = i; + if (buffer.uri == null && !(i == 0 && isGlb)) { + Debug.LogErrorFormat("Buffer {0} isGlb {1} has null uri", i, isGlb); + // leave the data buffer null + return; + } + if (uriLoader != null) { - // Only id 0 may lack a URI; this indicates that it is the binary chunk. - Debug.Assert(! (i != 0 && buffer.uri == null)); buffer.data = uriLoader.Load(buffer.uri); } } diff --git a/UnitySDK/Assets/TiltBrush/Scripts/Gltf/GltfSchemaCommon.cs b/UnitySDK/Assets/TiltBrush/Scripts/Gltf/GltfSchemaCommon.cs index bc390b4f..e368dafa 100644 --- a/UnitySDK/Assets/TiltBrush/Scripts/Gltf/GltfSchemaCommon.cs +++ b/UnitySDK/Assets/TiltBrush/Scripts/Gltf/GltfSchemaCommon.cs @@ -158,7 +158,7 @@ public abstract class GltfRootBase : IDisposable { public abstract IEnumerable Materials { get; } public abstract IEnumerable Meshes { get; } - public abstract void Dereference(IUriLoader uriLoader = null); + public abstract void Dereference(bool isGlb, IUriLoader uriLoader = null); // Disposable pattern, with Dispose(void) and Dispose(bool), as recommended by: // https://docs.microsoft.com/en-us/dotnet/api/system.idisposable diff --git a/UnitySDK/Assets/TiltBrush/Scripts/Gltf/ImportGltf.cs b/UnitySDK/Assets/TiltBrush/Scripts/Gltf/ImportGltf.cs index 95d4944f..2c8218a5 100644 --- a/UnitySDK/Assets/TiltBrush/Scripts/Gltf/ImportGltf.cs +++ b/UnitySDK/Assets/TiltBrush/Scripts/Gltf/ImportGltf.cs @@ -109,6 +109,44 @@ public void Dispose() { } } + public class GltfFileInfo : IDisposable { + public string Path { get; private set; } + public GltfSchemaVersion Version { get; private set; } + public bool IsGlb { get; private set; } + public TextReader Reader { get; private set; } + + public GltfFileInfo(string path) { + Path = path; + if (GlbParser.GetGlbVersion(path) is uint glbFormatVersion) { + IsGlb = true; + Version = (glbFormatVersion == 1 ? GltfSchemaVersion.GLTF1 : GltfSchemaVersion.GLTF2); + Reader = new StringReader(GlbParser.GetJsonChunkAsString(path)); + } else { + IsGlb = false; + string json = File.ReadAllText(path); + var gltf1Or2 = JsonConvert.DeserializeObject(json); + var versionString = gltf1Or2?.asset?.version ?? "2"; // default to 2, I guess? + Version = versionString.StartsWith("1") ? GltfSchemaVersion.GLTF1 : GltfSchemaVersion.GLTF2; + Reader = new StringReader(json); + } + } + + public void Dispose() { + if (Reader != null) { + Reader.Dispose(); + } + } + } + + // Used only by GetVersionAndReader. The layout of a gltf file changes significantly + // between gltf v1 and v2, but thankfully the location of the version info didn't change. + #pragma warning disable 0649 + [Serializable] + private class Gltf1Or2 { + public GltfAsset asset; + } + #pragma warning restore 0649 + /// /// Import a gltf model. /// If you would like to perform some of the work off the main thread, use @@ -122,10 +160,10 @@ public void Dispose() { /// /// public static GltfImportResult Import( - GltfSchemaVersion gltfVersion, TextReader gltfStream, IUriLoader uriLoader, + string gltfOrGlbPath, IUriLoader uriLoader, IImportMaterialCollector materialCollector, GltfImportOptions options) { - using (var state = BeginImport(gltfVersion, gltfStream, uriLoader, options)) { + using (var state = BeginImport(gltfOrGlbPath, uriLoader, options)) { IEnumerable meshCreator; GltfImportResult result = EndImport(state, uriLoader, materialCollector, out meshCreator); foreach (var unused in meshCreator) { @@ -204,17 +242,15 @@ private static void CheckCompatibility( /// /// An object which should be passed to and then disposed public static ImportState BeginImport( - GltfSchemaVersion gltfVersion, TextReader stream, IUriLoader uriLoader, - GltfImportOptions options) { - if (stream == null) { throw new ArgumentNullException("stream"); } + string gltfOrGlbPath, IUriLoader uriLoader, GltfImportOptions options) { if (uriLoader == null) { throw new ArgumentNullException("uriLoader"); } - SanityCheckImportOptions(options); - using (var reader = new JsonTextReader(stream)) { - var root = DeserializeGltfRoot(gltfVersion, reader); + using (var info = new GltfFileInfo(gltfOrGlbPath)) + using (var reader = new JsonTextReader(info.Reader)) { + var root = DeserializeGltfRoot(info.Version, reader); if (root == null) { throw new NullReferenceException("root"); } - root.Dereference(uriLoader); + root.Dereference(info.IsGlb, uriLoader); // Convert attribute names to the ones we expect. // This may eg overwrite TEXCOORD_0 with _TB_UNITY_TEXCOORD_0 (which will contain more data)