Merge pull request #3 from OpenPhysics/claude/simulation-review-cleanup-93jozu

Clean up simulation: fix orphaned timeline feature, dedupe, polish a11y

Author: veillette

CLAUDE.md

@@ -5,8 +5,8 @@ Sim-specific context for AI assistants. General SceneryStack guidance: [OpenPhys
 ## Project
 
 A two-screen SceneryStack simulation porting the NAAP **Solar System Models** lab,
-scaffolded from `TemplateSingleSim`. **Scaffolding only** — both screens are a
-placeholder label + Reset All; no model/physics yet.
+scaffolded from `TemplateSingleSim`. Both screens now have complete models and
+fully wired views (not scaffolding).
 
 - **Ptolemaic System** (`src/ptolemaic/`) — port of the NAAP *Ptolemaic System Simulator* (`ptolemaic.swf`): the Earth-centered (geocentric) model with deferent + epicycle and the resulting view from Earth.
 - **Planetary Configurations** (`src/configurations/`) — port of the NAAP *Planetary Configurations Simulator* (`configurationsSimulator.swf`): the Sun-centered system and the configurations (opposition, conjunction, elongation) that Earth and another planet form.
@@ -26,10 +26,10 @@ Shared code keeps the `SolarSystemModels` prefix; per-screen code uses the
 | `src/i18n/StringManager.ts` | Singleton localized string accessor; per-screen name + a11y getters |
 | `src/main.ts` | Entry point; registers both screens with the Sim |
 | `src/ptolemaic/PtolemaicScreen.ts` | `Screen<PtolemaicModel, PtolemaicScreenView>` wrapper |
-| `src/ptolemaic/model/PtolemaicModel.ts` | Ptolemaic screen state (scaffold) |
+| `src/ptolemaic/model/PtolemaicModel.ts` | Ptolemaic screen state: deferent/epicycle geometry, presets, memory |
 | `src/ptolemaic/view/PtolemaicScreenView.ts` | Ptolemaic visuals, `screenSummaryContent` + `pdomOrder` |
 | `src/configurations/ConfigurationsScreen.ts` | `Screen<ConfigurationsModel, ConfigurationsScreenView>` wrapper |
-| `src/configurations/model/ConfigurationsModel.ts` | Configurations screen state (scaffold) |
+| `src/configurations/model/ConfigurationsModel.ts` | Configurations screen state: orbits, synodic events, timeline |
 | `src/configurations/view/ConfigurationsScreenView.ts` | Configurations visuals, `screenSummaryContent` + `pdomOrder` |
 | `src/preferences/solarSystemModelsQueryParameters.ts` | `QueryStringMachine` parameters |
 | `scripts/decompile-flash.ts` | Extract ActionScript from the NAAP Flash `.swf` sources via JPEXS FFDec (→ `NAAP/decompiled/`) |

src/SolarSystemModelsConstants.ts

@@ -12,6 +12,9 @@ export const ORBIT_VIEW_SCALE = 95; // px per model unit
 export const ORBIT_VIEW_CENTER_X = 295; // px — model origin maps here (x)
 export const ORBIT_VIEW_CENTER_Y = 300; // px — model origin maps here (y)
 export const ZODIAC_LABEL_RADIUS = 285; // px — zodiac sign label ring
+export const ZODIAC_LABEL_MAX_WIDTH = 55; // px — max width of a zodiac sign label
+export const ZODIAC_TICK_INNER_RADIUS = 250; // px — zodiac sign boundary tick, inner end
+export const ZODIAC_TICK_OUTER_RADIUS = 270; // px — zodiac sign boundary tick, outer end
 export const ZODIAC_STRIP_HEIGHT = 80; // px — "view from Earth" strip height
 export const ZODIAC_STRIP_WIDTH = 600; // px — width matching AS factor 600/2π
 
@@ -39,6 +42,8 @@ export const CONFIGURATIONS_ORBIT_CENTER_Y = 285; // px — Sun maps here (y)
 export const CONFIGURATIONS_ORBIT_MARGIN = 60; // px — margin around orbit area
 export const CONFIGURATIONS_TIMELINE_WIDTH = 210; // px
 export const CONFIGURATIONS_TIMELINE_HEIGHT = 350; // px
+export const CONFIGURATIONS_TIMELINE_CYCLE_HEIGHT = 120; // px — vertical px per synodic cycle
+export const CONFIGURATIONS_ELONGATION_ARC_RADIUS = 35; // px — elongation indicator arc radius
 
 // ── Configurations preset orbital radii (AU) ───────────────────────────────────
 
@@ -59,6 +64,9 @@ SolarSystemModelsNamespace.register("SolarSystemModelsConstants", {
   ORBIT_VIEW_CENTER_X,
   ORBIT_VIEW_CENTER_Y,
   ZODIAC_LABEL_RADIUS,
+  ZODIAC_LABEL_MAX_WIDTH,
+  ZODIAC_TICK_INNER_RADIUS,
+  ZODIAC_TICK_OUTER_RADIUS,
   ZODIAC_STRIP_HEIGHT,
   ZODIAC_STRIP_WIDTH,
   PTOLEMAIC_DEFERENT_RADIUS,
@@ -71,6 +79,8 @@ SolarSystemModelsNamespace.register("SolarSystemModelsConstants", {
   CONFIGURATIONS_ORBIT_MARGIN,
   CONFIGURATIONS_TIMELINE_WIDTH,
   CONFIGURATIONS_TIMELINE_HEIGHT,
+  CONFIGURATIONS_TIMELINE_CYCLE_HEIGHT,
+  CONFIGURATIONS_ELONGATION_ARC_RADIUS,
   EPICYCLE_SIZE_RANGE,
   ECCENTRICITY_RANGE,
   MOTION_RATE_RANGE,

src/common/CelestialBodyNode.ts

@@ -2,17 +2,16 @@ import type { TReadOnlyProperty } from "scenerystack/axon";
 import type { Vector2 } from "scenerystack/dot";
 import type { ModelViewTransform2 } from "scenerystack/phetcommon";
 import type { NodeOptions, TPaint } from "scenerystack/scenery";
-import { Circle, Node, Text } from "scenerystack/scenery";
+import { Circle, Node } from "scenerystack/scenery";
 
 export type CelestialBodyNodeOptions = {
   radius?: number;
   fill?: TPaint;
-  label?: string;
 } & NodeOptions;
 
 /**
- * A Circle + optional label Text, auto-positioned via a model Vector2 Property
- * and a ModelViewTransform2. Used for Earth, Sun, planets, and markers.
+ * A Circle, auto-positioned via a model Vector2 Property and a
+ * ModelViewTransform2. Used for Earth, Sun, planets, and markers.
  */
 export class CelestialBodyNode extends Node {
   public constructor(
@@ -22,26 +21,13 @@ export class CelestialBodyNode extends Node {
   ) {
     const radius = providedOptions?.radius ?? 8;
     const fill = providedOptions?.fill ?? "#ffffff";
-    const label = providedOptions?.label;
 
     // Extract CelestialBodyNode-specific keys, pass remaining to super
-    const { radius: _r, fill: _f, label: _l, ...nodeOptions } = providedOptions ?? {};
+    const { radius: _r, fill: _f, ...nodeOptions } = providedOptions ?? {};
 
     const body = new Circle(radius, { fill });
-    const children: Node[] = [body];
 
-    if (label !== undefined) {
-      children.push(
-        new Text(label, {
-          font: "12px sans-serif",
-          fill: "#ffffff",
-          centerX: 0,
-          top: radius + 3,
-        }),
-      );
-    }
-
-    super({ children, cursor: "default", ...nodeOptions });
+    super({ children: [body], cursor: "default", ...nodeOptions });
 
     positionProperty.link((pos) => {
       const viewPos = mvt.modelToViewPosition(pos);

src/common/ZodiacStripBackground.ts

@@ -0,0 +1,47 @@
+import type { TReadOnlyProperty } from "scenerystack/axon";
+import { Node, Rectangle, Text } from "scenerystack/scenery";
+import { PhetFont } from "scenerystack/scenery-phet";
+import SolarSystemModelsColors from "../SolarSystemModelsColors.js";
+
+/** Wrap x into [0, width) — shared by both zodiac strips' longitude→x mapping. */
+export function wrapToWidth(x: number, width: number): number {
+  return ((x % width) + width) % width;
+}
+
+/**
+ * Shared "view from Earth" zodiac strip chrome: background band, 12 evenly
+ * spaced sign labels, and dividers between them. Screen-specific overlays
+ * (constellation art, sun/planet markers, elongation labels, ...) are added
+ * by the caller on top of this Node.
+ */
+export class ZodiacStripBackground extends Node {
+  public constructor(width: number, height: number, signStringProperties: readonly TReadOnlyProperty<string>[]) {
+    super();
+
+    const band = new Rectangle(0, 0, width, height, {
+      fill: SolarSystemModelsColors.zodiacBandColorProperty,
+      stroke: SolarSystemModelsColors.zodiacBorderColorProperty,
+      lineWidth: 1,
+    });
+    this.addChild(band);
+
+    const segW = width / 12;
+    for (let i = 0; i < 12; i++) {
+      const label = new Text(signStringProperties[i]!, {
+        font: new PhetFont(9),
+        fill: SolarSystemModelsColors.zodiacLabelColorProperty,
+        maxWidth: segW - 4,
+      });
+      label.centerX = (i + 0.5) * segW;
+      label.centerY = height * 0.25;
+      this.addChild(label);
+
+      if (i > 0) {
+        const divider = new Rectangle(i * segW, 0, 1, height, {
+          fill: SolarSystemModelsColors.zodiacDividerColorProperty,
+        });
+        this.addChild(divider);
+      }
+    }
+  }
+}

src/configurations/view/ConfigurationsDisplayPanel.ts

@@ -1,4 +1,5 @@
 import { Text, VBox } from "scenerystack/scenery";
+import { PhetFont } from "scenerystack/scenery-phet";
 import { Checkbox } from "scenerystack/sun";
 import { SolarSystemModelsPanel } from "../../common/SolarSystemModelsPanel.js";
 import { StringManager } from "../../i18n/StringManager.js";
@@ -7,7 +8,7 @@ import { PANEL_WIDTH } from "../../SolarSystemModelsConstants.js";
 import type { ConfigurationsModel } from "../model/ConfigurationsModel.js";
 
 const LABEL_OPTS = {
-  font: "14px sans-serif",
+  font: new PhetFont(14),
   fill: SolarSystemModelsColors.textColorProperty,
   maxWidth: PANEL_WIDTH - 60,
 } as const;

src/configurations/view/ConfigurationsElongationIndicator.ts

@@ -5,10 +5,9 @@ import type { ModelViewTransform2 } from "scenerystack/phetcommon";
 import { Node, Path, Text } from "scenerystack/scenery";
 import { ArrowNode, PhetFont } from "scenerystack/scenery-phet";
 import SolarSystemModelsColors from "../../SolarSystemModelsColors.js";
+import { CONFIGURATIONS_ELONGATION_ARC_RADIUS } from "../../SolarSystemModelsConstants.js";
 import type { ConfigurationsModel } from "../model/ConfigurationsModel.js";
 
-const ARC_RADIUS_VIEW = 35; // px — arc radius in view space
-
 export class ConfigurationsElongationIndicator extends Node {
   public constructor(model: ConfigurationsModel, mvt: ModelViewTransform2) {
     super({ visibleProperty: model.showElongationAngleProperty });
@@ -74,13 +73,13 @@ export class ConfigurationsElongationIndicator extends Node {
           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, ARC_RADIUS_VIEW, startAngle, endAngle, anticlockwise);
+          arcShape.arc(vp1.x, vp1.y, CONFIGURATIONS_ELONGATION_ARC_RADIUS, startAngle, endAngle, anticlockwise);
         }
         arcPath.shape = arcShape;
 
         // Label at midpoint angle
         const midAngle = (sunDir + planetDir) / 2;
-        const labelR = ARC_RADIUS_VIEW + 14;
+        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);

src/configurations/view/ConfigurationsKeyboardHelpContent.ts

@@ -2,15 +2,26 @@
  * ConfigurationsKeyboardHelpContent.ts
  *
  * Content for the keyboard-help dialog (the "?" button in the navigation bar).
- * The template's only interactions are buttons and Reset All, so a single
- * basic-actions section covers the available keyboard controls. Add a slider or
- * combo-box section here as the simulation grows.
+ * Covers the screen's keyboard-accessible interactions: basic actions (Tab,
+ * Reset All), the observer/target planet combo boxes, and the NumberControl
+ * sliders (orbit radii, animation rate, pause time). Dragging the planets
+ * (and Shift-dragging to set epoch angle) and scrubbing/clicking the timeline
+ * are mouse/touch only — they have no keyboard equivalent, so they aren't
+ * documented here.
  */
 
-import { BasicActionsKeyboardHelpSection, TwoColumnKeyboardHelpContent } from "scenerystack/scenery-phet";
+import {
+  BasicActionsKeyboardHelpSection,
+  ComboBoxKeyboardHelpSection,
+  SliderControlsKeyboardHelpSection,
+  TwoColumnKeyboardHelpContent,
+} from "scenerystack/scenery-phet";
 
 export class ConfigurationsKeyboardHelpContent extends TwoColumnKeyboardHelpContent {
   public constructor() {
-    super([new BasicActionsKeyboardHelpSection()], []);
+    super(
+      [new BasicActionsKeyboardHelpSection(), new ComboBoxKeyboardHelpSection()],
+      [new SliderControlsKeyboardHelpSection()],
+    );
   }
 }

src/configurations/view/ConfigurationsScreenSummaryContent.ts

@@ -13,26 +13,60 @@
  *   - currentDetailsContent — a LIVE paragraph describing current state
  *   - interactionHintContent — a short hint on how to get started
  *
- * ── Making "current details" live ─────────────────────────────────────────────
- * The template has no model state, so currentDetails is a static string. In a
- * real sim, build a DerivedProperty over the relevant model Properties and pass
- * it as `currentDetailsContent` so the paragraph updates as the sim runs.
- * See LunarLander/src/.../LunarLanderScreenSummaryContent.ts for the pattern.
+ * currentDetailsContent is a DerivedProperty over the observer/target planet
+ * presets, the current time, and (if locked) the current configuration name,
+ * so the paragraph updates as the sim runs.
  */
+import { DerivedProperty } from "scenerystack/axon";
 import { ScreenSummaryContent } from "scenerystack/sim";
 import { StringManager } from "../../i18n/StringManager.js";
 import type { ConfigurationsModel } from "../model/ConfigurationsModel.js";
+import { PRESET_KEYS } from "../model/ConfigurationsPlanet.js";
 
 export class ConfigurationsScreenSummaryContent extends ScreenSummaryContent {
-  // `model` is unused in the template but kept in the signature so real sims can
-  // derive a live currentDetailsContent from it without changing call sites.
-  public constructor(_model: ConfigurationsModel) {
+  public constructor(model: ConfigurationsModel) {
     const a11y = StringManager.getInstance().getConfigurationsA11yStrings();
+    const strings = StringManager.getInstance().getConfigurationsStrings();
+
+    // Ordered to match PRESET_KEYS: mercury, venus, earth, mars, jupiter, saturn.
+    const planetLabelProperties = [
+      strings.mercuryStringProperty,
+      strings.venusStringProperty,
+      strings.earthStringProperty,
+      strings.marsStringProperty,
+      strings.jupiterStringProperty,
+      strings.saturnStringProperty,
+    ] as const;
+
+    const currentDetailsProperty = new DerivedProperty(
+      [
+        model.preset1IndexProperty,
+        model.preset2IndexProperty,
+        model.timeProperty,
+        model.currentConfigurationProperty,
+        a11y.currentDetailsTemplateStringProperty,
+        a11y.currentConfigurationTemplateStringProperty,
+        ...planetLabelProperties,
+      ] as const,
+      (preset1Index, preset2Index, time, currentConfiguration, template, configTemplate, ...planetLabels) => {
+        const earthIndex = PRESET_KEYS.indexOf("earth");
+        const observerLabel = planetLabels[preset1Index] ?? planetLabels[earthIndex];
+        const targetLabel = planetLabels[preset2Index] ?? planetLabels[earthIndex];
+        const configPart = currentConfiguration === "" ? "" : configTemplate.replace("{0}", currentConfiguration);
+
+        return template
+          .replace("{0}", observerLabel ?? "")
+          .replace("{1}", targetLabel ?? "")
+          .replace("{2}", time.toFixed(2))
+          .replace("{3}", configPart)
+          .trim();
+      },
+    );
 
     super({
       playAreaContent: a11y.screenSummary.playAreaStringProperty,
       controlAreaContent: a11y.screenSummary.controlAreaStringProperty,
-      currentDetailsContent: a11y.currentDetailsStringProperty,
+      currentDetailsContent: currentDetailsProperty,
       interactionHintContent: a11y.screenSummary.interactionHintStringProperty,
     });
   }

src/configurations/view/ConfigurationsScreenView.ts

@@ -121,6 +121,7 @@ export class ConfigurationsScreenView extends ScreenView {
       tagName: "div",
       focusable: true,
       accessibleName: a11y.controls.observerDragStringProperty,
+      accessibleHelpText: a11y.controls.observerShiftDragStringProperty,
     });
     this.addChild(observerNode);
 
@@ -132,6 +133,7 @@ export class ConfigurationsScreenView extends ScreenView {
       tagName: "div",
       focusable: true,
       accessibleName: a11y.controls.targetDragStringProperty,
+      accessibleHelpText: a11y.controls.targetShiftDragStringProperty,
     });
     this.addChild(targetNode);
 
@@ -190,9 +192,8 @@ export class ConfigurationsScreenView extends ScreenView {
       updateSunPos();
     };
 
-    Multilink.multilink(
-      [model.semimajorAxis1Property, model.semimajorAxis2Property, s.auStringProperty] as const,
-      () => updateOrbits(),
+    Multilink.multilink([model.semimajorAxis1Property, model.semimajorAxis2Property, s.auStringProperty] as const, () =>
+      updateOrbits(),
     );
 
     // ── Zodiac strip at bottom ──────────────────────────────────────────────

src/configurations/view/ConfigurationsTimeReadout.ts

@@ -43,7 +43,9 @@ export class ConfigurationsTimeReadout extends SolarSystemModelsPanel {
         s.secondsStringProperty,
       ] as const,
       (remaining, pausedFor, second, seconds) => {
-        if (remaining <= 0) return "";
+        if (remaining <= 0) {
+          return "";
+        }
         const secs = Math.ceil(remaining);
         const unit = secs === 1 ? second : seconds;
         return pausedFor.replace("{0}", String(secs)).replace("{1}", unit);