diff --git a/packages/phoenix-event-display/src/managers/three-manager/import-manager.ts b/packages/phoenix-event-display/src/managers/three-manager/import-manager.ts index 8cb98759..230f1911 100644 --- a/packages/phoenix-event-display/src/managers/three-manager/import-manager.ts +++ b/packages/phoenix-event-display/src/managers/three-manager/import-manager.ts @@ -31,6 +31,12 @@ export class ImportManager { private EVENT_DATA_ID: string; /** Object group ID containing detector geometries. */ private GEOMETRIES_ID: string; + /** + * Shared DRACOLoader instance for GLTF loading. + * Reused across all GLTF load operations to prevent Web Worker leaks. + * Must be disposed via cleanup() when the ImportManager is no longer needed. + */ + private dracoLoader: DRACOLoader | null = null; /** * Constructor for the import manager. @@ -48,6 +54,33 @@ export class ImportManager { this.GEOMETRIES_ID = GEOMETRIES_ID; } + /** + * Get or create the shared DRACOLoader instance. + * Lazily initializes the loader on first use to avoid unnecessary resource allocation. + * @returns The shared DRACOLoader instance. + */ + private getDRACOLoader(): DRACOLoader { + if (!this.dracoLoader) { + this.dracoLoader = new DRACOLoader(); + this.dracoLoader.setDecoderPath( + `https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/jsm/libs/draco/`, + ); + } + return this.dracoLoader; + } + + /** + * Cleanup and dispose all resources held by the ImportManager. + * Must be called when the ImportManager is no longer needed to prevent memory leaks. + * Specifically disposes the shared DRACOLoader which holds Web Worker threads. + */ + public cleanup(): void { + if (this.dracoLoader) { + this.dracoLoader.dispose(); + this.dracoLoader = null; + } + } + /** * Loads an OBJ (.obj) geometry from the given filename. * @param filename Path to the geometry. @@ -182,12 +215,7 @@ export class ImportManager { callback: (geometries?: Object3D, eventData?: Object3D) => void, ): Promise { const loader = new GLTFLoader(); - - const dracoLoader = new DRACOLoader(); - dracoLoader.setDecoderPath( - `https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/jsm/libs/draco/`, - ); - loader.setDRACOLoader(dracoLoader); + loader.setDRACOLoader(this.getDRACOLoader()); const sceneString = JSON.stringify(scene, null, 2); @@ -379,11 +407,7 @@ export class ImportManager { initiallyVisible: boolean, ): Promise { const loader = new GLTFLoader(); - const dracoLoader = new DRACOLoader(); - dracoLoader.setDecoderPath( - `https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/jsm/libs/draco/`, - ); - loader.setDRACOLoader(dracoLoader); + loader.setDRACOLoader(this.getDRACOLoader()); return new Promise((resolve, reject) => { loader.parse( @@ -491,11 +515,8 @@ export class ImportManager { name: string, ): Promise { const loader = new GLTFLoader(); - const dracoLoader = new DRACOLoader(); - dracoLoader.setDecoderPath( - `https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/jsm/libs/draco/`, - ); - loader.setDRACOLoader(dracoLoader); + loader.setDRACOLoader(this.getDRACOLoader()); + return new Promise((resolve, reject) => { loader.parse( geometry, diff --git a/packages/phoenix-event-display/src/managers/three-manager/index.ts b/packages/phoenix-event-display/src/managers/three-manager/index.ts index 484644bd..55fd0a6e 100644 --- a/packages/phoenix-event-display/src/managers/three-manager/index.ts +++ b/packages/phoenix-event-display/src/managers/three-manager/index.ts @@ -1794,5 +1794,8 @@ export class ThreeManager { if (this.selectionManager) { this.selectionManager.cleanup(); } + if (this.importManager) { + this.importManager.cleanup(); + } } } diff --git a/packages/phoenix-event-display/src/tests/managers/three-manager/import-manager.test.ts b/packages/phoenix-event-display/src/tests/managers/three-manager/import-manager.test.ts new file mode 100644 index 00000000..b00a097f --- /dev/null +++ b/packages/phoenix-event-display/src/tests/managers/three-manager/import-manager.test.ts @@ -0,0 +1,202 @@ +/** + * @jest-environment jsdom + */ +import { ImportManager } from '../../../managers/three-manager/import-manager'; +import { Plane, Vector3 } from 'three'; +import { DRACOLoader } from 'three/examples/jsm/loaders/DRACOLoader.js'; + +// Mock DRACOLoader to track instantiation and disposal +jest.mock('three/examples/jsm/loaders/DRACOLoader.js', () => { + return { + DRACOLoader: jest.fn().mockImplementation(() => ({ + setDecoderPath: jest.fn(), + dispose: jest.fn(), + preload: jest.fn(), + })), + }; +}); + +// Mock GLTFLoader +jest.mock('three/examples/jsm/loaders/GLTFLoader.js', () => { + return { + GLTFLoader: jest.fn().mockImplementation(() => ({ + setDRACOLoader: jest.fn(), + parse: jest.fn(), + load: jest.fn(), + })), + }; +}); + +describe('ImportManager', () => { + let importManager: ImportManager; + const clipPlanes = [ + new Plane(new Vector3(0, 1, 0), 0), + new Plane(new Vector3(0, -1, 0), 0), + ]; + const EVENT_DATA_ID = 'EventData'; + const GEOMETRIES_ID = 'Geometries'; + + beforeEach(() => { + // Clear all mock calls before each test + jest.clearAllMocks(); + importManager = new ImportManager(clipPlanes, EVENT_DATA_ID, GEOMETRIES_ID); + }); + + afterEach(() => { + // Cleanup after each test + if (importManager) { + importManager.cleanup(); + } + }); + + describe('DRACOLoader lifecycle management', () => { + it('should not create DRACOLoader until first GLTF load', () => { + // DRACOLoader should not be instantiated on construction + expect(DRACOLoader).not.toHaveBeenCalled(); + expect(importManager['dracoLoader']).toBeNull(); + }); + + it('should lazily create DRACOLoader on first getDRACOLoader call', () => { + // Access the private method to trigger lazy initialization + const dracoLoader = importManager['getDRACOLoader'](); + + expect(DRACOLoader).toHaveBeenCalledTimes(1); + expect(dracoLoader).toBeDefined(); + expect(dracoLoader.setDecoderPath).toHaveBeenCalled(); + }); + + it('should reuse the same DRACOLoader instance across multiple calls', () => { + // First call creates the instance + const firstLoader = importManager['getDRACOLoader'](); + // Second call should return the same instance + const secondLoader = importManager['getDRACOLoader'](); + + expect(DRACOLoader).toHaveBeenCalledTimes(1); + expect(firstLoader).toBe(secondLoader); + }); + + it('should dispose DRACOLoader on cleanup', () => { + // Initialize the DRACOLoader + const dracoLoader = importManager['getDRACOLoader'](); + + // Call cleanup + importManager.cleanup(); + + // Verify dispose was called + expect(dracoLoader.dispose).toHaveBeenCalledTimes(1); + expect(importManager['dracoLoader']).toBeNull(); + }); + + it('should handle cleanup when DRACOLoader was never initialized', () => { + // DRACOLoader should not exist yet + expect(importManager['dracoLoader']).toBeNull(); + + // Cleanup should not throw + expect(() => importManager.cleanup()).not.toThrow(); + }); + + it('should allow re-initialization after cleanup', () => { + // Initialize and cleanup + importManager['getDRACOLoader'](); + importManager.cleanup(); + + expect(DRACOLoader).toHaveBeenCalledTimes(1); + expect(importManager['dracoLoader']).toBeNull(); + + // Re-initialize + const newLoader = importManager['getDRACOLoader'](); + + expect(DRACOLoader).toHaveBeenCalledTimes(2); + expect(newLoader).toBeDefined(); + }); + + it('should set correct decoder path for DRACOLoader', () => { + const dracoLoader = importManager['getDRACOLoader'](); + + // Verify setDecoderPath was called with a CDN URL + expect(dracoLoader.setDecoderPath).toHaveBeenCalledWith( + expect.stringContaining('cdn.jsdelivr.net/npm/three@'), + ); + expect(dracoLoader.setDecoderPath).toHaveBeenCalledWith( + expect.stringContaining('/examples/jsm/libs/draco/'), + ); + }); + }); + + describe('GLTF loading methods use shared DRACOLoader', () => { + it('parsePhnxScene should use shared DRACOLoader', () => { + // Mock the callback + const callback = jest.fn(); + + // Start loading (this will fail but we're testing DRACOLoader usage) + importManager.parsePhnxScene({}, callback).catch(() => { + // Expected to fail without valid GLTF data + }); + + // Verify DRACOLoader was created only once + expect(DRACOLoader).toHaveBeenCalledTimes(1); + }); + + it('multiple GLTF operations should share the same DRACOLoader', async () => { + const callback = jest.fn(); + + // Simulate multiple load operations + importManager.parsePhnxScene({}, callback).catch(() => {}); + importManager.parsePhnxScene({}, callback).catch(() => {}); + importManager.parsePhnxScene({}, callback).catch(() => {}); + + // Should still only have one DRACOLoader instance + expect(DRACOLoader).toHaveBeenCalledTimes(1); + }); + }); + + describe('Memory leak prevention', () => { + it('should not leak DRACOLoader instances on repeated operations', () => { + // Simulate the problematic pattern that was causing leaks + // Before fix: each operation created a new DRACOLoader + // After fix: all operations share one DRACOLoader + + for (let i = 0; i < 10; i++) { + importManager['getDRACOLoader'](); + } + + // Only one DRACOLoader should have been created + expect(DRACOLoader).toHaveBeenCalledTimes(1); + }); + + it('should properly dispose all resources on cleanup', () => { + // Use the loader + const dracoLoader = importManager['getDRACOLoader'](); + + // Verify initial state + expect(importManager['dracoLoader']).not.toBeNull(); + + // Cleanup + importManager.cleanup(); + + // Verify cleanup + expect(dracoLoader.dispose).toHaveBeenCalled(); + expect(importManager['dracoLoader']).toBeNull(); + }); + }); +}); + +describe('ImportManager integration with ThreeManager cleanup', () => { + it('should be safe to call cleanup multiple times', () => { + const importManager = new ImportManager( + [new Plane(new Vector3(0, 1, 0), 0)], + 'EventData', + 'Geometries', + ); + + // Initialize the DRACOLoader + importManager['getDRACOLoader'](); + + // Multiple cleanup calls should not throw + expect(() => { + importManager.cleanup(); + importManager.cleanup(); + importManager.cleanup(); + }).not.toThrow(); + }); +});