diff --git a/src/common/model/optics/BaseElement.ts b/src/common/model/optics/BaseElement.ts index dd1a1b9..013f904 100644 --- a/src/common/model/optics/BaseElement.ts +++ b/src/common/model/optics/BaseElement.ts @@ -56,6 +56,13 @@ export abstract class BaseElement implements OpticalElement { */ public reassignIdForDeserialization(id: string): void { this._id = id; + const match = /^element-(\d+)$/.exec(id); + if (match) { + const restoredSequence = Number(match[1]); + if (Number.isSafeInteger(restoredSequence)) { + nextElementId = Math.max(nextElementId, restoredSequence + 1); + } + } } /** diff --git a/src/common/model/optics/OpticalElementPhetioObject.ts b/src/common/model/optics/OpticalElementPhetioObject.ts index 81bd242..971f9e4 100644 --- a/src/common/model/optics/OpticalElementPhetioObject.ts +++ b/src/common/model/optics/OpticalElementPhetioObject.ts @@ -2,6 +2,7 @@ * PhET-iO instrumented wrapper for one {@link OpticalElement} in a {@link PhetioGroup}. */ +import { Emitter } from "scenerystack/axon"; import type { Tandem } from "scenerystack/tandem"; import { IOType, ObjectLiteralIO, PhetioObject } from "scenerystack/tandem"; import OpticsLabNamespace from "../../../OpticsLabNamespace.js"; @@ -12,6 +13,8 @@ type ElementStateRecord = Record; export default class OpticalElementPhetioObject extends PhetioObject { public opticalElement: OpticalElement; + /** Fires when PhET-iO state replaces the wrapped model object. */ + public readonly opticalElementReplacedEmitter = new Emitter<[OpticalElement, OpticalElement]>(); public constructor(tandem: Tandem, stateOrLive: ElementStateRecord) { const live = stateOrLive[LIVE_ELEMENT_STATE_KEY] as OpticalElement | undefined; @@ -37,6 +40,7 @@ export default class OpticalElementPhetioObject extends PhetioObject { public override dispose(): void { this.opticalElement.dispose(); + this.opticalElementReplacedEmitter.dispose(); super.dispose(); } @@ -50,12 +54,14 @@ export default class OpticalElementPhetioObject extends PhetioObject { toStateObject: (obj) => ({ ...obj.opticalElement.serialize(), id: obj.opticalElement.id }), stateObjectToCreateElementArguments: (state: ElementStateRecord) => [state], applyState: (obj, state) => { - obj.opticalElement.dispose(); const el = deserializeElement(state); if (!el) { throw new Error("OpticalElementPhetioObject.applyState: invalid element state"); } + const previousElement = obj.opticalElement; obj.opticalElement = el; + obj.opticalElementReplacedEmitter.emit(previousElement, el); + previousElement.dispose(); }, }, ); diff --git a/src/common/model/optics/OpticsScene.ts b/src/common/model/optics/OpticsScene.ts index f614d32..6958fdc 100644 --- a/src/common/model/optics/OpticsScene.ts +++ b/src/common/model/optics/OpticsScene.ts @@ -91,6 +91,8 @@ export class OpticsScene extends PhetioObject { public readonly opticalElementsGroup: PhetioGroup]>; public readonly sceneChangedEmitter: Emitter; + /** Fires when PhET-iO state replaces an element object inside its stable group wrapper. */ + public readonly elementReplacedEmitter = new Emitter<[OpticalElement, OpticalElement]>(); /** Undo/redo history for add/remove element commands. */ public readonly history: CommandHistory = new CommandHistory(); @@ -216,11 +218,24 @@ export class OpticsScene extends PhetioObject { }, ); - this.opticalElementsGroup.elementCreatedEmitter.addListener(() => { + this.opticalElementsGroup.elementCreatedEmitter.addListener((wrapper) => { + this._elementById.set(wrapper.opticalElement.id, wrapper.opticalElement); + wrapper.opticalElementReplacedEmitter.addListener((previousElement, replacementElement) => { + if (this._elementById.get(previousElement.id) === previousElement) { + this._elementById.delete(previousElement.id); + } + this._elementById.set(replacementElement.id, replacementElement); + this.invalidate(); + this.sceneChangedEmitter.emit(); + this.elementReplacedEmitter.emit(previousElement, replacementElement); + }); this.invalidate(); this.sceneChangedEmitter.emit(); }); - this.opticalElementsGroup.elementDisposedEmitter.addListener(() => { + this.opticalElementsGroup.elementDisposedEmitter.addListener((wrapper) => { + if (this._elementById.get(wrapper.opticalElement.id) === wrapper.opticalElement) { + this._elementById.delete(wrapper.opticalElement.id); + } this.invalidate(); this.sceneChangedEmitter.emit(); }); @@ -239,6 +254,9 @@ export class OpticsScene extends PhetioObject { */ public addElement(element: OpticalElement, recordHistory = true): void { const doAdd = (): void => { + if (this._elementById.has(element.id)) { + throw new Error(`Cannot add duplicate optical element id "${element.id}"`); + } this.opticalElementsGroup.createNextElement({ ...element.serialize(), id: element.id, diff --git a/src/common/view/RayTracingCommonView.ts b/src/common/view/RayTracingCommonView.ts index 44a76d5..520a6e6 100644 --- a/src/common/view/RayTracingCommonView.ts +++ b/src/common/view/RayTracingCommonView.ts @@ -32,6 +32,7 @@ import { RayPropagationView } from "./RayPropagationView.js"; import { sceneHistoryRegistry } from "./SceneHistoryRegistry.js"; import { downloadSceneSVG } from "./SceneSVGExporter.js"; import { ToolsPanel } from "./ToolsPanel.js"; +import { trackRegistry } from "./TrackRegistry.js"; import { ViewOptionsModel } from "./ViewOptionsModel.js"; import { viewSnapState } from "./ViewSnapState.js"; @@ -247,11 +248,19 @@ export class RayTracingCommonView extends ScreenView { super(options); this.model = model; - sceneHistoryRegistry.setHistory(model.scene.history); const tandem = options?.tandem; const uiStrings = StringManager.getInstance().getUIStrings(); this.viewOptions = new ViewOptionsModel(tandem?.createTandem("viewOptions")); + this.visibleProperty.link((visible) => { + if (visible) { + sceneHistoryRegistry.setHistory(model.scene.history); + trackRegistry.setActiveScope(this.viewOptions.scopeId); + } else { + sceneHistoryRegistry.clearHistory(model.scene.history); + trackRegistry.clearActiveScope(this.viewOptions.scopeId); + } + }); // ── Model-View Transform ──────────────────────────────────────────────── // Maps model origin (0, 0) to the centre of the visible play area. @@ -509,6 +518,33 @@ export class RayTracingCommonView extends ScreenView { } }); + // PhET-iO can replace the model object inside an existing group wrapper. + // Rebuild the corresponding view so editing, rendering, and lookup all + // continue to reference the same replacement object. + model.scene.elementReplacedEmitter.addListener((previousElement, replacementElement) => { + if (this.selectedElementProperty.value === previousElement) { + this.selectedElementProperty.value = null; + } + const previousView = this.elementViewMap.get(previousElement.id); + if (previousView) { + this.elementViewMap.delete(previousElement.id); + previousView.dispose(); + } + const previousTandem = this.elementTandemMap.get(previousElement.id); + if (previousTandem) { + RayTracingCommonView._cleanupElementTandem(previousTandem); + this.elementTandemMap.delete(previousElement.id); + } + + const tn = replacementElement.id.replace(/-(\d+)$/, (_, n: string) => n); + const et = tandem?.createTandem(tn) ?? Tandem.OPTIONAL; + const replacementView = createOpticalElementView(replacementElement, modelViewTransform, et, this.viewOptions); + if (replacementView) { + this.elementTandemMap.set(replacementElement.id, et); + this._setupView(replacementElement, replacementView); + } + }); + // ── Tools Panel ─────────────────────────────────────────────────────────── // ToolsPanel owns the measuring tape, protractor, all toggle checkboxes, // the ray-density control, and the accordion box. It pins the accordion to @@ -627,6 +663,9 @@ export class RayTracingCommonView extends ScreenView { // clicking the trash icon), but only when no text input has focus. // Stored as a class field so it can be removed in dispose(). this._handleKeyDown = (event: KeyboardEvent): void => { + if (!this.isVisible()) { + return; + } const target = event.target as HTMLElement; const isTextInput = target.tagName === "INPUT" || target.tagName === "TEXTAREA" || target.isContentEditable; @@ -700,6 +739,8 @@ export class RayTracingCommonView extends ScreenView { public override dispose(): void { window.removeEventListener("keydown", this._handleKeyDown); + sceneHistoryRegistry.clearHistory(this.model.scene.history); + trackRegistry.clearActiveScope(this.viewOptions.scopeId); super.dispose(); this.viewOptions.dispose(); } diff --git a/src/common/view/SceneHistoryRegistry.ts b/src/common/view/SceneHistoryRegistry.ts index 1b8064c..0bd99cb 100644 --- a/src/common/view/SceneHistoryRegistry.ts +++ b/src/common/view/SceneHistoryRegistry.ts @@ -6,7 +6,8 @@ * threading the reference through every call site. * * Lifecycle: - * RayTracingCommonView calls setHistory(model.scene.history) during construction. + * A visible RayTracingCommonView registers its model.scene.history and + * conditionally clears it when that screen becomes inactive. * Drag and slider helpers call sceneHistoryRegistry.history to get the * CommandHistory instance (null-safe: no-op when history is not set). * @@ -19,11 +20,18 @@ import type { CommandHistory } from "../model/optics/CommandHistory.js"; class SceneHistoryRegistryImpl { private _history: CommandHistory | null = null; - /** Called once by RayTracingCommonView during construction. */ + /** Set the history for the currently visible screen. */ public setHistory(history: CommandHistory | null): void { this._history = history; } + /** Clear a screen's history only if it is still the active registration. */ + public clearHistory(history: CommandHistory): void { + if (this._history === history) { + this._history = null; + } + } + /** Returns the active CommandHistory, or null when not wired up. */ public get history(): CommandHistory | null { return this._history; diff --git a/src/common/view/TrackRegistry.ts b/src/common/view/TrackRegistry.ts index d0f632a..7a259d1 100644 --- a/src/common/view/TrackRegistry.ts +++ b/src/common/view/TrackRegistry.ts @@ -8,25 +8,39 @@ import type { Point } from "../model/optics/Geometry.js"; export interface TrackDescriptor { + scopeId: string; getP1: () => Point; getP2: () => Point; } class TrackRegistryImpl { private readonly tracks = new Map(); + private activeScopeId: string | null = null; - public register(id: string, getP1: () => Point, getP2: () => Point): void { - this.tracks.set(id, { getP1, getP2 }); + public register(id: string, scopeId: string, getP1: () => Point, getP2: () => Point): void { + this.tracks.set(id, { scopeId, getP1, getP2 }); } public unregister(id: string): void { this.tracks.delete(id); } + public setActiveScope(scopeId: string): void { + this.activeScopeId = scopeId; + } + + public clearActiveScope(scopeId: string): void { + if (this.activeScopeId === scopeId) { + this.activeScopeId = null; + } + } + public getAllTracks(): ReadonlyArray<{ id: string; p1: Point; p2: Point }> { const result: Array<{ id: string; p1: Point; p2: Point }> = []; for (const [id, desc] of this.tracks) { - result.push({ id, p1: desc.getP1(), p2: desc.getP2() }); + if (desc.scopeId === this.activeScopeId) { + result.push({ id, p1: desc.getP1(), p2: desc.getP2() }); + } } return result; } diff --git a/src/common/view/ViewOptionsModel.ts b/src/common/view/ViewOptionsModel.ts index c58bf3a..9afd2b6 100644 --- a/src/common/view/ViewOptionsModel.ts +++ b/src/common/view/ViewOptionsModel.ts @@ -22,7 +22,11 @@ import { RAY_STUB_LENGTH_MAX_PX, RAY_STUB_LENGTH_MIN_PX } from "../../OpticsLabC import OpticsLabNamespace from "../../OpticsLabNamespace.js"; import opticsLabQueryParameters from "../../preferences/opticsLabQueryParameters.js"; +let nextViewScopeId = 1; + export class ViewOptionsModel { + /** Identifies the owning screen for process-wide interaction registries. */ + public readonly scopeId = `view-scope-${nextViewScopeId++}`; public readonly handlesVisibleProperty: BooleanProperty; public readonly focalMarkersVisibleProperty: BooleanProperty; public readonly rayArrowsVisibleProperty: BooleanProperty; diff --git a/src/common/view/guides/TrackView.ts b/src/common/view/guides/TrackView.ts index ea4b307..2f00d97 100644 --- a/src/common/view/guides/TrackView.ts +++ b/src/common/view/guides/TrackView.ts @@ -112,6 +112,7 @@ export class TrackView extends BaseOpticalElementView { // Register with the track registry for snap logic. trackRegistry.register( track.id, + viewOptions?.scopeId ?? "unscoped", () => track.p1, () => track.p2, ); diff --git a/tests/memory-leak.test.ts b/tests/memory-leak.test.ts index d945199..8de46a5 100644 --- a/tests/memory-leak.test.ts +++ b/tests/memory-leak.test.ts @@ -609,6 +609,7 @@ describe("Memory leak regression", () => { const trackId = el.id; const view = createOpticalElementView(el, mvt, Tandem.OPT_OUT, viewOptions); + trackRegistry.setActiveScope(viewOptions.scopeId); // Immediately after construction the track must appear in the registry. const registeredBefore = trackRegistry.getAllTracks().some((t) => t.id === trackId); @@ -620,6 +621,7 @@ describe("Memory leak regression", () => { // After disposal the entry must be gone. const registeredAfter = trackRegistry.getAllTracks().some((t) => t.id === trackId); expect(registeredAfter).toBe(false); + trackRegistry.clearActiveScope(viewOptions.scopeId); }); // ── Multiple sequential scenes do not cross-retain each other's elements ── diff --git a/tests/review-regressions.test.ts b/tests/review-regressions.test.ts index 7cdc82b..0cf4a00 100644 --- a/tests/review-regressions.test.ts +++ b/tests/review-regressions.test.ts @@ -31,14 +31,19 @@ import { PlanoConvexLens } from "../src/common/model/glass/PlanoConvexLens.js"; import { SphericalLens } from "../src/common/model/glass/SphericalLens.js"; import { BeamSource } from "../src/common/model/light-sources/BeamSource.js"; import { DivergentBeam } from "../src/common/model/light-sources/DivergentBeam.js"; +import { PointSourceElement } from "../src/common/model/light-sources/PointSourceElement.js"; import { SingleRaySource } from "../src/common/model/light-sources/SingleRaySource.js"; import { AperturedParabolicMirror } from "../src/common/model/mirrors/AperturedParabolicMirror.js"; import { ArcMirror } from "../src/common/model/mirrors/ArcMirror.js"; +import { CommandHistory } from "../src/common/model/optics/CommandHistory.js"; import { deserializeElement } from "../src/common/model/optics/elementSerialization.js"; import { arcBounds, point } from "../src/common/model/optics/Geometry.js"; +import OpticalElementPhetioObject from "../src/common/model/optics/OpticalElementPhetioObject.js"; import { OpticsScene } from "../src/common/model/optics/OpticsScene.js"; import type { OpticalElement, SimulationRay } from "../src/common/model/optics/OpticsTypes.js"; import { RayTracer } from "../src/common/model/optics/RayTracer.js"; +import { sceneHistoryRegistry } from "../src/common/view/SceneHistoryRegistry.js"; +import { trackRegistry } from "../src/common/view/TrackRegistry.js"; // ── Helpers ────────────────────────────────────────────────────────────────── @@ -348,3 +353,107 @@ describe("tracer culls unphysical rays at the boundary", () => { expect(Number.isFinite(seg.p2.x) && Number.isFinite(seg.p2.y)).toBe(true); }); }); + +describe("multi-screen interaction registries", () => { + it("does not let an inactive screen clear the active screen history", () => { + const inactiveHistory = new CommandHistory(); + const activeHistory = new CommandHistory(); + sceneHistoryRegistry.setHistory(inactiveHistory); + sceneHistoryRegistry.setHistory(activeHistory); + + sceneHistoryRegistry.clearHistory(inactiveHistory); + expect(sceneHistoryRegistry.history).toBe(activeHistory); + + sceneHistoryRegistry.clearHistory(activeHistory); + expect(sceneHistoryRegistry.history).toBeNull(); + }); + + it("returns tracks only from the active screen scope", () => { + trackRegistry.register( + "track-a", + "scope-a", + () => point(0, 0), + () => point(1, 0), + ); + trackRegistry.register( + "track-b", + "scope-b", + () => point(0, 2), + () => point(1, 2), + ); + try { + trackRegistry.setActiveScope("scope-a"); + expect(trackRegistry.getAllTracks().map((track) => track.id)).toEqual(["track-a"]); + + trackRegistry.setActiveScope("scope-b"); + expect(trackRegistry.getAllTracks().map((track) => track.id)).toEqual(["track-b"]); + } finally { + trackRegistry.unregister("track-a"); + trackRegistry.unregister("track-b"); + trackRegistry.clearActiveScope("scope-b"); + } + }); +}); + +describe("deserialized element identity", () => { + it("advances generated IDs beyond a restored sparse ID", () => { + const restored = deserializeElement({ + type: "PointSource", + x: 0, + y: 0, + brightness: 1, + wavelength: 550, + id: "element-9000000", + }); + const next = new PointSourceElement(point(1, 1), 1, 550); + + expect(restored?.id).toBe("element-9000000"); + expect(Number(next.id.replace("element-", ""))).toBeGreaterThan(9_000_000); + }); + + it("rejects duplicate IDs instead of corrupting the scene lookup map", () => { + const scene = new OpticsScene(Tandem.OPT_OUT); + const first = new PointSourceElement(point(0, 0), 1, 550); + const second = new PointSourceElement(point(1, 1), 1, 550); + second.reassignIdForDeserialization(first.id); + scene.addElement(first, false); + + expect(() => scene.addElement(second, false)).toThrow(/duplicate optical element id/); + expect(scene.getElement(first.id)).toBe(first); + }); +}); + +describe("PhET-iO element replacement synchronization", () => { + it("updates scene lookup, invalidates tracing, and emits the replacement", () => { + const scene = new OpticsScene(Tandem.OPT_OUT); + const original = new PointSourceElement(point(0, 0), 1, 550); + scene.addElement(original, false); + const wrapper = scene.opticalElementsGroup.getArray()[0]; + expect(wrapper).toBeDefined(); + if (!wrapper) { + return; + } + + const cachedResult = scene.simulate(); + let emittedReplacement: OpticalElement | null = null; + scene.elementReplacedEmitter.addListener((_previous, replacement) => { + emittedReplacement = replacement; + }); + + OpticalElementPhetioObject.opticalElementInstanceIO.applyState(wrapper, { + type: "PointSource", + x: 3, + y: 4, + brightness: 0.75, + wavelength: 600, + id: original.id, + }); + + expect(wrapper.opticalElement).not.toBe(original); + expect(scene.getElement(original.id)).toBe(wrapper.opticalElement); + expect(emittedReplacement).toBe(wrapper.opticalElement); + const updatedResult = scene.simulate(); + expect(updatedResult).not.toBe(cachedResult); + expect(updatedResult.rays[0]?.origin).toEqual(point(3, 4)); + }); +});