Skip to content

Commit

Permalink
fix: ensure models are loaded in the same order the add*-functions ar…
Browse files Browse the repository at this point in the history
…e called (#3956)

* fix: ensure models are loaded in the same order the add*-functions are called
  • Loading branch information
haakonflatval-cognite authored Nov 28, 2023
1 parent 1bb7b10 commit 4963595
Show file tree
Hide file tree
Showing 4 changed files with 181 additions and 16 deletions.
8 changes: 7 additions & 1 deletion viewer/packages/360-images/src/Image360Facade.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@ import { DefaultImage360Collection } from './collection/DefaultImage360Collectio
import { IconCullingScheme } from './icons/IconCollection';
import { Image360RevisionEntity } from './entity/Image360RevisionEntity';
import { Image360AnnotationFilterOptions } from './annotation/types';
import { AsyncSequencer } from '@reveal/utilities/src/AsyncSequencer';

export class Image360Facade<T> {
private readonly _image360Collections: DefaultImage360Collection[];
private readonly _rayCaster: THREE.Raycaster;
private readonly _image360Cache: Image360LoadingCache;

private readonly _loadSequencer = new AsyncSequencer();

get collections(): DefaultImage360Collection[] {
return this._image360Collections;
}
Expand Down Expand Up @@ -54,13 +57,16 @@ export class Image360Facade<T> {
postTransform = new THREE.Matrix4(),
preComputedRotation = true
): Promise<DefaultImage360Collection> {
const sequencer = this._loadSequencer.getNextSequencer();

const image360Collection = await this._entityFactory.create(
dataProviderFilter,
postTransform,
preComputedRotation,
annotationFilter
);
this._image360Collections.push(image360Collection);

await sequencer(() => this._image360Collections.push(image360Collection));
return image360Collection;
}

Expand Down
59 changes: 44 additions & 15 deletions viewer/packages/api/src/public/migration/Cognite3DViewer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ import {
} from '@reveal/360-images';
import { Image360ApiHelper } from '../../api-helpers/Image360ApiHelper';
import html2canvas from 'html2canvas';
import { AsyncSequencer, SequencerFunction } from '../../../../utilities/src/AsyncSequencer';

type Cognite3DViewerEvents =
| 'click'
Expand Down Expand Up @@ -178,6 +179,11 @@ export class Cognite3DViewer {

private readonly spinner: Spinner;

/**
* Enbles us to ensure models are added in the order their load is initialized.
*/
private readonly _addModelSequencer: AsyncSequencer = new AsyncSequencer();

private get revealManager(): RevealManager {
return this._revealManagerHelper.revealManager;
}
Expand Down Expand Up @@ -707,15 +713,19 @@ export class Cognite3DViewer {
);
}

const type = await this.determineModelType(options.modelId, options.revisionId);
switch (type) {
case 'cad':
return this.addCadModel(options);
case 'pointcloud':
return this.addPointCloudModel(options);
default:
throw new Error('Model is not supported');
}
const modelLoadSequencer = this._addModelSequencer.getNextSequencer<void>();

return (async () => {
const type = await this.determineModelType(options.modelId, options.revisionId);
switch (type) {
case 'cad':
return this.addCadModelWithSequencer(options, modelLoadSequencer);
case 'pointcloud':
return this.addPointCloudModelWithSequencer(options, modelLoadSequencer);
default:
throw new Error('Model is not supported');
}
})();
}

/**
Expand All @@ -733,15 +743,26 @@ export class Cognite3DViewer {
* });
* ```
*/
async addCadModel(options: AddModelOptions): Promise<CogniteCadModel> {
addCadModel(options: AddModelOptions): Promise<CogniteCadModel> {
const modelLoaderSequencer = this._addModelSequencer.getNextSequencer<void>();
return this.addCadModelWithSequencer(options, modelLoaderSequencer);
}

private async addCadModelWithSequencer(
options: AddModelOptions,
modelLoadSequencer: SequencerFunction<void>
): Promise<CogniteCadModel> {
const nodesApiClient = this._dataSource.getNodesApiClient();

const { modelId, revisionId } = options;

const cadNode = await this._revealManagerHelper.addCadModel(options);

const model3d = new CogniteCadModel(modelId, revisionId, cadNode, nodesApiClient);
this._models.push(model3d);
this._sceneHandler.addCadModel(cadNode, cadNode.cadModelIdentifier);
await modelLoadSequencer(() => {
this._models.push(model3d);
this._sceneHandler.addCadModel(cadNode, cadNode.cadModelIdentifier);
});

return model3d;
}
Expand All @@ -761,17 +782,25 @@ export class Cognite3DViewer {
* });
* ```
*/
async addPointCloudModel(options: AddModelOptions): Promise<CognitePointCloudModel> {
addPointCloudModel(options: AddModelOptions): Promise<CognitePointCloudModel> {
const sequencerFunction = this._addModelSequencer.getNextSequencer<void>();
return this.addPointCloudModelWithSequencer(options, sequencerFunction);
}

private async addPointCloudModelWithSequencer(options: AddModelOptions, modelLoadSequencer: SequencerFunction<void>) {
if (options.geometryFilter) {
throw new Error('geometryFilter is not supported for point clouds');
}

const { modelId, revisionId } = options;

const pointCloudNode = await this._revealManagerHelper.addPointCloudModel(options);
const model = new CognitePointCloudModel(modelId, revisionId, pointCloudNode);
this._models.push(model);

this._sceneHandler.addPointCloudModel(pointCloudNode, pointCloudNode.modelIdentifier);
await modelLoadSequencer(() => {
this._models.push(model);
this._sceneHandler.addPointCloudModel(pointCloudNode, pointCloudNode.modelIdentifier);
});

return model;
}
Expand Down
46 changes: 46 additions & 0 deletions viewer/packages/utilities/src/AsyncSequencer.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*!
* Copyright 2023 Cognite AS
*/

import { AsyncSequencer } from './AsyncSequencer';

import { jest } from '@jest/globals';

describe(AsyncSequencer.name, () => {
test("doesn't mix up calls in the right order", async () => {
const asyncSequencer = new AsyncSequencer();

const fn = jest.fn<(n: number) => void>();

const sequencer1 = asyncSequencer.getNextSequencer();
const sequencer2 = asyncSequencer.getNextSequencer();

await sequencer1(() => fn(1));
await sequencer2(() => fn(2));

expect(fn).toHaveBeenCalledTimes(2);
expect(fn.mock.calls[0][0]).toBe(1);
expect(fn.mock.calls[1][0]).toBe(2);
});

test('reverses calls in the wrong order', async () => {
const asyncSequencer = new AsyncSequencer();

const fn = jest.fn<(n: number) => void>();

const sequencer1 = asyncSequencer.getNextSequencer();
const sequencer2 = asyncSequencer.getNextSequencer();
const sequencer3 = asyncSequencer.getNextSequencer();

const res2 = sequencer2(() => fn(2));
const res3 = sequencer3(() => fn(3));
const res1 = sequencer1(() => fn(1));

await Promise.all([res2, res3, res1]);

expect(fn).toHaveBeenCalledTimes(3);
expect(fn.mock.calls[0][0]).toBe(1);
expect(fn.mock.calls[1][0]).toBe(2);
expect(fn.mock.calls[2][0]).toBe(3);
});
});
84 changes: 84 additions & 0 deletions viewer/packages/utilities/src/AsyncSequencer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*!
* Copyright 2023 Cognite AS
*/

export type SequencerFunction<T> = (region: () => Promise<T> | T) => Promise<T>;

/**
* AsyncSequencer - helper class for making sure a sequence of operations
* are performed in the same order, while permitting dependant computations
* to be performed in arbitrary order.
* See the following example of a function that loads data and puts it in an
* array:.
*
* ```
* function loadData(id: any): Promise<void> {
* const result = await expensiveFetchOperation(id);
* this._array.push(result); // Shared array used for all retrieved data
* }
* ```
*
* Calling `loadData` multiple times sequentially without awaiting may
* cause the retrieved data to be pushed to the array in an arbitrary order.
* This may be fine in some cases, but not in others.
* `AsyncSequencer` guarantees the order in the following way:
*
* ```
* const asyncSequencer = new AsyncSequencer();
* ...
* // Same function signature as before
* function loadData(id: any): Promise<void> {
*
* // `getNextSequencer` returns a _sequencer_ function that takes another
* // function (the "critical region") as input and ensures it is run
* // after the critical region of the previous _sequencer_ function
* // retrieved from the `asyncSequencer` object with `getNextSequencer`,
* // and before the next such sequencer's critical region
* const sequencer = asyncSequencer.getNextSequencer<void>();
*
* // The following line still runs and finishes at arbitrary times
* // across different calls to `loadData` ...
* const result = await expensiveFetchOperation(id);
*
* // ... However, the function given to `sequencer` will always
* // run in the same order as the `sequencer`s were created with
* // `getNextSequencer`
* await sequencer(() => {
* this._array.push(result)
* });
* }
* ```
* Note that this approach allows `expensiveFetchOperation` to be run in parallel
* while still guaranteeing the order of the results.
* Also, be aware that if `loadData` had been declared `async`, it is not certain
* that the calls to `getNextSequencer` would have been in the same order as
* the calls to the corresponding `loadData`.
*/
export class AsyncSequencer {
private _currentPromise: Promise<void> = Promise.resolve();

/**
* Returns a `sequencer` function that guarantees that the
* function it is called with is run after the previous `sequencer`'s
* function, and before the next one's.
*/
getNextSequencer<T>(): SequencerFunction<T> {
const lastPromise = this._currentPromise;
let resolver: () => void;

this._currentPromise = new Promise(res => {
resolver = res;
});

const func = async (region: () => T | Promise<T>): Promise<T> => {
await lastPromise;
const result = await region();

resolver();

return result;
};

return func;
}
}

0 comments on commit 4963595

Please sign in to comment.