Update ThreeJS imports #8787
Update ThreeJS imports #8787
Conversation
📝 Walkthrough""" WalkthroughThis change refactors all imports of the "three" library throughout the codebase, replacing broad namespace imports with selective named imports of only the required classes, types, and constants. Corresponding type annotations and references are updated to use the imported names directly. No logic or control flow is altered. Changes
Estimated code review effort2 (~20 minutes) Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
frontend/javascripts/viewer/controller/renderer.ts (1)
23-41:getRenderer()fallback returns plain{}– will explode on first useWhen no
#render-canvaselement is found the ternary expression yields{}which is then cast toWebGLRenderer. Any subsequent call such asrenderer.dispose()will throw.- renderer = ( - renderCanvasElement != null - ? // Create a WebGL2 renderer - new WebGLRenderer({ … }) - : {} - ) as WebGLRenderer; + if (renderCanvasElement == null) { + throw new Error("#render-canvas element not found – renderer cannot be created."); + } + + renderer = new WebGLRenderer({ + canvas: renderCanvasElement, + preserveDrawingBuffer: true, + antialias: Store.getState().userConfiguration.antialiasRendering, + });Fail fast or return
nulland let callers handle it – either is safer than a masquerading plain object.frontend/javascripts/viewer/geometries/cube.ts (1)
71-81: Material instances are leaked on every highlight toggle
getLineMaterial()creates a newLineBasicMaterialeach call, andsetIsHighlightedreplaces the material without disposing the old one. This steadily increases GPU memory.+disposeMaterial(mat: LineBasicMaterial) { + mat.dispose(); +} setIsHighlighted(highlighted: boolean) { if (highlighted === this.isHighlighted) { return; } this.isHighlighted = highlighted; - this.getMeshes().forEach((mesh) => { - mesh.material = this.getLineMaterial(); - }); + this.getMeshes().forEach((mesh) => { + this.disposeMaterial(mesh.material as LineBasicMaterial); + mesh.material = this.getLineMaterial(); + }); app.vent.emit("rerender"); }Consider caching one light & one dark material instead of re-creating every time.
Also applies to: 180-184
🧹 Nitpick comments (25)
frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts (1)
97-109: Minor mutability nitpick in matrix handling.
Matrix4.invert()mutatesviewportRotationMatrixin-place; if the original matrix will ever be re-used later, a.clone().invert()would be safer.-const viewportRotationMatrixInverted = viewportRotationMatrix.invert(); +const viewportRotationMatrixInverted = viewportRotationMatrix.clone().invert();Currently the variable is not re-used, so the change is optional.
frontend/javascripts/libs/order_points_with_mst.ts (1)
66-94: Potential O(N²) scalability bottleneck.
computeMSTbuilds every edge (≃ N²/2) and sorts them – fine for a few hundred points but quadratic for large data sets.If point sets can grow to thousands, consider a spatial data structure (e.g., KD-tree) or Prim’s algorithm with a binary heap to get O(N²) ➝ O(E log N).
No change required if point counts stay small.
frontend/javascripts/viewer/model/bucket_data_handling/bucket.ts (1)
101-103: UninitialisedvisualizationColorcan be avoided.The field triggers the TS 2564 suppression comment. Initialise it once in the constructor to drop the suppression and avoid undefined reads.
constructor( elementClass: ElementClass, zoomedAddress: BucketAddress, @@ this.data = null; + this.visualizationColor = new Color(0x000000); // default blackfrontend/javascripts/viewer/controller/combinations/skeleton_handlers.ts (3)
4-4: Consider a less ambiguous alias for the runtime Vector3
Vector3is imported twice – once astype Vector3(viewer constants) and once as the runtimeVector3class from ThreeJS (aliased here asThreeVector3).
The current alias works but can be confusing when scanning the file. Renaming the runtime import to something likeThreeJsVector3(or the type alias toVec3) would remove the cognitive overhead.-import { Euler, Matrix4, Scene, Vector3 as ThreeVector3 } from "three"; +import { Euler, Matrix4, Scene, Vector3 as ThreeJsVector3 } from "three";Follow-up: adjust the two occurrences (
movementVector,new ThreeVector3()) below.
173-175: Shared mutable helper instances – safe but fragile
flycamRotationEuler,flycamRotationMatrix, andmovementVectorare reused across calls tomoveNode.
While this avoids repeated allocations, the objects are mutated during the call. IfmoveNodeis ever called re-entrantly (e.g. via async callbacks or event batching) the helpers will hold intermediate state and yield incorrect results.If absolute performance is not critical here, instantiating the helpers inside the function keeps the implementation side-effect free.
428-430: Potential allocation hotspot
maybeGetNodeIdFromPositioncreates a newSceneon every invocation. For rapid mouse-move-driven picking this can add measurable GC pressure.If the scene contents are stateless between calls, consider pooling / reusing the scene object and only re-attaching the freshly built picking node:
-const pickingScene = new Scene(); -pickingScene.add(pickingNode); +const pickingScene = _pickingScene ?? ( _pickingScene = new Scene() ); +pickingScene.clear(); // remove previous children +pickingScene.add(pickingNode);(Not critical – just something to watch for during profiling.)
frontend/javascripts/viewer/api/api_latest.ts (2)
1420-1432: Reduce per-frame allocations in tweenInside the
centerPositionAnimatedtween the code allocates freshQuaternionandEulerinstances on every frame. For long animations this becomes a hot path.A tiny optimisation is to hoist the scratch objects outside the
onUpdatecallback and just.copy()/.set()them:-const interpolatedQuaternion = new Quaternion().slerpQuaternions( - startQuaternion, - endQuaternion, - t, -); -const interpolatedEuler = new Euler().setFromQuaternion(interpolatedQuaternion, "XYZ"); +interpolatedQuaternion.slerpQuaternions(startQuaternion, endQuaternion, t); +interpolatedEuler.setFromQuaternion(interpolatedQuaternion, "XYZ");You’d declare:
const interpolatedQuaternion = new Quaternion(); const interpolatedEuler = new Euler();once before the tween is constructed.
This keeps GC pressure minimal without affecting readability.
1458-1464: Hoist scratch Quaternion / Euler objects (follow-up to previous comment)Same allocation comment as above – these lines can reuse the cached helpers proposed earlier.
frontend/javascripts/test/model/accessors/view_mode_accessors.spec.ts (1)
233-236: Avoid per-iteration heap churn by re-using helper vectorsInside the nested loops this line chain allocates three new
ThreeVector3instances for every offset-rotation combination. In tight test suites that’s fine, but in production code this pattern would create a lot of garbage.-const rotatedAndScaledOffset = new ThreeVector3(offset[0], offset[1], 0) - .applyEuler(new Euler(...rotationInRadian, "ZYX")) - .multiply(new ThreeVector3(...scaleFactor)) - .toArray(); +static _tmpVec = new ThreeVector3(); +static _tmpScale = new ThreeVector3(); +const rotatedAndScaledOffset = _tmpVec.set(offset[0], offset[1], 0) + .applyEuler(new Euler(...rotationInRadian, "ZYX")) + .multiply(_tmpScale.set(...scaleFactor)) + .toArray();Purely optional here (tests), but worth keeping in mind for hot code paths.
frontend/javascripts/libs/stl_exporter.ts (1)
37-48: Update error message to match new import styleThe runtime check still complains about “THREE.BufferGeometry” although the namespace import was removed. Minor wording tweak keeps messages consistent.
-throw new Error("STLExporter: Geometry is not of type THREE.BufferGeometry."); +throw new Error("STLExporter: Geometry is not of type BufferGeometry.");frontend/javascripts/viewer/model/bucket_data_handling/layer_rendering_manager.ts (1)
11-12: Minor: prefer value-less import when the symbol is types-only
DataTextureis imported withtype—good.
Double-check no runtime usage later; otherwise drop thetypemodifier.frontend/javascripts/viewer/view/rendering_utils.ts (1)
66-86: Generic helper mutates its argument silently
adaptCameraToCurrentClippingDistanceclones the camera but re-assigns it to the parameter, hiding side-effect from callers.Not critical, but returning a new variable (
const cloned = camera.clone() as T; … return cloned;) clarifies intent.frontend/javascripts/test/api/api_skeleton_latest.spec.ts (1)
25-29: Tiny optimisation: usemapfor vector conversion-const toRadian = (arr: Vector3): Vector3 => [ - MathUtils.degToRad(arr[0]), - MathUtils.degToRad(arr[1]), - MathUtils.degToRad(arr[2]), -]; +const toRadian = (arr: Vector3): Vector3 => arr.map(MathUtils.degToRad) as Vector3;Reduces repetition; no functional change.
frontend/javascripts/viewer/view/arbitrary_view.ts (1)
160-171: Avoid per-frame heap allocations in hot loop
new Matrix4()is created twice for every render pass; in typical scenes that is every animation frame and quickly shows up in the GC profile.-const rotY = new Matrix4().makeRotationY(Math.PI); -… -camera.matrix.multiply(rotY); -const translate = new Matrix4().makeTranslation(...this.cameraPosition); -camera.matrix.multiply(translate); +// Declare once at module scope or as private fields +this.tmpRotY.makeRotationY(Math.PI); +this.tmpTranslation.makeTranslation(...this.cameraPosition); +camera.matrix.multiply(this.tmpRotY); +camera.matrix.multiply(this.tmpTranslation);Small change → measurable frame-time stability on mid-tier devices.
frontend/javascripts/viewer/geometries/arbitrary_plane.ts (1)
64-66: Cache the temporaryMatrix4to reduce GC churn
updateMeshexecutes every time the plane is re-rendered and instantiates two freshMatrix4s. Re-use one instance instead:-const meshMatrix = new Matrix4(); -… -mesh.matrix.multiply(new Matrix4().makeRotationY(Math.PI)); +this.tmpMatrix.identity(); +… +this.tmpMatrix.makeRotationY(Math.PI); +mesh.matrix.multiply(this.tmpMatrix);Minor, but this code is hit extremely often during navigation.
Also applies to: 85-86
frontend/javascripts/libs/trackball_controls.ts (1)
580-583: Restoreconstructorafter prototype overwriteAfter
Object.create(EventDispatcher.prototype)theconstructorproperty now points toEventDispatcher, which breaksinstanceofchecks and class-name based debugging.TrackballControls.prototype = Object.create(EventDispatcher.prototype); +TrackballControls.prototype.constructor = TrackballControls;frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts (1)
1077-1097: Consider object re-use incheckLineIntersection
Ray,Raycaster, and twoVector3s are allocated on every call. During flood-fill this may be executed thousands of times and becomes allocation-heavy. A tiny object pool or module-level singletons would cut the pressure without affecting readability.Not blocking, but worth a follow-up if flood-fill is a performance hotspot.
frontend/javascripts/viewer/geometries/materials/edge_shader.ts (1)
17-18:materialshould be declared with definite-assignment to satisfystrictPropertyInitialization- material: RawShaderMaterial; + material!: RawShaderMaterial;Avoids the need to disable
strictin tsconfig and silences the initialisation warning.frontend/javascripts/viewer/view/plane_view.ts (1)
86-94: Consider reusing the sameVector3instances for the staticup/lookAtvectorsThese allocations happen once per view, so the impact is tiny, but caching constants avoids unnecessary GC churn:
- this.cameras[OrthoViews.PLANE_XY].up = new ThreeVector3(0, -1, 0); - … - this.cameras[OrthoViews.TDView].up = new ThreeVector3(0, 0, -1); +const UP_NEG_Y = new ThreeVector3(0, -1, 0); +const UP_NEG_Z = new ThreeVector3(0, 0, -1); +this.cameras[OrthoViews.PLANE_XY].up = UP_NEG_Y; +… +this.cameras[OrthoViews.TDView].up = UP_NEG_Z;Pure micro-optimisation—feel free to skip if you prefer current readability.
frontend/javascripts/viewer/geometries/crosshair.ts (1)
59-62: Avoid per-frame object allocations insideupdate()
update()is executed every render tick but allocates three fresh objects (Matrix4,Matrix4,Vector3) each call, creating avoidable GC pressure. Cache these as private fields and reuse them:- mesh.matrix.multiply(new Matrix4().makeRotationY(Math.PI)); - mesh.matrix.multiply(new Matrix4().makeTranslation(0, 0, 0.5)); - mesh.matrix.scale(new Vector3(this.scale, this.scale, this.scale)); + // tmp matrices/vectors are declared once on the class (e.g. this.tmpRotY, this.tmpTranslate, this.tmpScale) + this.tmpRotY.makeRotationY(Math.PI); + this.tmpTranslate.makeTranslation(0, 0, 0.5); + this.tmpScale.set(this.scale, this.scale, this.scale); + + mesh.matrix.multiply(this.tmpRotY); + mesh.matrix.multiply(this.tmpTranslate); + mesh.matrix.scale(this.tmpScale);This cuts three allocations per frame.
frontend/javascripts/viewer/controller/segment_mesh_controller.ts (2)
36-37: DuplicateMesh.prototype.raycastpatch
Mesh.prototype.raycast = acceleratedRaycastis already set once inscene_controller.ts.
Keeping the patch in two places is harmless but noisy and risks diverging behaviour if changed later. Centralise it (e.g. athree-setup.ts) and import once.
213-216: Unnecessary allocation in_.getdefault
_.get(obj, path, new Group())constructs theGroupeven when the value already exists.- new Group(), + undefined,and afterwards:
let targetGroup = _.get(this.meshesGroupsPerSegmentId, keys) as Group | undefined; if (!targetGroup) { targetGroup = new Group(); _.set(this.meshesGroupsPerSegmentId, keys, targetGroup); }Avoids thousands of throw-away
Groupinstances during bulk mesh loads.frontend/javascripts/viewer/geometries/skeleton.ts (3)
40-42: Narrowmeshtyping to the concrete ThreeJS classes
meshis always created via eithernew Pointsornew LineSegments.
Keeping it asObject3Dloses all compile-time safety (e.g. access to.geometry,.material,.isPoints, …).Consider tightening the type:
-type Buffer = { +type Buffer = { capacity: number; nextIndex: number; geometry: BufferGeometryWithBufferAttributes; - mesh: Object3D; + mesh: Points | LineSegments; };If you’d like to stay fully generic, introduce
<T extends Object3D>and thread that throughBuffer/BufferCollectioninstead.
This is purely a typing improvement – no runtime impact.
74-76: Return a concrete class frombuildMeshLikewise,
NodeBufferHelperType.buildMeshcan returnPointsinstead ofObject3D:- buildMesh(geometry: BufferGeometry, material: RawShaderMaterial): Object3D { - return new Points(geometry, material); - }, + buildMesh(geometry: BufferGeometry, material: RawShaderMaterial): Points { + return new Points(geometry, material); + },Same for the edge helper (
LineSegments). This cascades nicely into the stricterBuffertyping suggested above.
498-501:getAllNodescan exposePoints[]for better type safety
buffer.meshis guaranteed to bePointsfor the node collection, so exposingPoints[]is safe and helps callers avoid manual casts:-getAllNodes(): Object3D[] { - return this.nodes.buffers.map((buffer) => buffer.mesh); -} +getAllNodes(): Points[] { + return this.nodes.buffers.map((buffer) => buffer.mesh as Points); +}Optional, but makes downstream code clearer.
frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts
Show resolved
Hide resolved
|
@daniel-wer do you maybe have some time to review this? |
|
Yes, I'll have a look later today or tomorrow |
daniel-wer
requested review from
daniel-wer
and removed request for
MichaelBuessemeyer
|
@hotzenklotz I had a look at all the nitpick suggestions. Nothing earth-shattering, a lot of suggestions I'd classify as premature optimization, but there were some nice small improvements to type safety, GC churn and code readability in there which I've addressed. |
This PR updates the ThreeJs imports for smaller bundle size, better style points and maybe improved typing(?). The main change is to replace all the very general
import * as THREE from "three"with more specific import, e.g.import {WebGLRenderer, Vector3} from "three".In some files, the updated import lead to name conflicts for
Vector3fromlibs/constantsandthree, so renamed theThree.Vector3 as ThreeVector3in these cases.The ThreeJS
yarninstallation comes with prebuild "binaries" (see./node_moudles/three/build/three.module.js) and the source files. I configure webpack to use the source files for bundling which resulted ~300kb smaller bundles (and likely less parsing time by browser).Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)