From 7c7845a5e837a3697c42c08406bb406583eef571 Mon Sep 17 00:00:00 2001 From: Sebastien DUMETZ Date: Mon, 15 Jul 2024 13:53:36 +0200 Subject: [PATCH] cancellable fetch for derivatives fix a race condition on derivative load callback. Explain early return cases in comments --- source/client/components/CVAssetReader.ts | 4 +- source/client/components/CVModel2.ts | 49 +++++++++++--- source/client/io/ModelReader.ts | 82 ++++++++++++++++++----- source/client/models/Derivative.ts | 15 ++++- 4 files changed, 123 insertions(+), 27 deletions(-) diff --git a/source/client/components/CVAssetReader.ts b/source/client/components/CVAssetReader.ts index ab7b7298..9d668bfe 100644 --- a/source/client/components/CVAssetReader.ts +++ b/source/client/components/CVAssetReader.ts @@ -104,10 +104,10 @@ export default class CVAssetReader extends Component return this.fileLoader.getText(url); } - async getModel(assetPath: string): Promise + async getModel(assetPath: string, {signal}:{signal?:AbortSignal}={}): Promise { const url = this.assetManager.getAssetUrl(assetPath); - return this.modelLoader.get(url); + return this.modelLoader.get(url, {signal}); } async getGeometry(assetPath: string): Promise diff --git a/source/client/components/CVModel2.ts b/source/client/components/CVModel2.ts index f4d397f4..1bae649f 100644 --- a/source/client/components/CVModel2.ts +++ b/source/client/components/CVModel2.ts @@ -149,6 +149,12 @@ export default class CVModel2 extends CObject3D private _derivatives = new DerivativeList(); private _activeDerivative: Derivative = null; + /** + * Separate from activeDerivative because when switching quality levels, + * we want to keep the active model until the new one is ready + */ + private _loadingDerivative :Derivative = null; + private _visible: boolean = true; private _boxFrame: Mesh = null; private _localBoundingBox = new Box3(); @@ -306,7 +312,7 @@ export default class CVModel2 extends CObject3D } else if (ins.quality.changed) { const derivative = this.derivatives.select(EDerivativeUsage.Web3D, ins.quality.value); - if (derivative && derivative !== this.activeDerivative) { + if (derivative) { this.loadDerivative(derivative) .catch(error => { console.warn("Model.update - failed to load derivative"); @@ -749,7 +755,7 @@ export default class CVModel2 extends CObject3D // load sequence of derivatives one by one return sequence.reduce((promise, derivative) => { - return promise.then(() => { this.loadDerivative(derivative)}); + return promise.then(() => this.loadDerivative(derivative)); }, Promise.resolve()); } @@ -757,32 +763,54 @@ export default class CVModel2 extends CObject3D * Loads and displays the given derivative. * @param derivative */ - protected loadDerivative(derivative: Derivative): Promise + protected async loadDerivative(derivative: Derivative): Promise { if(!this.node || !this.assetReader) { // TODO: Better way to handle active loads when node has been disposed? console.warn("Model load interrupted."); return; } + if(this._loadingDerivative && this._loadingDerivative != derivative) { + this._loadingDerivative.unload(); + this._loadingDerivative = null; + } + if (this._activeDerivative == derivative){ + return; + } + if(this._loadingDerivative == derivative) { + return new Promise(resolve=> this._loadingDerivative.on("load", resolve)); + } + + this._loadingDerivative = derivative; return derivative.load(this.assetReader) .then(() => { - if (!derivative.model || !this.node || - (this._activeDerivative && derivative.data.quality != this.ins.quality.value)) { + if ( !derivative.model + || !this.node + || (this._activeDerivative && derivative.data.quality != this.ins.quality.value) + ) { + //Either derivative is not valid, or we have been disconnected, + // or this derivative is no longer needed as it's not the requested quality + // AND we already have _something_ to display derivative.unload(); return; } + if(this._activeDerivative && this._activeDerivative == derivative){ + //a race condition can happen where a derivative fires it's callback but it's already the active one. + return; + } + // set asset manager flag for initial model load if(!this.assetManager.initialLoad && !this._activeDerivative) { this.assetManager.initialLoad = true; } if (this._activeDerivative) { - this.removeObject3D(this._activeDerivative.model); + if(this._activeDerivative.model) this.removeObject3D(this._activeDerivative.model); this._activeDerivative.unload(); } - this._activeDerivative = derivative; + this._loadingDerivative = null; this.addObject3D(derivative.model); this.renderer.activeSceneComponent.scene.updateMatrixWorld(true); @@ -855,8 +883,11 @@ export default class CVModel2 extends CObject3D this.emit({ type: "model-load", quality: derivative.data.quality }); //this.getGraphComponent(CVSetup).navigation.ins.zoomExtents.set(); - }) - .catch(error => Notification.show(`Failed to load model derivative: ${error.message}`)); + }).catch(error =>{ + if(error.name == "AbortError" || error.name == "ABORT_ERR") return; + console.error(error); + Notification.show(`Failed to load model derivative: ${error.message}`) + }); } protected addObject3D(object: Object3D) diff --git a/source/client/io/ModelReader.ts b/source/client/io/ModelReader.ts index b5fc370e..33cf4055 100644 --- a/source/client/io/ModelReader.ts +++ b/source/client/io/ModelReader.ts @@ -17,7 +17,7 @@ //import resolvePathname from "resolve-pathname"; import UberPBRAdvMaterial from "client/shaders/UberPBRAdvMaterial"; -import { LoadingManager, Object3D, Scene, Mesh, MeshStandardMaterial, SRGBColorSpace } from "three"; +import { LoadingManager, Object3D, Scene, Mesh, MeshStandardMaterial, SRGBColorSpace, LoaderUtils } from "three"; import {DRACOLoader} from 'three/examples/jsm/loaders/DRACOLoader.js'; import {MeshoptDecoder} from "three/examples/jsm/libs/meshopt_decoder.module.js"; @@ -39,6 +39,8 @@ export default class ModelReader protected renderer: CRenderer; protected gltfLoader :GLTFLoader; + protected loading :Recordany, onerror: (e:Error)=>any, signal:AbortSignal}[], abortController :AbortController}> = {} + protected customDracoPath = null; set dracoPath(path: string) @@ -100,24 +102,74 @@ export default class ModelReader return ModelReader.mimeTypes.indexOf(mimeType) >= 0; } - get(url: string): Promise + get(url: string, {signal}:{signal?:AbortSignal}={}): Promise { - return new Promise((resolve, reject) => { - this.gltfLoader.load(url, gltf => { - resolve(this.createModelGroup(gltf)); - }, null, error => { - if(this.gltfLoader === null || this.gltfLoader.dracoLoader === null) { - // HACK to avoid errors when component is removed while loading still in progress. - // Remove once Three.js supports aborting requests again. - resolve(null); + let resourcePath = LoaderUtils.extractUrlBase( url ); + return this.loadModel(url, {signal}) + .then(data=>this.gltfLoader.parseAsync(data, resourcePath)) + .then(gltf=>this.createModelGroup(gltf)); + } + + /** + * + * extracted from GLTFLoader https://github.com/mrdoob/three.js/blob/master/examples/jsm/loaders/GLTFLoader.js#L186 + * Adds an abort capability while waiting for [THREE.js #23070](https://github.com/mrdoob/three.js/pull/23070) + * This implementation does not quite match what is proposed there because it allows _some_ duplicate requests to be aborted without aborting the `fetch` + */ + async loadModel( url :string, {signal} :{signal?:AbortSignal}={}) :Promise{ + + // Tells the LoadingManager to track an extra item, which resolves after + // the model is fully loaded. This means the count of items loaded will + // be incorrect, but ensures manager.onLoad() does not fire early. + if(signal){ + const onAbort = ()=>{ + const idx = this.loading[url]?.listeners.findIndex(l=>l.signal === signal) ?? -1; + if(idx == -1) return; + const {onerror} = this.loading[url].listeners.splice(idx, 1)[0]; + onerror(new DOMException(signal.reason, "AbortError")); + + if(this.loading[url].listeners.length == 0){ + ENV_DEVELOPMENT && console.debug("Abort request for URL : ", url); + this.loading[url].abortController.abort(); + }else{ + ENV_DEVELOPMENT && console.debug("Abort listener for URL : %s (%d)", url, this.loading[url].listeners.length ); + } + } + signal.addEventListener("abort", onAbort); + } + + if(!this.loading[url]?.listeners.length){ + this.loadingManager.itemStart( url ); + + this.loading[url] = {listeners:[], abortController: new AbortController()}; + + fetch(url, { + signal: this.loading[url].abortController.signal, + }).then(r=>{ + if(!r.ok){ + throw new Error( `fetch for "${r.url}" responded with ${r.status}: ${r.statusText}`); } - else { - console.error(`failed to load '${url}': ${error}`); - reject(new Error(error as any)); + //Skip all the progress tracking from FileLoader since we don't use it. + return r.arrayBuffer(); + }).then(data=> { + this.loadingManager.itemEnd( url ); + this.loading[url].listeners.forEach(({onload})=>onload(data)); + }, (e)=>{ + this.loading[url].listeners.forEach(({onerror})=>onerror(e)); + if(e.name != "AbortError" && e.name != "ABORT_ERR"){ + console.error(e); + this.loadingManager.itemError( url ); } - }) + this.loadingManager.itemEnd( url ); + }).finally(()=>{ + delete this.loading[url]; + }); + } + + return new Promise((onload, onerror)=>{ + this.loading[url].listeners.push({onload, onerror, signal}); }); - } + } protected createModelGroup(gltf): Object3D { diff --git a/source/client/models/Derivative.ts b/source/client/models/Derivative.ts index dfc1e23d..798de125 100755 --- a/source/client/models/Derivative.ts +++ b/source/client/models/Derivative.ts @@ -58,6 +58,13 @@ export default class Derivative extends Document model: Object3D = null; + abortControl :AbortController = null; + + constructor(json?: IDerivativeJSON){ + super(json); + this.addEvent("load"); + } + dispose() { this.unload(); @@ -70,10 +77,15 @@ export default class Derivative extends Document throw new Error("can't load, not a Web3D derivative"); } + if(this.abortControl){ + console.warn("Aborting inflight derivative load"); + this.abortControl.abort("Derivative load cancelled"); //This should not happen, but if in doubt, cancel duplicates + } + this.abortControl = new AbortController(); const modelAsset = this.findAsset(EAssetType.Model); if (modelAsset) { - return assetReader.getModel(modelAsset.data.uri) + return assetReader.getModel(modelAsset.data.uri, {signal: this.abortControl.signal}) .then(object => { if (this.model) { disposeObject(this.model); @@ -114,6 +126,7 @@ export default class Derivative extends Document unload() { + this.abortControl?.abort(); if (this.model) { disposeObject(this.model); this.model = null;