fix(io): Handle duplicate URIs on read/write (#1511)

packages/core/src/io/reader-context.ts

@@ -1,4 +1,5 @@
 import type { JSONDocument } from '../json-document.js';
+import type { Document } from '../document.js';
 import type {
 	Accessor,
 	Animation,
@@ -13,6 +14,7 @@ import type {
 	TextureInfo,
 } from '../properties/index.js';
 import type { GLTF } from '../types/gltf.js';
+import type { ILogger } from '../utils/logger.js';
 
 /**
  * Model class providing glTF Transform objects representing each definition in the glTF file, used
@@ -22,6 +24,11 @@ import type { GLTF } from '../types/gltf.js';
  * @hidden
  */
 export class ReaderContext {
+	public readonly document: Document;
+	// TODO(v5): Rename to jsonDocument;
+	public readonly jsonDoc: JSONDocument;
+	private readonly logger: ILogger;
+
 	public buffers: Buffer[] = [];
 	public bufferViews: Uint8Array[] = [];
 	public bufferViewBuffers: Buffer[] = [];
@@ -36,7 +43,13 @@ export class ReaderContext {
 	public animations: Animation[] = [];
 	public scenes: Scene[] = [];
 
-	constructor(public readonly jsonDoc: JSONDocument) {}
+	private _usedURIs = new Set<string>();
+
+	constructor(document: Document, jsonDoc: JSONDocument) {
+		this.document = document;
+		this.jsonDoc = jsonDoc;
+		this.logger = document.getLogger();
+	}
 
 	public setTextureInfo(textureInfo: TextureInfo, textureInfoDef: GLTF.ITextureInfo): void {
 		this.textureInfos.set(textureInfo, textureInfoDef);
@@ -67,4 +80,15 @@ export class ReaderContext {
 			textureInfo.setWrapT(samplerDef.wrapT);
 		}
 	}
+
+	public requestURI(uri: string): string {
+		// https://github.com/KhronosGroup/glTF/issues/2446
+		if (this._usedURIs.has(uri)) {
+			this.logger.warn(`Duplicate resource URI, "${uri}".`);
+			return '';
+		}
+
+		this._usedURIs.add(uri);
+		return uri;
+	}
 }

packages/core/src/io/reader.ts

@@ -40,7 +40,7 @@ export class GLTFReader {
 
 		/* Reader context. */
 
-		const context = new ReaderContext(jsonDoc);
+		const context = new ReaderContext(document, jsonDoc);
 
 		/** Asset. */
 
@@ -98,7 +98,7 @@ export class GLTFReader {
 			if (bufferDef.extras) buffer.setExtras(bufferDef.extras);
 
 			if (bufferDef.uri && bufferDef.uri.indexOf('__') !== 0) {
-				buffer.setURI(bufferDef.uri);
+				buffer.setURI(context.requestURI(bufferDef.uri));
 			}
 
 			return buffer;
@@ -172,7 +172,7 @@ export class GLTFReader {
 			} else if (imageDef.uri !== undefined) {
 				texture.setImage(jsonDoc.resources[imageDef.uri]);
 				if (imageDef.uri.indexOf('__') !== 0) {
-					texture.setURI(imageDef.uri);
+					texture.setURI(context.requestURI(imageDef.uri));
 				}
 			}
 

packages/core/src/io/writer-context.ts

@@ -184,10 +184,26 @@ export class WriterContext {
 		} else {
 			const extension = ImageUtils.mimeTypeToExtension(texture.getMimeType());
 			imageDef.uri = this.imageURIGenerator.createURI(texture, extension);
-			this.jsonDoc.resources[imageDef.uri] = data;
+			this.assignResourceURI(imageDef.uri, data);
 		}
 	}
 
+	public assignResourceURI(uri: string, data: Uint8Array): void {
+		const resources = this.jsonDoc.resources;
+
+		// https://github.com/KhronosGroup/glTF/issues/2446
+		if (uri in resources && data !== resources[uri]) {
+			throw new Error(`Resource URI "${uri}" already assigned to different data.`);
+		}
+
+		if (uri in resources) {
+			this.logger.warn(`Duplicate resource URI, "${uri}".`);
+			return;
+		}
+
+		resources[uri] = data;
+	}
+
 	/**
 	 * Returns implicit usage type of the given accessor, related to grouping accessors into
 	 * buffer views. Usage is a superset of buffer view target, including ARRAY_BUFFER and

packages/core/src/io/writer.ts

@@ -437,9 +437,11 @@ export class GLTFWriter {
 			.filter((extension) => extension.prewriteTypes.includes(PropertyType.BUFFER))
 			.forEach((extension) => extension.prewrite(context, PropertyType.BUFFER));
 
-		const hasBinaryResources =
-			root.listAccessors().length > 0 || root.listTextures().length > 0 || context.otherBufferViews.size > 0;
-		if (hasBinaryResources && root.listBuffers().length === 0) {
+		const needsBuffer =
+			root.listAccessors().length > 0 ||
+			context.otherBufferViews.size > 0 ||
+			(root.listTextures().length > 0 && options.format === Format.GLB);
+		if (needsBuffer && root.listBuffers().length === 0) {
 			throw new Error('Buffer required for Document resources, but none was found.');
 		}
 
@@ -552,7 +554,7 @@ export class GLTFWriter {
 
 				// Write buffer views to buffer.
 				bufferDef.byteLength = bufferByteLength;
-				jsonDoc.resources[uri] = BufferUtils.concat(buffers);
+				context.assignResourceURI(uri, BufferUtils.concat(buffers));
 			}
 
 			json.buffers!.push(bufferDef);

packages/core/test/io/platform-io.test.ts

@@ -179,3 +179,88 @@ test('glb with unknown chunk', async (t) => {
 	const node = document.getRoot().listNodes()[0];
 	t.is(node.getName(), 'RootNode', 'parses nodes');
 });
+
+test('read duplicate buffer URIs', async (t) => {
+	const io = await createPlatformIO();
+
+	// https://github.com/KhronosGroup/glTF/issues/2446
+	const jsonDocument: JSONDocument = {
+		json: {
+			asset: { version: '2.0' },
+			buffers: [
+				{ uri: 'scene.bin', byteLength: 20 },
+				{ uri: 'scene.bin', byteLength: 20 },
+			],
+		},
+		resources: { 'scene.bin': new Uint8Array(20) },
+	};
+
+	const document = await io.readJSON(jsonDocument);
+
+	const bufferURIs = document
+		.getRoot()
+		.listBuffers()
+		.map((buffer) => buffer.getURI());
+
+	t.deepEqual(bufferURIs, ['scene.bin', ''], 'removes duplicate buffer URIs');
+});
+
+test('read duplicate image URIs', async (t) => {
+	const io = await createPlatformIO();
+
+	// https://github.com/KhronosGroup/glTF/issues/2446
+	const image = new Uint8Array(20);
+	const jsonDocument: JSONDocument = {
+		json: {
+			asset: { version: '2.0' },
+			images: [{ uri: 'color.png' }, { uri: 'color.png' }],
+		},
+		resources: { 'color.png': image },
+	};
+
+	const document = await io.readJSON(jsonDocument);
+
+	const imageURIs = document
+		.getRoot()
+		.listTextures()
+		.map((texture) => texture.getURI());
+	const images = document
+		.getRoot()
+		.listTextures()
+		.map((texture) => texture.getImage());
+
+	t.deepEqual(imageURIs, ['color.png', ''], 'removes duplicate image URIs');
+	t.deepEqual(images, [image, image], 'loads duplicate image data');
+});
+
+test('write duplicate buffer URIs', async (t) => {
+	const io = await createPlatformIO();
+
+	// https://github.com/KhronosGroup/glTF/issues/2446
+	const document = new Document();
+	const buffer1 = document.createBuffer().setURI('scene.bin');
+	const buffer2 = document.createBuffer().setURI('scene.bin');
+	document.createAccessor().setBuffer(buffer1).setArray(new Float32Array(10).fill(1));
+	document.createAccessor().setBuffer(buffer2).setArray(new Float32Array(10).fill(2));
+
+	await t.throwsAsync(() => io.writeJSON(document), { message: /Resource URI/i }, 'duplicate buffer URIs');
+
+	buffer2.setURI('aux.bin');
+
+	t.truthy(await io.writeJSON(document), 'unique buffer URIs');
+});
+
+test('write duplicate image URIs', async (t) => {
+	const io = await createPlatformIO();
+
+	// https://github.com/KhronosGroup/glTF/issues/2446
+	const document = new Document();
+	const image1 = document.createTexture().setImage(new Uint8Array(2)).setURI('color.png');
+	document.createTexture().setImage(new Uint8Array(1)).setURI('color.png');
+
+	await t.throwsAsync(() => io.writeJSON(document), { message: /Resource URI/i }, 'duplicate image URIs');
+
+	image1.setURI('normal.png');
+
+	t.truthy(await io.writeJSON(document), 'unique image URIs');
+});