Skip to content

Commit

Permalink
b/149241458: Don't try to open null gltf buffer URIs
Browse files Browse the repository at this point in the history
Manually cherry-picked from go/tbcl/118853

Change-Id: Id000b015b9ea3122d41154be31c6158428546338
  • Loading branch information
dubois committed Feb 13, 2020
1 parent d5c68f7 commit 9d7f8bc
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 18 deletions.
3 changes: 1 addition & 2 deletions UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb1Importer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions UnitySDK/Assets/TiltBrush/Scripts/Editor/Glb2Importer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf1Schema.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
10 changes: 7 additions & 3 deletions UnitySDK/Assets/TiltBrush/Scripts/Gltf/Gltf2Schema.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
2 changes: 1 addition & 1 deletion UnitySDK/Assets/TiltBrush/Scripts/Gltf/GltfSchemaCommon.cs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public abstract class GltfRootBase : IDisposable {
public abstract IEnumerable<GltfMaterialBase> Materials { get; }
public abstract IEnumerable<GltfMeshBase> 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
Expand Down
54 changes: 45 additions & 9 deletions UnitySDK/Assets/TiltBrush/Scripts/Gltf/ImportGltf.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Gltf1Or2>(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

/// <summary>
/// Import a gltf model.
/// If you would like to perform some of the work off the main thread, use
Expand All @@ -122,10 +160,10 @@ public void Dispose() {
/// <seealso cref="ImportGltf.BeginImport" />
/// <seealso cref="ImportGltf.EndImport" />
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<Null> meshCreator;
GltfImportResult result = EndImport(state, uriLoader, materialCollector, out meshCreator);
foreach (var unused in meshCreator) {
Expand Down Expand Up @@ -204,17 +242,15 @@ private static void CheckCompatibility(
/// </summary>
/// <returns>An object which should be passed to <seealso cref="ImportGltf.EndImport"/> and then disposed</returns>
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)
Expand Down

0 comments on commit 9d7f8bc

Please sign in to comment.