Update shouldRender so it support multiple models #6
Conversation
packages/tools/viewer/src/viewer.ts
Outdated
| export type Model = IDisposable & | ||
| ModelInternal & |
There was a problem hiding this comment.
I don't think we want to merge ModelInternal into Model. I think we want to do the opposite, and then use ModelInternal for the private backing fields.
type ModelInternal = Model & {
_animationPlaying(): boolean;
_shouldRender(): boolean;
};
...
private _loadedModelsBacking: ModelInternal[] = [];
private _activeModelBacking: Nullable<ModelInternal> = null;
packages/tools/viewer/src/viewer.ts
Outdated
| private _shouldModelRenderPredicate(model: Model) { | ||
| return model._shouldRender(); | ||
| } |
There was a problem hiding this comment.
I think it is actually ok for this to just be an inline lambda. I did some quick memory profiling and it doesn't look like the lambdas on line 1060 actually result in any memory allocations. I'm guessing the JS engine is smart enough to see that this lambda doesn't need to be re-allocated since it doesn't capture any local state.
… to activate a model from the protected API
Make _setActiveModel private
packages/tools/viewer/src/viewer.ts
Outdated
| this._setActiveModel(await this._loadModel(source, options, abortController.signal), Object.assign({ source, interpolateCamera: false }, options)); | ||
| const model = await this._loadModel(source, options, abortController.signal); | ||
| model.makeActive(Object.assign({ source, interpolateCamera: false }, options)); | ||
| } else { |
There was a problem hiding this comment.
Sorry I added this else block but somehow I didn't notice that line 1152 already sets the _activeModelBacking to null, so I don't think this else block actually does anything.
No description provided.