From 0dd59d1046b0d6c4cd37ac4482d38f732b87f3a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 1 Jul 2026 10:55:46 +0000 Subject: [PATCH] Fix planet-marker desync on orbit rescale and preset-collision UI bug - Configurations screen: rebuilding the ModelViewTransform2 on axis change replaced the object outright, so the planet markers and elongation indicator (which captured the old transform in a closure at construction) silently kept rendering at the previous scale while the orbit circles resized correctly. Now mutate the transform's matrix in place and explicitly resync the dependent nodes. - ConfigurationsModel.applyPreset now reports success/failure instead of always committing the preset index; the control panel reverts a preset combo box when both planets would land on the same orbit (previously the combo box showed the newly picked planet while the semimajor axis silently stayed unchanged). - ConfigurationsTimeline: guard the drag handler against a zero/negative synodic period the same way the render loop already does. - ConfigurationsTimeReadout: bind Text directly to the model Property instead of a redundant manual link. - Removed two non-null assertions in the zodiac strip label loops by iterating with forEach instead of indexing. Verified in-browser: reverting the fix and dragging the target planet's semimajor-axis slider reproduces the marker staying frozen at the old orbit distance; with the fix, both planets track their resized orbits correctly, including while playing. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018qTbZeFneh9h3njJEhTUSQ --- src/common/ZodiacStripBackground.ts | 6 +- .../model/ConfigurationsModel.ts | 16 ++- .../view/ConfigurationsControlPanel.ts | 15 +-- .../view/ConfigurationsElongationIndicator.ts | 115 +++++++++++------- .../view/ConfigurationsScreenView.ts | 15 ++- .../view/ConfigurationsTimeReadout.ts | 6 +- .../view/ConfigurationsTimeline.ts | 3 + src/ptolemaic/view/PtolemaicScreenView.ts | 6 +- 8 files changed, 111 insertions(+), 71 deletions(-) diff --git a/src/common/ZodiacStripBackground.ts b/src/common/ZodiacStripBackground.ts index 0304348..1777bee 100644 --- a/src/common/ZodiacStripBackground.ts +++ b/src/common/ZodiacStripBackground.ts @@ -26,8 +26,8 @@ export class ZodiacStripBackground extends Node { this.addChild(band); const segW = width / 12; - for (let i = 0; i < 12; i++) { - const label = new Text(signStringProperties[i]!, { + signStringProperties.forEach((signStringProperty, i) => { + const label = new Text(signStringProperty, { font: new PhetFont(9), fill: SolarSystemModelsColors.zodiacLabelColorProperty, maxWidth: segW - 4, @@ -42,6 +42,6 @@ export class ZodiacStripBackground extends Node { }); this.addChild(divider); } - } + }); } } diff --git a/src/configurations/model/ConfigurationsModel.ts b/src/configurations/model/ConfigurationsModel.ts index 555d345..fa6ea0b 100644 --- a/src/configurations/model/ConfigurationsModel.ts +++ b/src/configurations/model/ConfigurationsModel.ts @@ -569,16 +569,24 @@ export class ConfigurationsModel implements TModel { } } - public applyPreset(id: 1 | 2, key: PlanetPresetKey): void { + /** + * Applies a planet preset's semimajor axis. Returns false (and leaves the + * preset index Properties untouched) when the preset would put both + * planets on the same orbit — that configuration has no synodic period, + * so the caller should revert the requesting UI element instead. + */ + public applyPreset(id: 1 | 2, key: PlanetPresetKey): boolean { const a = PLANET_PRESETS[key]; - this.setSemimajorAxis(id, a, false); + if (!this.setSemimajorAxis(id, a, false)) { + return false; + } + const idx = PRESET_KEYS.indexOf(key); if (id === 1) { - const idx = PRESET_KEYS.indexOf(key); this.preset1IndexProperty.value = idx >= 0 ? idx : 0; } else { - const idx = PRESET_KEYS.indexOf(key); this.preset2IndexProperty.value = idx >= 0 ? idx : 0; } + return true; } public reset(): void { diff --git a/src/configurations/view/ConfigurationsControlPanel.ts b/src/configurations/view/ConfigurationsControlPanel.ts index 444f142..e39da73 100644 --- a/src/configurations/view/ConfigurationsControlPanel.ts +++ b/src/configurations/view/ConfigurationsControlPanel.ts @@ -59,17 +59,18 @@ export class ConfigurationsControlPanel extends SolarSystemModelsPanel { accessibleName: a11y.controls.targetPlanetStringProperty, }); - // When preset index changes, apply preset - model.preset1IndexProperty.lazyLink((idx) => { + // When preset index changes, apply preset. Both planets landing on the same + // orbit has no synodic period, so revert the combo box if that's rejected. + model.preset1IndexProperty.lazyLink((idx, oldIdx) => { const key = PRESET_KEYS[idx]; - if (key !== undefined) { - model.applyPreset(1, key); + if (key !== undefined && !model.applyPreset(1, key)) { + model.preset1IndexProperty.value = oldIdx; } }); - model.preset2IndexProperty.lazyLink((idx) => { + model.preset2IndexProperty.lazyLink((idx, oldIdx) => { const key = PRESET_KEYS[idx]; - if (key !== undefined) { - model.applyPreset(2, key); + if (key !== undefined && !model.applyPreset(2, key)) { + model.preset2IndexProperty.value = oldIdx; } }); diff --git a/src/configurations/view/ConfigurationsElongationIndicator.ts b/src/configurations/view/ConfigurationsElongationIndicator.ts index b128d6a..e0e1797 100644 --- a/src/configurations/view/ConfigurationsElongationIndicator.ts +++ b/src/configurations/view/ConfigurationsElongationIndicator.ts @@ -9,81 +9,104 @@ import { CONFIGURATIONS_ELONGATION_ARC_RADIUS } from "../../SolarSystemModelsCon import type { ConfigurationsModel } from "../model/ConfigurationsModel.js"; export class ConfigurationsElongationIndicator extends Node { + private mvt: ModelViewTransform2; + private readonly model: ConfigurationsModel; + private readonly sunArrow: ArrowNode; + private readonly planetArrow: ArrowNode; + private readonly arcPath: Path; + private readonly elongLabel: Text; + public constructor(model: ConfigurationsModel, mvt: ModelViewTransform2) { super({ visibleProperty: model.showElongationAngleProperty }); - const sunArrow = new ArrowNode(0, 0, 1, 0, { + this.model = model; + this.mvt = mvt; + + this.sunArrow = new ArrowNode(0, 0, 1, 0, { stroke: null, fill: SolarSystemModelsColors.elongationColorProperty, headWidth: 8, headHeight: 6, tailWidth: 2, }); - const planetArrow = new ArrowNode(0, 0, 1, 0, { + this.planetArrow = new ArrowNode(0, 0, 1, 0, { stroke: null, fill: SolarSystemModelsColors.elongationColorProperty, headWidth: 8, headHeight: 6, tailWidth: 2, }); - const arcPath = new Path(null, { + this.arcPath = new Path(null, { stroke: SolarSystemModelsColors.elongationColorProperty, lineWidth: 1.5, }); - const elongLabel = new Text("", { + this.elongLabel = new Text("", { font: new PhetFont(11), fill: SolarSystemModelsColors.elongationColorProperty, }); - this.addChild(arcPath); - this.addChild(sunArrow); - this.addChild(planetArrow); - this.addChild(elongLabel); + this.addChild(this.arcPath); + this.addChild(this.sunArrow); + this.addChild(this.planetArrow); + this.addChild(this.elongLabel); Multilink.multilink( [model.pos1Property, model.pos2Property, model.elongationDegProperty, model.elongationLabelProperty] as const, - (p1, p2, elongDeg, elongLabel_) => { - // Convert model positions to view - const vp1 = mvt.modelToViewPosition(p1); - const vp2 = mvt.modelToViewPosition(p2); - const vSun = mvt.modelToViewPosition(Vector2.ZERO); + (p1, p2, elongDeg, elongLabel_) => this.update(p1, p2, elongDeg, elongLabel_), + ); + } - // Direction from p1 toward Sun (view) - const sunDir = Math.atan2(vSun.y - vp1.y, vSun.x - vp1.x); - // Direction from p1 toward p2 (view) - const planetDir = Math.atan2(vp2.y - vp1.y, vp2.x - vp1.x); + /** Called by the screen view when the orbit scale changes and a new transform is built. */ + public setModelViewTransform(mvt: ModelViewTransform2): void { + this.mvt = mvt; + this.update( + this.model.pos1Property.value, + this.model.pos2Property.value, + this.model.elongationDegProperty.value, + this.model.elongationLabelProperty.value, + ); + } - // Arrow endpoints — extend to edge of diagram - const arrowLen = 180; - sunArrow.setTailAndTip(vp1.x, vp1.y, vp1.x + arrowLen * Math.cos(sunDir), vp1.y + arrowLen * Math.sin(sunDir)); - planetArrow.setTailAndTip( - vp1.x, - vp1.y, - vp1.x + arrowLen * Math.cos(planetDir), - vp1.y + arrowLen * Math.sin(planetDir), - ); + private update(p1: Vector2, p2: Vector2, elongDeg: number, elongLabel_: string): void { + // Convert model positions to view + const vp1 = this.mvt.modelToViewPosition(p1); + const vp2 = this.mvt.modelToViewPosition(p2); + const vSun = this.mvt.modelToViewPosition(Vector2.ZERO); - // Arc — draw from sunDir to planetDir in the elongation direction - const arcShape = new Shape(); - if (Math.abs(elongDeg) > 0.5) { - // AS: if elongationValue < 0 (East): drawArc from -sunDir to -planetDir - // In view coords (y-down), angles are negated vs model - // We draw CW or CCW depending on sign - const startAngle = sunDir; - const endAngle = planetDir; - // Determine sweep direction: elongDeg < 0 (East) means target is east of Sun - const anticlockwise = elongDeg > 0; // W = clockwise sweep, E = anticlockwise - arcShape.arc(vp1.x, vp1.y, CONFIGURATIONS_ELONGATION_ARC_RADIUS, startAngle, endAngle, anticlockwise); - } - arcPath.shape = arcShape; + // Direction from p1 toward Sun (view) + const sunDir = Math.atan2(vSun.y - vp1.y, vSun.x - vp1.x); + // Direction from p1 toward p2 (view) + const planetDir = Math.atan2(vp2.y - vp1.y, vp2.x - vp1.x); - // Label at midpoint angle - const midAngle = (sunDir + planetDir) / 2; - const labelR = CONFIGURATIONS_ELONGATION_ARC_RADIUS + 14; - elongLabel.string = `${Math.abs(elongDeg).toFixed(1)}° ${elongLabel_}`; - elongLabel.centerX = vp1.x + labelR * Math.cos(midAngle); - elongLabel.centerY = vp1.y + labelR * Math.sin(midAngle); - }, + // Arrow endpoints — extend to edge of diagram + const arrowLen = 180; + this.sunArrow.setTailAndTip(vp1.x, vp1.y, vp1.x + arrowLen * Math.cos(sunDir), vp1.y + arrowLen * Math.sin(sunDir)); + this.planetArrow.setTailAndTip( + vp1.x, + vp1.y, + vp1.x + arrowLen * Math.cos(planetDir), + vp1.y + arrowLen * Math.sin(planetDir), ); + + // Arc — draw from sunDir to planetDir in the elongation direction + const arcShape = new Shape(); + if (Math.abs(elongDeg) > 0.5) { + // AS: if elongationValue < 0 (East): drawArc from -sunDir to -planetDir + // In view coords (y-down), angles are negated vs model + // We draw CW or CCW depending on sign + const startAngle = sunDir; + const endAngle = planetDir; + // Determine sweep direction: elongDeg < 0 (East) means target is east of Sun + const anticlockwise = elongDeg > 0; // W = clockwise sweep, E = anticlockwise + arcShape.arc(vp1.x, vp1.y, CONFIGURATIONS_ELONGATION_ARC_RADIUS, startAngle, endAngle, anticlockwise); + } + this.arcPath.shape = arcShape; + + // Label at midpoint angle + const midAngle = (sunDir + planetDir) / 2; + const labelR = CONFIGURATIONS_ELONGATION_ARC_RADIUS + 14; + this.elongLabel.string = `${Math.abs(elongDeg).toFixed(1)}° ${elongLabel_}`; + this.elongLabel.centerX = vp1.x + labelR * Math.cos(midAngle); + this.elongLabel.centerY = vp1.y + labelR * Math.sin(midAngle); } } diff --git a/src/configurations/view/ConfigurationsScreenView.ts b/src/configurations/view/ConfigurationsScreenView.ts index 1c00c74..0e0177d 100644 --- a/src/configurations/view/ConfigurationsScreenView.ts +++ b/src/configurations/view/ConfigurationsScreenView.ts @@ -166,10 +166,19 @@ export class ConfigurationsScreenView extends ScreenView { const updateOrbits = () => { const a1 = model.semimajorAxis1Property.value; const a2 = model.semimajorAxis2Property.value; - this.mvt = buildMvt(a1, a2); + // Mutate the existing mvt's matrix in place (rather than replacing the + // object) so CelestialBodyNode's internal closure — captured once at + // construction — keeps reading the current transform on every future + // position update, not just the one right after this call. + this.mvt.setMatrix(buildMvt(a1, a2).matrix); + + // pos1Property/pos2Property may have already reacted to the axis change + // (and pushed stale-scale positions) before this listener ran, so + // explicitly re-sync the planet nodes and elongation indicator now. + observerNode.translation = this.mvt.modelToViewPosition(model.pos1Property.value); + targetNode.translation = this.mvt.modelToViewPosition(model.pos2Property.value); + elongationIndicator.setModelViewTransform(this.mvt); - // Update elongation indicator's MVT reference - // (It holds a closure over mvt, so we need to re-link — simplest: rebuild shape via update) const center = this.mvt.modelToViewPosition(Vector2.ZERO); const r1 = this.mvt.modelToViewDeltaX(a1); diff --git a/src/configurations/view/ConfigurationsTimeReadout.ts b/src/configurations/view/ConfigurationsTimeReadout.ts index 7a165e9..715178a 100644 --- a/src/configurations/view/ConfigurationsTimeReadout.ts +++ b/src/configurations/view/ConfigurationsTimeReadout.ts @@ -54,13 +54,9 @@ export class ConfigurationsTimeReadout extends SolarSystemModelsPanel { const timeText = new Text(timeStringProperty, FONT_OPTS); const synodicText = new Text(synodicStringProperty, FONT_OPTS); - const configText = new Text("", FONT_OPTS); + const configText = new Text(model.currentConfigurationProperty, FONT_OPTS); const countdownText = new Text(countdownStringProperty, FONT_OPTS); - model.currentConfigurationProperty.link((cfg) => { - configText.string = cfg; - }); - const content = new VBox({ children: [timeText, synodicText, configText, countdownText], spacing: 4, diff --git a/src/configurations/view/ConfigurationsTimeline.ts b/src/configurations/view/ConfigurationsTimeline.ts index 2c6a050..f5d3f29 100644 --- a/src/configurations/view/ConfigurationsTimeline.ts +++ b/src/configurations/view/ConfigurationsTimeline.ts @@ -185,6 +185,9 @@ export class ConfigurationsTimeline extends Node { }, drag: (_event, listener) => { const synodic = model.synodicPeriodProperty.value; + if (synodic <= 0) { + return; + } const scale = CYCLE_HEIGHT_PX / synodic; const deltaY = initY - listener.parentPoint.y; const newTime = initTime + deltaY / scale; diff --git a/src/ptolemaic/view/PtolemaicScreenView.ts b/src/ptolemaic/view/PtolemaicScreenView.ts index 15278b3..b6e8210 100644 --- a/src/ptolemaic/view/PtolemaicScreenView.ts +++ b/src/ptolemaic/view/PtolemaicScreenView.ts @@ -103,12 +103,12 @@ export class PtolemaicScreenView extends ScreenView { } // ── Zodiac sign labels at sign centers (+15°) ───────────────────────── - for (let i = 0; i < 12; i++) { + ZODIAC_SIGN_PROPS.forEach((signStringProperty, i) => { const angle = ((i + 0.5) * Math.PI) / 6; // 15°, 45°, 75°, ... (sign centers) const vx = ORBIT_VIEW_CENTER_X + Math.cos(angle) * ZODIAC_LABEL_RADIUS; // Inverted Y: positive y in model = up in view, so negate for angle const vy = ORBIT_VIEW_CENTER_Y - Math.sin(angle) * ZODIAC_LABEL_RADIUS; - const label = new Text(ZODIAC_SIGN_PROPS[i]!, { + const label = new Text(signStringProperty, { font: new PhetFont(10), fill: SolarSystemModelsColors.zodiacLabelColorProperty, maxWidth: ZODIAC_LABEL_MAX_WIDTH, @@ -116,7 +116,7 @@ export class PtolemaicScreenView extends ScreenView { label.centerX = vx; label.centerY = vy; this.addChild(label); - } + }); // ── Path trail (behind orbit circles) ───────────────────────────────── this.pathTrail = new PtolemaicPathTrail(model, mvt);