Remove three.js types from public LoadSpec interface#405
Conversation
|
Might investigate the possibility of allowing |
toloudis
left a comment
toloudis
left a comment
There was a problem hiding this comment.
I love this change and wonder whether we have other threejs type leaks through the public api of this package
|
|
||
| import { CImageInfo, type ImageInfo } from "../ImageInfo.js"; | ||
| import { LoadSpec } from "./IVolumeLoader.js"; | ||
| import { LoadSpec, type Region, regionToBox3 } from "./IVolumeLoader.js"; |
There was a problem hiding this comment.
nit: is there a place that is more "math utils"-like to put the regionToBox3 and box3toRegion functions?
There was a problem hiding this comment.
There's utils/num_utils.ts, but it seems to be geared more towards formatting numbers for display
| const { loader } = getLoader(loaderId); | ||
|
|
||
| return await loader.createImageInfo(rebuildLoadSpec(loadSpec)); | ||
| return await loader.createImageInfo(loadSpec); |
There was a problem hiding this comment.
is this safe to do now because it now uses primitive types instead of the threejs classes?
There was a problem hiding this comment.
Yep! All rebuildLoadSpec was doing was re-creating the Box3 to get the prototype chain back.
|
I wonder if there is some static analysis check we can do to make sure no threejs types are part of the public api... |
I think there are some exposed types (Color, Vector3, Texture) through some of the drawable objects and the colorize configuration added to vole-core. |
4 participants
Review time: smallish (~15min)
Resolves #254: removes the three.js type
Box3fromLoadSpecin favor of a vanilla JS variant. This has two benefits:Box3's prototype chain when aLoadSpecarrives from the volume load worker.