Quantum Wave Interference

// REVIEW: I would remove comments like this (its already out of date) // See

export function getDisplaySlitLayout(
  slitSeparation: number, slitSeparationDisplayMin: number, slitSeparationDisplayMax: number,
  regionHeight: number

Why the fallback to 0.5 when separation is 0? This seems odd and I wonder if it should be affirm.

const separationFraction = separationRange > 0 ? ( slitSeparation - slitSeparationDisplayMin ) / separationRange : 0.5;
const displaySeparationPixels = linear(
  0,

This class has many responsibilities and is nearly 900 lines. Factor into smaller sub-models or helpers? Did you consider abstractions for source type, slits, waves, screens, and so on?

export default abstract class BaseSceneModel extends PhetioObject {

Can you please add documentation to many of these physical attributes? Most importantly, units? I believe some are meters and others are milimeters.

public readonly sourceType: SourceType;
private readonly particleMass: number;
public readonly slitWidth: number;

Should this collection be an observable array? How important is it that hits.length === totalHitsProperty.value? If critical, should that be abstracted/enforced? Do you want to make hits private and use totalHitsProperty every hits.length is used?

public readonly hits: Vector2[];
public readonly totalHitsProperty: NumberProperty;
public readonly hitsChangedEmitter: TEmitter;

Would this be better as a DerivedProperty or MappedProperty?

this.barrierTypeProperty = new StringUnionProperty<BarrierType>( 'doubleSlit', {
  validValues: BarrierTypeValues
} );

This is out of place, could it be moved alongside the other instance fields and documented there?

private wavefrontReached = false;

This does quite a bit, can you document? I see it does more than add - it stores the event updates counts, prunes history, and syncs the soler?

protected addDecoherenceEvent( event: DecoherenceEvent ): void {
  this.decoherenceEvents.push( event );
  if ( event.clickedDetectorSlit === 'topSlit' ) {

I was referred here by another comment. Can you explain what is happening in this method and how this bi-directional link works? Or maybe docs for setupSlitConfigurationListeners should move here?

private linkSlitConfigurationToBarrierType(
  slitConfigurationProperty: StringUnionProperty<SlitConfigurationWithNoBarrier>
): void {

this.syncSolverParameters(); // REVIEW: Why is this here twice? Remove or document order requirement.

this.syncSolverParameters(); // REVIEW: Why is this here twice? Remove or document order requirement.

If identical, could the shared behavior be factored into a helper? If not, the reasoning would be helpful to indicate in the note.

public deleteSnapshot( snapshot: Snapshot ): void {
  this.snapshotsProperty.value = renumberSnapshots( this.snapshotsProperty.value.filter( s => s !== snapshot ) );
}

Both subclasses override this, should it be abstract?

protected getSubclassState(): object {
  return {};
}

If subclasses should call super at the start (as documented), this will be false before reset is complete. Is that OK?

  this.isResetting = false;
  this.hitsChangedEmitter.emit();
}

In that case, should this move into the view code? Why frame count instead of time interval? Why 10 frames?

const ACCESSIBLE_STATE_STEP_INTERVAL = 10;

Should scenes be a ReadonlyArray?

public readonly scenes: T[];
public readonly sceneProperty: Property<T>;

There are 8 cases of const activeIndex = model.scenes.indexOf( model.sceneProperty.value ); in this sim, and each duplicates the bounds assertion. Want to factor that out or create a Property for this?

public readonly scenes: T[];
public readonly sceneProperty: Property<T>;

This is a cool pattern! Consider adding documentation describing why it works and how it is used. Consider adding documentation pointing the reader to scene classes to find more detailed documentation about each of these Properties and values.

public readonly currentBarrierTypeProperty: DynamicProperty<BarrierType, BarrierType, BaseSceneModel>;
public readonly currentIsEmittingProperty: DynamicProperty<boolean, boolean, BaseSceneModel>;
public readonly currentIsMaxHitsReachedProperty: DynamicProperty<boolean, boolean, BaseSceneModel>;

Can you please comment on the coordinate frame and units for these positions? The default values look like view coordinates, even though they are stored in the model.

public readonly measuringTapeBasePositionProperty: Vector2Property;
public readonly measuringTapeTipPositionProperty: Vector2Property;
public readonly stopwatch: Stopwatch;

This is an unusual pattern can you describe what this does, how it is consumed, and so on? Why is this in the model? It seems like more of a view concern? Maybe documentation for ACCESSIBLE_STATE_STEP_INTERVAL is appropriate here?

public readonly accessibleStateStepProperty: NumberProperty;
private accessibleStateStepFrameCount = 0;

This is only used by BaseScreenModel. Should it be private, and can its type be specified explicitly?

protected readonly toolsTandem;
private readonly normalTimeSpeedFactor: number;
private readonly fastTimeSpeedFactor: number;

This is only used by BaseScreenModel and can be private.

protected readonly toolsTandem;
private readonly normalTimeSpeedFactor: number;
private readonly fastTimeSpeedFactor: number;

I could not find anything that writes to currentBarrierTypeProperty, and its users type it as read-only. Does this need to be bidirectional?

  bidirectional: true
} );

It looks like snapshots are modified through takeSnapshot() and deleteSnapshot(), and users treat this as read-only. Does this need to be bidirectional?

  bidirectional: true
} );

This seems like an implementation detail, should it really be instrumented?

    tandem: tandem.createTandem( 'accessibleStateStepProperty' )
  } );
}

Both subclasses widen this to public. Should it be public here?

protected abstract takeSnapshot(): void;

Can you please document reset(), particularly if there are any order dependencies (like the subclass hook running first, or why sceneProperty is last)?.

public reset(): void {
  this.scenes.forEach( scene => scene.reset() );

Can you please document reset(), particularly if there are any order dependencies (like the scene

public reset(): void {
  this.scenes.forEach( scene => scene.reset() );

This says the shared tool Properties have already been reset, but reset() calls this BEFORE resetting them. Which ordering is intended?

protected abstract resetToolVisibility(): void;

Why is this only stepped while currentIsEmittingProperty.value?

if ( this.currentIsEmittingProperty.value ) {
  this.stepAccessibleState();
}

wavelength, waveSpeed, and slitWidth are included in WaveSolverParameters and passed by BaseSceneModel, but I do not see them used here or in either subclass. Should they be removed?

this.setIfDefined( params.displaySpeedScale, value => { this.displaySpeedScale = value; } );
this.setIfDefined( params.displayWavelengths, value => { this.displayWavelengths = value; } );
this.setIfDefined( params.barrierType, value => { this.barrierType = value; } );

reset() preserves most parameter state, is that correct? Can the documentation clarify?

public reset(): void {
  this.time = 0;
  this.isSourceOn = false;

I could not find any callers for this, is it still needed?

public invalidate(): void {
  this.dirty = true;
}

getGridCellY() samples cell centers, but this usually samples the left edge of each horizontal cell. Is that asymmetry intentional? Why?

return this.barrierType === 'doubleSlit' && gridX === barrierIx ?
       this.barrierFractionX * this.regionWidth :
       gridX * this.regionWidth / this.gridWidth;

Why are these DynamicProperties factored into helpers when most are created inline?

type SceneWithDetectionMode = {
  detectionModeProperty: TProperty<DetectionMode>;
};

Why is this featured but read-only and stateless while each scene Property is writable and stateful?

phetioReadOnly: true,
phetioState: false,
phetioValueType: StringUnionIO( DetectionModeValues ),

This is factored out into WaveMath. Can this use the shared helper?

function smoothStep( edge0: number, edge1: number, value: number ): number {
  const t = clamp( ( value - edge0 ) / ( edge1 - edge0 ), 0, 1 );
  return t * t * ( 3 - 2 * t );

Snapshot is documented as immutable, but its fields and arrays are mutable. Should the type enforce that?

export type Snapshot = CoreRecord<typeof SNAPSHOT_SCHEMA>;

Re "no rendering assumptions", It looks like this DOES include layering/alpha information now, is that correct? Would you like to change WaveKernal or can you clarify doc?

import { applyDecoherenceEvent, applyPlaneWaveDecoherenceEventLayers } from './WaveDecoherence.js';
import { type FieldSample, type LayeredFieldSample, type WaveParameters } from './WaveKernelTypes.js';
import { applyGaussianPacketMeasurementProjectionLayers, applyMeasurementProjections } from './WaveMeasurementProjection.js';

The CRC has an item about undefined valued keys being omitted during serialization. Does anything need to be done for this field?

  clickedDetectorSlit?: DecoherenceSlit;
};

JSDoc indicates edge0 < edge1 is important, do you want to add affirms for that?

if ( x <= edge0 ) {
  return 0;
}

The only usage selects a Gaussian packet, but this silently accepts plane waves. Should this affirm instead of silently falling back?

if ( source.kind !== 'gaussianPacket' ) {
  return {
    kind: 'field',

This is only used by plot views. Should it live in common/view?

export type WaveDisplayModePolarity = 'unipolar' | 'bipolar';

Does this need to be exported/factored out? I can't find any usages beyond this file.

export function getDisplayModePolarity( mode: WaveDisplayMode ): WaveDisplayModePolarity {
  return mode === 'amplitude' ? 'unipolar' : 'bipolar';
}

This is implemented by one base class, is this interface necessary?

import type Complex from '../../../../dot/js/Complex.js';
import Vector2 from '../../../../dot/js/Vector2.js';
import { type BarrierType } from './BarrierType.js';

The one implementation of this interface implements both methods. Could they be required to simplify the requirements described in the documentation?

usesLayeredFieldSamples?(): boolean;

It seems like this is a view concern, is the model the right place for it?

type WaveVisualizableScene = {

I am not sure about this pattern. I believe its trying to decouple view from the full model, but I don't think that is achieved and it adds more complexity. Fields are duplicated and you have to manually extend the types (see MeasurementSceneLike). IMO it would be better for view components to take the BaseSceneModel (or smaller submodules from it if BaseSceneModel is broken up).

type WaveVisualizableScene = {

I usually see credits in the main file, and that is the only usage. Should they move there?

const CREDITS: CreditsData = {
  leadDesign: 'Matthew Blackman',
  softwareDevelopment: 'Sam Reid, Matthew Blackman',

The photon range is expressed five different ways in this file. Could it (and other ranges) just be a static Range?

const PHOTON_WAVELENGTH_RANGE_NM = [ 400, 700 ] as const;

Three of these are [ 0, 0 ] placeholders. Why? I *think* it is meant ot indicate "no" range, but wondered if there is a better way to declare that.

const WAVELENGTH_RANGE_NM_BY_SOURCE_TYPE: Readonly<Record<SourceType, readonly [ number, number ]>> = {
  photons: PHOTON_WAVELENGTH_RANGE_NM,
  electrons: [ 0, 0 ],

THis constant is used like this in five places, each has a 0 guard: QuantumWaveInterferenceConstants.PLANCK_CONSTANT / ( particleMass * snapshot.effectiveWavelength ) Could a shared helper live here too?

public static readonly PLANCK_CONSTANT = 6.626e-34; // J·s
public static readonly SPEED_OF_LIGHT = 3e8; // m/s

Units? Seconds of display time, I assume.

public static readonly WAVE_PACKET_TRAVERSAL_TIME = 1.5;

Are the original values still worth mentioning? And if both must always change together, why two constants?

public static readonly WAVE_PACKET_SIGMA_X_FRACTION = 0.15;
public static readonly WAVE_PACKET_SIGMA_Y_FRACTION = 0.15;

Four files rebuild new Range( 0, this ), and two use this * 0.5 as the initial value. Want a shared range and default here, like you have for wavelength?

public static readonly SCREEN_BRIGHTNESS_MAX = 100;

Four files recreate new Range( 0, this ), and two use this * 0.5 as the initial value. Want a shared range

public static readonly SCREEN_BRIGHTNESS_MAX = 100;

Nothing shuts off the source with this, it is only the default and ceiling for the maxHits query parameter, which also bounds the unrelated maxDetections. Decide which should be the source of truth.

public static readonly MAX_HITS = 25000;

This is assumed in accesisibility descriptions, either add as a dependency or note that it cannot change.

public static readonly MAX_SNAPSHOTS = 4;

Can you describe these? They sum to 1, is that important? Should one be derived from the other?

public static readonly DETECTOR_SCREEN_VISIBLE_FRACTION = 0.67;
public static readonly DETECTOR_SCREEN_OVERLAP_FRACTION = 0.33;

SourceControlPanelConstants.ts exists, should this go there?

public static readonly SOURCE_CONTROL_PANEL_TOP = 178;

This returns the same Range as createWavelengthRangeNM( 'photons' ), do you need both?

public static createPhotonWavelengthControlRangeNM(): Range {
  return new Range(
    QuantumWaveInterferenceConstants.PHOTON_WAVELENGTH_CONTROL_MIN_NM,

Only one function is exported, so should this be getApparentDetectorIntensity.ts like getDisplaySlitLayout.ts and renderDetectorScreenTexture.ts?

import { clamp } from '../../../../dot/js/util/clamp.js';
import { type DetectorPatternOptions, getExactDetectorIntensity, getLocalDoubleSlitFringeSpacing, getSingleSlitEnvelopeIntensity } from '../model/DetectorPattern.js';
import { showsDoubleSlitInterferencePattern } from '../model/SlitConfiguration.js';

Should this also be in WaveMath next to SmoothStep? Why does this use smootherStep when others use smoothStep?

function smootherStep( t: number ): number {
  const clampedT = clamp( t, 0, 1 );

Only regionWidth is used, so could this take a TReadOnlyProperty<number> instead? Related to the overall pattern comment I had in WaveVisualizableScene.

type BarrierScreenDistanceIndicatorScene = {
  readonly regionWidth: number;
};

Could this be a DerivedStringProperty so Text owns its own locale updates, instead of linking the unit Properties by hand below?

const distanceText = new Text( '', {
  font: SPAN_LABEL_FONT
} );

Replace with Multilink.multilink?

Array.from( new Set( [
  sceneProperty,
  barrierTypeProperty,

This comment does not explain why the options are copied. The two are nearly identical, including the tandem. Could ScreenControlsPanel use BrightnessControl instead? Also see https://github.com/phetsims/quantum-wave-interference/issues/375 for other cases of this.

const brightnessSlider = new HSlider( screenBrightnessProperty, brightnessRange, {
  trackSize: new Dimension2( 130, 3 ),
  thumbSize: new Dimension2( 13, 22 ),

Note that SourceControlPanelConstants also duplicates this track width, height, and font. See https://github.com/phetsims/quantum-wave-interference/issues/375

const brightnessSlider = new HSlider( screenBrightnessProperty, brightnessRange, {
  trackSize: new Dimension2( 130, 3 ),
  thumbSize: new Dimension2( 13, 22 ),

Should this be isDisposable: false, like most other Nodes in common/view? Nothing disposes it. Also see https://github.com/phetsims/quantum-wave-interference/issues/383

super( {
  spacing: 2,
  stretch: true,

This creates one Node, forwards ten arguments, and adds a child. I think it might be better for callers to create SlitConfigurationControlsRow and add it themselves.

export default function createAndAddSlitConfigurationControlsRow<TScene extends BaseSceneModel>(
  slitConfigurationProperty: PhetioProperty<SlitConfigurationWithNoBarrier>,
  sceneProperty: TReadOnlyProperty<TScene>,

I recommend deleting this file, but if you keep it, listParent is used for combo box parent AND the row parent. That works for the ScreenView, but you might want a second arg or clearly document.

listParent.addChild( bottomRow );

This should be a ProfileColorProperty in QuantumWaveInterferenceColors.

const MATTER_WAVE_FLASH_COLOR = new Color( 200, 200, 200 );

IMO, Better to just take the model in this case - or a narrowed type if you like that pattern, We could remove this file entirely by adding the detector hit counts and flash emitters to WaveRegionModel, then setting up options in WaveRegionNode the way it already does for the covered slits. THen you could get rid of additionalDoubleSlitOptions too. I think that would be cleaner.

export default function createFrontFacingSlitDetectorOptions(
  slitConfigurationProperty: TReadOnlyProperty<SlitConfigurationWithNoBarrier>,
  leftDetectorCountProperty: TReadOnlyProperty<number>,

thinking about the model/view separation here - this is the only caller of createSlitDetectorProperties. Should htese Properties be owned by the model? If its a view concept, should createSlitDetectorProperties be moved into the view?

const slitDetectorProperties = createSlitDetectorProperties( slitConfigurationProperty );

Why not just TReadOnlyProperty<string>?

type SlitConfigStringProperty = typeof QuantumWaveInterferenceFluent.bothOpenStringProperty;
type SlitConfigAccessibleName = NonNullable<ComboBoxItem<SlitConfigurationWithNoBarrier>[ 'accessibleName' ]>;

We don't care about PhET-iO API compatibility, right? Remove this complexity.

export type SlitConfigComboItemTandems = {
  topCoveredTandemName: 'topCoveredItem' | 'topClosedItem';
  bottomCoveredTandemName: 'bottomCoveredItem' | 'bottomClosedItem';

The items are threaded through two layers to reach SlitConfigurationControlsRow, which already takes the tandem. Could it build them itself?

export default function createSlitConfigComboItems(
  tandems: SlitConfigComboItemTandems,
  options?: SlitConfigComboItemOptions

SourceControlPanel is the only usage, and it just re-lists its own options here as eight positional arguments. Move into SourceControlPanel and delete this file?

export default function createSourceControlPanelContent<T extends SourceControlScene>(
  sceneProperty: Property<T>,
  scenes: T[],

Both screens pass the same four, in the same order. Could DetectorScreenControls just create them? Its doc says each screen needs its own list, but that is no longer true.

export default function createStandardToolCheckboxes<T extends BaseSceneModel>(
  model: BaseScreenModel<T>,
  tandem: Tandem

I would recommend documenting units here, they are hard to follow and the doc above is incorrect, most are in meters I believe

export type TheoreticalPatternScene = {
  getEffectiveWavelength(): number;
  slitWidth: number;

This is a general maintainability concern with description, and I do not have a great solution. These thresholds describe visual pattern formation, so they depend on dot size, detector rendering, histogram bin count, and the maximum number of hits. Changing any of those could make the descriptions inaccurate or make later stages unreachable.

const HIT_STAGE_THRESHOLDS: readonly HitStageThreshold[] = [
  { minimumHits: 1, hitStage: 'few' },
  { minimumHits: 188, hitStage: 'emerging' },

Document intent for T? I had to look at usages to understand. See

  items: CanonicalExperimentDetailsListItems<T>
): T[] {
  return [

I don't think this method should use sceneRadioButtonGroup strings. Either duplicate the strings for this context, or rename and move the strings in the YAML if context is truly identical across all languages.

const withScalePattern = QuantumWaveInterferenceFluent.a11y.sceneRadioButtonGroup.accessibleContextResponseWithWaveAreaScale;
const withoutScalePattern = QuantumWaveInterferenceFluent.a11y.sceneRadioButtonGroup.accessibleContextResponse;

I noticed this is hard-coded to a dummy Property at every usage. Is that a bug or can this file be simplified?

  isRulerVisibleProperty: TReadOnlyProperty<boolean>
) {

I don't think anyone provides their own hasWavefrontReachedScreen, can this be simplified or removed entirely?

function hasPopulatedIntensityScreen( scene: DetectorScreenDescriberScene ): boolean {
  return scene.hasWavefrontReachedScreen ? scene.hasWavefrontReachedScreen() : scene.isEmittingProperty.value;
}

This is only ever true. Remove?

  useSharedDetectorPatternDescription: boolean
) {

This is a lot of arguments, and many have defaults. Why should this support defaults or options? It might be clearer with optionize.

export function formatDetectorPatternDescription(
  isEmitting: boolean,
  detectionMode: DetectionMode,

The rest of the control flow is difficult for a human to understand, I would recommend writing it out more explicitly.

const patternKind = getPatternKind( slitConfiguration );

This can return an empty string. That won't work well for the pattern where this is used. I think it is OK because in that case the entire string contents should be hidden. But its not obvious from the implementation. Consider documenting/ asserting/or changing so this is clear?

return sourceType === 'photons' ? '' :
       kilogramsUnit.getAccessibleString(
         QuantumWaveInterferenceConstants.getParticleMass( sourceType ),

This can return an empty string. That won't work well for the pattern where this is used. I think it is OK because in that case the entire string contents should be hidden. But its not obvious from the implementation. Consider documenting/ asserting/or changing so this is clear?

return scene.sourceType === 'photons' ? '' :
       picometersUnit.getAccessibleString(
         roundSymmetric( metersToPicometers( scene.getEffectiveWavelength() ) ), {

The speed and slit-separation unit-selection/formatting are now duplicated with ParticleVelocityControl and SlitSeparationNumberControl.

const particleSpeedStringProperty = DerivedProperty.deriveAny(
  Array.from( new Set( [
    model.sceneProperty,

This is coupled to BandSpacingCategory in that adding a category there cchanges interfaces in this file and both arrays below need to be manually kept in sync. Consider moving this into BandAnalysis or creating a shared owner.

export type WavePeakSpacingCategory = BandSpacingCategory;

hasReachedSlits seems unused. Remove from return type?

  hasReachedSlits: hasReachedSlits,
  hasPassedSlits: hasPassedSlits
};

This assumes that getGeometryProperty returns the same Property as the last time this was called? I assume that is working today but seems potentially confusing.

getGeometryProperty( previousScene ).unlink( fullUpdate );
previousScene.slitSeparationProperty.unlink( fullUpdate );
previousScene.wavelengthProperty.unlink( fullUpdate );

this is unused now, remove?

  currentDetailsContent?: SectionContent;
};

Unused, remove this and implementation logic?

includeExperimentSetupDetails?: boolean;

I am confused by snapshotHitsParagraph pattern, what is it doing?

return hitStage === 'none' ?
       liveDescription :
       QuantumWaveInterferenceFluent.a11y.detectorScreen.snapshotHitsParagraph.format( {

Since this lives for the lifetime of the sim but does not fully dispose its listeners and Properties, should it be marked isDisposable: false?

super();

Why is detectorScreenNode an argument and handled here? It seems like DetectorPatternGraphLayerNode does not need a detectorScreenNode, and detectorScreenNode could manage its visibility.

isVisibleProperty.link( isVisible => {
  this.visible = isVisible;
  this.graphNode.visible = isVisible;

Isn't this just detectionModeProperty?.value ?? null? I could not find a case where they differ. Between this and the two maps above there are four fields tracking zoom state. reset() has to null-check three of them. Could that be simplified?

private activeDetectionMode: DetectionMode | null;

Why as never?

detectionModeProperty: undefined as never,
initialZoomLevel: 'default',
initialZoomLevels: {},

This limit is duplicated in accessible strings as "6 zoom levels", Consider making this a dependency for those patterns, or note here that it is dangerous to change.

export const MAX_DETECTOR_PATTERN_GRAPH_ZOOM_LEVEL = 6;

formatFrontFacingScreenDistance is its own file. Should this one be too?

function formatFrontFacingSlitSeparation( slitSepMM: number, snapshot: Snapshot ): DualString {
  if ( snapshot.sourceType === 'photons' ) {
    const slitSeparationUM = millimetersToMicrometers( slitSepMM );

MAX_HITS is 25000, so past 10000 the screen quietly drops its oldest dots while the graph histogram keeps binning all of them. Is that difference intended? Related to review comment for MAX_HITS in QuantumWaveInterferenceConstants.

const MAX_RENDERED_HITS = 10000;

Delete this and simplify rects?

const CORNER_RADIUS = 0;

Should this and arrow fill use ProfileColorProperties, like slitCoverFillProperty?

const BARRIER_FILL = '#939393';
const ARROW_WIDTH = 40;
const WAVE_REGION_FILL_INSET = 0.5;

Nine arguments seems like a lot, and indicates a different sort of abstraction might be beneficial. If you don't want to break up the model, I would just take the WaveRegionModel.

public constructor(
  sceneProperty: TReadOnlyProperty<{
    readonly regionWidth: number;

Where is the default yellow? I can't find yellow here or in defaults. Is that stale?

import { type SourceType } from '../model/SourceType.js';

These should likely be in QuantumWaveInterferenceColors, with particleBeamColorProperty.

const PARTICLE_EMITTER_PALETTES: Record<Exclude<SourceType, 'photons'>, ParticleEmitterPalette> = {
  electrons: {
    topColor: 'rgb(100, 120, 180)',

All users of this have to check for null. Like palette ? palette.x : undefined or hasGlass: palette !== null. Factor that here, or give photons an entry too so callers can just use it?

export default function getParticleEmitterPalette( sourceType: SourceType ): ParticleEmitterPalette | null {
  return sourceType === 'photons' ? null : PARTICLE_EMITTER_PALETTES[ sourceType ];
}

I don't care for this pattern. It is a lot of boilerplate and does not successfully decouple from the full model. I would delete this type and just accept the base class. Same as my comment in WaveVisualizableScene. That said, the groupings below outline potential abstractions the model is missing, and those look worth having: ToolVisibilityProperties for the four is*VisibleProperties. A barrier/slit class for things lik currentBarrierTypeProperty, currentSlitSeparationProperty and currentSlitConfigurationProperty. These are identical in HighIntensityModel and SingleParticlesModel, so this could help with duplication. A measuring tape class owning the base and tip position Properties.

type MeasurementToolsModel = {

Could it be done with only one visibleProperty? And I think this could be buggy with phet-io, since they can be set independently,

model.isStopwatchVisibleProperty.link( isVisible => {
  model.stopwatch.isVisibleProperty.value = isVisible;
} );

Why 20, and why is the tape inset when the stopwatch above is bounded by the full visibleBounds?

visibleBoundsProperty.link( visibleBounds => measuringTapeNode.setDragBounds( visibleBounds.eroded( 20 ) ) );
model.isMeasuringTapeVisibleProperty.linkAttribute( measuringTapeNode, 'visible' );

All this units code deserves its own file, like getMeasuringTapeUnits,

type StopwatchTimeUnit = {

This is duplicated with matterWavelengthStringProperty in ExperimentSetupDetailsNode. The only difference is getAccessibleString instead of getVisualSymbolPatternString. Factor out and share in another location.

const deBroglieWavelengthValueStringProperty = DerivedProperty.deriveAny( [
    sceneProperty,
    currentParticleSpeedProperty,

// REVIEW: I suspect this should be called VelocitySpeedControl. I see speed in all the Properties and // ranges from the model. "speed" is in teh tandem name.

import NumberProperty from '../../../../axon/js/NumberProperty.js';
import Dimension2 from '../../../../dot/js/Dimension2.js';
import Range from '../../../../dot/js/Range.js';

Factor out formatting options.

visualString: kilometersPerSecondUnit.getVisualSymbolPatternString( roundedValue, {
  decimalPlaces: 0,
  showTrailingZeros: false,

Maybe use this instead? That would match getParticleSpeedDelta and be more explicit. const useKilometersPerSecond = sourceType === 'electrons';

const useKilometersPerSecond = particleSpeedRange.max >= 10000;
const options = optionize<ParticleVelocityControlOptions, SelfOptions, ParentOptions>()( {
  responseOptions: null

These seem like the same, doyou need both?

const POSITION_PROBE_FRAME_THICKNESS = POSITION_PROBE_SIDE_FRAME_WIDTH;
const POSITION_PROBE_SIDE_FRAME_OVERLAP = 1;

I could not find anything that writes to this, it is always 1. Why is it a Property? TimePlotNode has the same one.

this.maxDisplayValueProperty = new TinyProperty( 1 );

This file is larger than I expected it to be. We just created SnapshotButtonGroup in scenery-phet based on this file. - snapshotCaptured sound file was moved to scenery-phet, can it be removed from the sim? - Do you really need to override the sound behavior for Dialog and these buttons? - Can options for colors, touch areas, accessible context response behavior, and so on be removed or moved into scenery-phet? - This sim has custom accessibility strings for this component so I don't think those can be.

import type Property from '../../../../axon/js/Property.js';
import type { TReadOnlyProperty } from '../../../../axon/js/TReadOnlyProperty.js';
import SnapshotButtonGroup from '../../../../scenery-phet/js/buttons/SnapshotButtonGroup.js';

Do you think this should be a feature of Dialog? Alternatively, is it best to suppress the closing sound? Why not let both play?

let shouldResumeOnDialogClose = false;

Do you think this should be a feature of Dialog? Alternatively, it may be best to remove this feature. Why not let both play?

let shouldResumeOnDialogClose = false;

// REVIEW: This seems very much like sun/ToggleNode + unselectedChildrenSceneGraphStrategy: 'excluded'. // I think the missing part is the stable bounds. Do you think we should add that as a feature of ToggleNode? //

import type { TReadOnlyProperty } from '../../../../axon/js/TReadOnlyProperty.js';
import Bounds2 from '../../../../dot/js/Bounds2.js';
import affirm from '../../../../perennial-alias/js/browser-and-node/affirm.js';

ScreenBrightnessUtils exports an RGB type. Can it be used here and in the six inline { r: number; g: number; b: number } parameters below?

export type DetectorScreenHitSpriteParams = {
  r: number;
  g: number;

This renderer never reads any of these, and the two callers populate different subsets - getDetectorScreenTexture sets effectiveWavelength but not rampFactor, DetectorScreenTextureRenderer does the opposite (and never calls this file at all). Should the type and its compare function live somewhere else?

export type DetectorScreenTextureRenderParameters = {
  brightness: number;
  wavelength: number;

Why string instead of DetectionMode?

detectionMode: string;
intensity: number;
isEmitting: boolean;

Should this use optionize? Right now defaults are applied with || at each use site, so a legitimate 0 silently becomes the default (maxRenderedHits: 0 draws every hit).

export type DetectorScreenRenderOptions = {

This and createDetectorScreenHitSpriteParams take the same six positional arguments, and every call site passes them in the same order. Would it be simpler to build the params object once and compare objects?

export function detectorScreenHitSpriteParamsMatch(
  params: DetectorScreenHitSpriteParams | null,
  rgb: { r: number; g: number; b: number },

This glow-radius expression is repeated in paintHits with displayGain, want to factor it out? Also, HITS_SCREEN_BRIGHTNESS_MAX_MULTIPLIER is 1.8, so the sqrt tops out around 1.34 and Math.min( 2, ... ) never binds in either place. Is the cap doing anything?

const maxGlowRadius = BASE_HIT_GLOW_RADIUS * hitRadiusScale *
                      Math.min( 2, Math.sqrt( Math.max( 1, HITS_SCREEN_BRIGHTNESS_MAX_MULTIPLIER ) ) );

DetectorScreenTextureRenderer has its own copy of this incremental-hit logic, plus its own getHitSprite, fillCircle, and rgbToRGBA. Can the High Intensity screen render through this function too? The file header says the point of living here is to keep two implementations from drifting apart.

function paintHits(
  context: CanvasRenderingContext2D,
  renderState: DetectorScreenRenderState,

This fires when hitCount === maxRenderedHits, where startIndex is 0 and nothing has been dropped yet. Should it be startIndex > 0? And since the scene models break out of their hit loop at maxHits, and getDetectorScreenTexture passes that same maxHits here, I don't think hits are ever actually dropped - so the "Hit counter continues to accumulate" log is misleading.

if ( renderCount >= maxRenderedHits ) {
  options.onRenderCapReached && options.onRenderCapReached();
}

Two questions here. A snapshot captured while the source is off will render blank in Intensity mode, is that intended? And effectiveWavelength === 0 is being used as a "not ready yet" sentinel, but DetectorScreenRenderState types it as a plain required number with no mention of that. Can you document it?

if ( !renderState.isEmitting || renderState.effectiveWavelength === 0 ) {
  return;
}

Why not just Vector2?

export type Vertex = { x: number; y: number };

This assumes 4 scenes, but scenes can have variable length.

const GRID_COLUMNS = 2;
const GRID_HORIZONTAL_SPACING = 12;
const GRID_VERTICAL_SPACING = 12;

You use AlignBox for this like you do above,

const iconSlotNode = new Rectangle( 0, 0, ICON_SLOT_WIDTH, ICON_SLOT_HEIGHT, {
  fill: null,
  stroke: null,

I don't think we should reach into children for this, it makes assumptions about RectangularRadioButtonGroup implementation. GridBox has autoColumns for exactly this...but RectangularRadioButtonGroup uses FlowBox. I am seeing preferredWidth being used for this in other sims and think we should improve RectangularRadioButtonGroup.

this.preferredWidth = actualChildWidth * GRID_COLUMNS + GRID_HORIZONTAL_SPACING * ( GRID_COLUMNS - 1 );

this.children[ index ] assumes sun creates one child per item in order, and labels is a parallel array populated by side effect in the map above. Can getButtonForValue and the item's own label replace both?

scenes.forEach( ( scene, index ) => {
  const radioButton = this.getButtonForValue( scene );
  const layoutNode = this.children[ index ];

This is a general problem and should be improved in RectangularRadioButtonGroup

labels[ index ].visibleProperty = radioButton.visibleProperty;

Why do you need this? If important, it should be improved in RectangularRadioButtonGroup.

const pointerBounds = layoutNode.localBounds.dilatedXY(
  GRID_HORIZONTAL_SPACING / 2,
  GRID_VERTICAL_SPACING / 2

I can see why you need this for Interactive Description, but don't love the pattern. Order is not guaranteed (fails listenerOrder=random). I am wondering about having accessibleContextResponse take an array of alertables. Or perhaps its best to manually combine "follow up" strings in the one value for accessibleContextResponse. If we keep this, consider moving addFireCompleteListener to ButtonNode.

radioButton.addFireCompleteListener( () => {
  const followUp = sceneRadioButtonGroupOptions.createAccessibleContextResponseFollowUp( scene );
  followUp !== null && this.addAccessibleContextResponse( followUp );

I would remove "used by" comments like this. This one is no longer correct, the users are

import { clamp } from '../../../../dot/js/util/clamp.js';
import { linear } from '../../../../dot/js/util/linear.js';
import VisibleColor from '../../../../scenery-phet/js/VisibleColor.js';

Most of these exports are not used outside of this file, make private.

export const PERCEPTUAL_VISIBILITY_THRESHOLD = 0.004;

A maintainer won't understand this comment. Remove or clarify. (Is this from the java)?

const COMPUTED_INTENSITY_BRIGHTNESS_MAX_FRACTION = 0.25;

This takes 0-1 while getIntensityDisplayGain takes 0-100. Would be better if the file could use one convention for brightness. I noticed all users of getHighIntensityIntensityDisplayGain manually normalize brightness, so it should probably do that too.

export function getHighIntensityIntensityDisplayGain( brightness: number, intensity: number ): number {
  const clampedBrightness = clamp( brightness, 0, 1 );
  return linear(

This indicates a bad API in sun. Is it trying to use type CreateTextFunction, should we just export that (or something more specific)?

type CreateContextResponseAlert =
  NonNullable<NonNullable<NumberControlOptions[ 'sliderOptions' ]>[ 'createContextResponseAlert' ]>;

PickRequired would be more idiomatic,

readonly startCallback: NonNullable<NumberControlOptions[ 'startCallback' ]>;
readonly endCallback: NonNullable<NumberControlOptions[ 'endCallback' ]>;
readonly createContextResponseAlert: CreateContextResponseAlert;

Many arguments, some are optional/nullable. Should this use options?

public constructor(
  slitConfigurationProperty: PhetioProperty<T>,
  slitConfigItems: ComboBoxItem<T>[],

There is already an affirm for this case.

const activeSlitSeparationControl = slitSeparationNodes[ activeSceneIndex >= 0 ? activeSceneIndex : 0 ];
const slitConfigTitleTopOffset = slitConfigTitle.boundsTo( slitConfigSection ).top;

Replace with Multilink.multilink?

slitConfigSection.localBoundsProperty.link( updateLayout );
slitSeparationContainer.localBoundsProperty.link( updateLayout );
slitSeparationNodes.forEach( node => node.localBoundsProperty.link( updateLayout ) );

Is this supposed to be isSlitDetectorOnProperty? If not, add JSDoc to constructor.

isSlitDetectorProperty: TReadOnlyProperty<boolean>,
detectorCountProperty: TReadOnlyProperty<number>,
detectorFlashColorProperty: TReadOnlyProperty<Color>,

Why is this optional, it looks like all users provide one,

  detectorFlashEmitter: TEmitter | null = null
) {
  super( {

Why do we need both this flag and DETECTOR_FLASH_HOLD_SECONDS?

if ( this.detectorFlashStartedSinceLastStep ) {
  this.detectorFlashStartedSinceLastStep = false;
  return;

Why is this here and why is it false initially?

  this.detectorWirePath.visible = true;
}

I am surprised that NumberDisplay allows this. Do you really want to support infinite values? NumberDisplay uses the range for layout purposes, how does this work?

const DETECTOR_COUNT_DISPLAY_RANGE = new Range( 0, Number.POSITIVE_INFINITY );
const DETECTOR_COUNT_DISPLAY_X_MARGIN = 2;
const DETECTOR_COUNT_DISPLAY_Y_MARGIN = 2;

Why have defaults here? All users provide their own values. But SlitDetectorNode imports DEFAULT_DETECTOR_INDICATOR_FLASH_DURATION just to hand it right back. Why?

public constructor(
  detectorCountProperty: TReadOnlyProperty<number>,
  detectorIndicatorFlashEmitter: TEmitter | null = null,

This number appears 6 times in ranges in this sim, could it be factored out or are all contexts different enough?

const NANOMETER_RANGE_THRESHOLD_MM = 0.0001;
const MICROMETER_RANGE_THRESHOLD_MM = 0.1;

This if/else duplicates quite a bit of code (formatter, dependencies, text, width, ticks) Could factor out unit descriptor and then build the configuration once.

if ( usesNanometers ) {

This dimension is copied 6 times, fator out?

thumbSize: new Dimension2( 13, 22 ),
majorTickLength: SLIDER_MAJOR_TICK_LENGTH,
tickLabelSpacing: SLIDER_TICK_LABEL_SPACING,

QuantumWaveInterferenceConstants.MAX_HITS is 25000, so this never happens. Is this stale, or can snapshots contain more hits than the scene model allows? If possible, one defined max would be best.

const MAX_RENDERED_SNAPSHOT_HITS = 100000;

Is this really the best way to signify "experiment" snapshots? What if that changes?

private isExperimentSnapshot( snapshot: Snapshot ): boolean {
  return this.getZoomedScreenHalfWidth !== null && snapshot.intensityDistribution.length === 0;
}

Some duplication between this string and the accessible value (speed, max threshold)

const particleMass = QuantumWaveInterferenceConstants.getParticleMass( snapshot.sourceType );
const speed = snapshot.effectiveWavelength === 0 ? 0 :
              QuantumWaveInterferenceConstants.PLANCK_CONSTANT / ( particleMass * snapshot.effectiveWavelength );

This is unconventional for our project and I would recommend keeping keeping a reference to the instance of the SnapshotMetadataProperties and accessing instance variables. const metadata = new SnapshotMetadataProperties( snapshotProperty, options )

const {
  headingProperty,
  isSlitGeometryRelevantProperty,

It seems strange that getDescription would be optional here. What if it depends on things that are not dependencies of this DerivedProperty? Perhaps users should provide their own descriptionProperty instead.

const getDescription = options.getDescription;

Is this just NodeOptions?

const superOptions: Record<string, unknown> = {
  isDisposable: false,
  children: nodeChildren,

Move this part or keep references so you don't have to depend on the order of nodeChildren?

const descriptionNode = nodeChildren[ 2 ];
const metadataListNode = nodeChildren[ 3 ];
this.pdomOrder = [ descriptionNode, metadataListNode, trashButton ];

Why would the overlay need to be in the PDOM? I don't see anywhere that includeInPDOM is true, consider removing.

public addSnapshotOverlayChild( child: Node, includeInPDOM = false ): void {
  this.detectorSnapshotSlot.addChild( child );
  if ( includeInPDOM && this.pdomOrder && this.pdomOrder.length > 0 ) {

Most of this is duplicated with SnapshotNodeOptions and SnapshotMetadataPropertiesOptions, consider an abstraction for snapshot representation.

slitOrientation?: 'leftRight' | 'topBottom';

Is it true that these three must be provided to support zoom? If so, combine into a single structure or affirm and document.

detectorScreenScaleIndexProperty?: NumberProperty;

This is only used once and the actual exported value has unexplained 130 * 1.15. Simplify or document.

const DEFAULT_SLIDER_TRACK_WIDTH = 130;

Remove this and only export one. Backward compatibility should not be a concern for us.

export type SourceParameterControlResponseOptions = SliderInteractionResponseOptions;

Why are we re-exporting this?

export type { SliderInteractionResponseOptions };

For this pattern, we need to provide percentUnit.getDependentProperties into option descriptionDependencies. We might improve this someday in https://github.com/phetsims/sun/issues/929

createAriaValueText: value => percentUnit.getAccessibleString( value * 100, {
  decimalPlaces: 0,
  showTrailingZeros: false,

This constructor has seven positional arguments, including two coordinate numbers and three different Properties, so the single call site is difficult to read without repeatedly consulting this signature. Could the wave-region geometry be a Bounds2 and the remaining configuration use named options?

public constructor(
  sceneProperty: TReadOnlyProperty<WaveVisualizableScene>,
  activeDisplayModeProperty: TReadOnlyProperty<WaveDisplayMode>,

This constructor has seven arguments, so the single call site is difficult to read without repeatedly reading this file. Maybe the wave-region geometry be a Bounds2. Or just take the MeasurementToolsModel. Or named config.

public constructor(
  sceneProperty: TReadOnlyProperty<WaveVisualizableScene>,
  activeDisplayModeProperty: TReadOnlyProperty<WaveDisplayMode>,

This is always 1 and nothing writes to it. Why is it a Property? This is the same issue noted in PositionPlotNode.

this.maxDisplayValueProperty = new TinyProperty( 1 );

probePositionProperty is writable through PhET-iO, but only the input listeners enforce waveRegionBounds. An external value can draw the probe outside the region while getProbeModelPosition() silently samples a clamped point. Should the Property enforce the bounds too?

this.probeNode = this.createCrosshairProbe();

This seems like a good candidate for one SoundRichDragListener that can do both forms of input.

probe.addInputListener( new SoundDragListener( {
  positionProperty: this.probePositionProperty,
  dragBoundsProperty: dragBoundsProperty,

Why does it do that? All of this is for item maxWidth? Ideally simplify or provide a value by inspection, but if it must stay document why.

const COMBO_BOX_ITEM_HEIGHT = new Text( 'test', { font: COMBO_BOX_FONT } ).height;
const COMBO_BOX_ARROW_AREA_WIDTH = COMBO_BOX_ITEM_HEIGHT + 2 * COMBO_BOX_Y_MARGIN;
const COMBO_BOX_CHROME_WIDTH = 2 * COMBO_BOX_X_MARGIN + COMBO_BOX_LINE_WIDTH + COMBO_BOX_ARROW_AREA_WIDTH;

Why widened to string instead of SourceType?

sceneProperty: TReadOnlyProperty<{ sourceType: string }>;
currentPhotonWaveDisplayModeProperty: PhetioProperty<PhotonWaveDisplayMode>;
currentMatterWaveDisplayModeProperty: PhetioProperty<MatterWaveDisplayMode>;

The photon and matter sections repeat the ComboBox, width affirmation, titled VBox, PhET-iO options, and gated visibility construction. Could one named control creator accept the mode Property, items, strings, gate, and tandem name so the two variants remain visibly parallel without duplicating the plumbing?

const photonWaveDisplayControl = new VBox( {
  spacing: 6,
  align: 'center',

The photon and matter sections repeat the ComboBox, affirm, titled VBox, options, and GatedVisibleProperty. Maybe one creator could reduce lines of code.

const photonWaveDisplayControl = new VBox( {
  spacing: 6,
  align: 'center',

the zone-to-string association is duplicated here and in getWavelengthColorZoneStringProperty. You could have one Record<WavelengthColorZone, TReadOnlyProperty<string>>? and the dependency Properties could be derived from it.

export type WavelengthColorZone = 'violet' | 'blue' | 'indigo' | 'green' | 'yellow' | 'orange' | 'red';

Should the utility function do the rounding so that clients don't have to?

export function getWavelengthColorZone( wavelength: number ): WavelengthColorZone {

Wait, isn't indigo closer to violet? Is "indigo" supposed to be "cyan"?

return wavelength <= 450 ? 'violet' :
       wavelength <= 485 ? 'blue' :
       wavelength <= 500 ? 'indigo' :

This is NOT the half height in unipolar mode? Should this be renamed?

this.halfAmplitudeHeightProperty = new DerivedProperty( [ options.polarityProperty ], polarity =>
  polarity === 'unipolar' ? this.chartHeight - 5 : this.chartHeight / 2 - 5
);

This seems like a good candidate for one SoundRichDragListener that can do both forms of input.

this.addInputListener( new SoundDragListener( {
  positionProperty: this.positionProperty,
  dragBoundsProperty: adjustedDragBoundsProperty,

A lot of duplication here with BaseScreenModel, similar to the pattern what I described WaveVisualizableScene and MeasurementToolsLayerNode. I don't think this is useful decoupling.

readonly sceneProperty: TReadOnlyProperty<WaveVisualizableScene & {
  readonly barrierPositionFractionProperty: TRangedProperty;
  readonly slitSeparationDisplayRange: Range;

I noticed that these RGBA functions allocate objects for every grid cell on every paint. This area seems performance intensive so I wanted to call it out in case you want to tryreusable objects or a cache.

const color = usesLayeredFieldSamples && solver.getLayeredFieldSampleAtGridCell ?
              getLayeredFieldSampleRGBA( solver.getLayeredFieldSampleAtGridCell( gx, gy ), displayMode, baseColor, amplitudeScale, colorPower ) :
              getFieldSampleRGBA( solver.getFieldSampleAtGridCell( gx, gy ), displayMode, baseColor, amplitudeScale, colorPower );

What does "nice" mean? I assume pleasent for layout or visuals, but it would help to define at the top here.

function computeNiceScale( regionWidthMeters: number, regionWidthPixels: number ): { distanceMeters: number; barPixels: number } {
  const metersPerPixel = regionWidthMeters / regionWidthPixels;
  const targetMeters = TARGET_BAR_PX * metersPerPixel;

step() already invalidates, why do you need these?

sceneProperty.link( ( scene, previousScene ) => {
  if ( previousScene ) {
    previousScene.isWaveVisibleProperty.unlink( invalidateWaveCanvas );

Is this working with dynamic locales? I didn't see any problems but the label can change.

  timeLabel.bottom = height - SCALE_MARGIN;
  this.addChild( timeLabel );
}

This has a lot more now! Update doc? See

import { toFixed } from '../../../dot/js/util/toFixed.js';
import Node from '../../../scenery/js/nodes/Node.js';
import Text from '../../../scenery/js/nodes/Text.js';

"Increased" sounds like a reference to an old prompt, I would remove that.

public static readonly FRONT_FACING_ROW_TOP = 180;

I would remove getDecimalPlacesForValue and getRangeDecimalPlaces now that they are in common.

public static getDecimalPlacesForValue( value: number ): number {
  return QuantumWaveInterferenceConstants.getRangeDecimalPlaces( value, value );
}

Update this doc? No longer single screen.

import Screen, { ScreenOptions } from '../../../joist/js/Screen.js';
import ScreenIcon, { MINIMUM_HOME_SCREEN_ICON_SIZE } from '../../../joist/js/ScreenIcon.js';
import optionize, { EmptySelfOptions } from '../../../phet-core/js/optionize.js';

The length of this is assumed in accessible strings as "4 zoom levels", want to move that data into string patterns? If not, note that it is dangerous to addd scales.

export const DETECTOR_SCREEN_SCALE_OPTIONS = [
  { minMM: -20, maxMM: 20 },
  { minMM: -15, maxMM: 15 },

These fields are only used during construction. Could they be locals, and should scenes be a ReadonlyArray?

private readonly photonsScene: SceneModel;
private readonly electronsScene: SceneModel;
private readonly neutronsScene: SceneModel;

Good documentation. But incomplete, and I don't think it should be. Instead of duplicating, point people to documentation in SceneModel?

public readonly currentSlitConfigurationProperty: DynamicProperty<SlitConfiguration, SlitConfiguration, SceneModel>;
public readonly currentDetectionModeProperty: DynamicProperty<DetectionMode, DetectionMode, SceneModel>;
public readonly currentIsEmittingProperty: DynamicProperty<boolean, boolean, SceneModel>;

Users treat this as read-only. Does this need to be bidirectional?

  bidirectional: true
} );

This appears to be in view coordinates, and layout bounds are a view concern. Should the position be initialized there?

this.rulerPositionProperty = new Vector2Property( new Vector2( 300, 350 ), {
  tandem: tandem.createTandem( 'rulerPositionProperty' ),
  phetioReadOnly: true

const effectiveDt = timeSpeed === TimeSpeed.FAST ? dt * 4 : // REVIEW: Factor out like NORMAL_TIME_SPEED_FACTOR?

const effectiveDt = timeSpeed === TimeSpeed.FAST ? dt * 4 : // REVIEW: Factor out like NORMAL_TIME_SPEED_FACTOR? https://github.com/phetsims/totality/issues/221

This file is large and duplicates a lot of source, hit, snapshot, and reset code from BaseSceneModel. Could those responsibilities be shared through smaller composed models or helpers? What if the hierarchy was more like this? BaseSceneModel ExperimentSceneModel BaseWaveSceneModel HighIntensitySceneModel SingleParticlesSceneModel

export default class SceneModel extends PhetioObject {

Should hits be private or read-only so mutations cannot bypass totalHitsProperty and hitsChangedEmitter? It looks like users only access hits for length. But should they be using totalHitsProperty?

public readonly hits: Vector2[];

Why do you need this? This is a feature of Screen.ts and the default value is 0.5s. If it is needed, should it be a shared named constant? It is also in HighIntensitySceneModel.

if ( dt > 0.5 ) {
  return;
}

This stands out, this is not a clone or copy, its the actual array. This is described as a "snapshot" is this an issue?

    hits: sceneModel.hits
  };
}

This depends on exact localized description strings. That will not be safe for translated versions of the description and is generally fragile. Can this be implemented with simulation state?

const detectorScreenIsEmptyProperty = new DerivedProperty(
  [
    detectorScreenDescriber.descriptionProperty,

should this be marked isDisposable: false?

super( {
  accessibleTemplate: AccessibleList.createTemplateProperty( {
    leadingParagraphStringProperty: leadingParagraphStringProperty,

Do you think we should make these mappings unnecessary? This maps state -> other state -> fluent. Why can't we do enumeration -> fluent? Might be worth an issue in chipper. See

return timeSpeed === TimeSpeed.NORMAL ? 'normal' :
       timeSpeed === TimeSpeed.FAST ? 'fast' :
       ( () => { throw new Error( `Unrecognized timeSpeed: ${timeSpeed}` ); } )();

This class builds the PDOM order for Nodes owned elsewhere. I think it would be better for each section Node to own its heading and internal PDOM order while the screen view orders sections.

sourceControlPanel: SourceControlPanel<ExperimentSceneModel>,
sceneRadioButtonGroup: SceneRadioButtonGroup<ExperimentSceneModel>,
slitControlPanel: SlitControlPanel,

I don't understand why this Node is necessary, recommend deleting. If you keep it, recommend renaming because it does more than announce "start".

const sourceStartedResponseNode = new Node();
this.addChild( sourceStartedResponseNode );
model.currentIsEmittingProperty.lazyLink( isEmitting => {

This file is complicated and hard to understand. Is there a feature missing from AccessibleValueHandler? I suspect this would be easier to understand if you did not use createContextResponseAlert and just triggered your own response queueing from endCallback. Alternatively, would this have been easier if AccessibleValueHandler had queuing options like responseGroup for its responses?

import { TReadOnlyProperty } from '../../../../../axon/js/TReadOnlyProperty.js';
import TimeSpeed from '../../../../../scenery-phet/js/TimeSpeed.js';
import type SceneryEvent from '../../../../../scenery/js/input/SceneryEvent.js';

This duplicates with formatExperimentLiveHitsResponse.

const scene = model.sceneProperty.value;
const slitSetting = model.currentSlitConfigurationProperty.value;
const isDoubleSlit = showsDoubleSlitInterferencePattern( slitSetting );

responseCreated is set to true, even when createResponse returns null. Is that OK?

interaction.responseCreated = true;
const response = createResponse(
  valueProperty.value,

This is duplicated with formatExperimentLiveHitsResponse and ExperimentSliderAccessibleResponses.

const scene = model.sceneProperty.value;
const analysis = BandAnalysis.analyzeTheoreticalPattern(
  scene,

I am pretty sure this comment is stale and the description now lives in ExperimentSliderAccessibleResponses? I would remove comments like this, it is too become stale. If the match is important, consider factoring out the implementation.

return formatLiveHitsDescription(
  hitStage,
  model.currentSlitConfigurationProperty.value,

This is hard for a human to understand. I don't think shorthand is appropriate,

return mode === 'intensity' ?
       ( model.currentIsEmittingProperty.value ? formatExperimentDetectorPatternResponse( model ) : screenEmptyResponse ) :
       model.currentIsEmittingProperty.value && model.isPlayingProperty.value ? null :

We usually place these below the constructor and instance variables.

public static formatDescription( sceneModel: ExperimentSceneModel, isRulerVisible: boolean ): string {
  const detectionMode = sceneModel.detectionModeProperty.value;
  const slitSetting = sceneModel.slitConfigurationProperty.value as SlitConfigurationWithNoBarrier;

This basically duplicates the list in linkSceneDependencies, could that utility be used?

sceneModel.hitsChangedEmitter.addListener( update );
sceneModel.detectionModeProperty.lazyLink( fullUpdate );
sceneModel.isEmittingProperty.lazyLink( fullUpdate );

I think this many arguments hurts readability, it would be better to just take the ExperimentModel, and maybe the detectorColumnNode.

public constructor(
  sceneProperty: TReadOnlyProperty<SceneModel>,
  scenes: SceneModel[],

Labels have negative values, is this a place where we need to use StringUtils.toFixedLTR?

  return toFixed( labelValue, labelDecimalPlaces );
} );

should this be marked isDisposable: false?

super( {
  children: [ rulerContainer ],
  cursor: 'pointer',

combineOptions would be better for this,

tagName: AccessibleDraggableOptions.tagName,
focusable: AccessibleDraggableOptions.focusable,
ariaRole: AccessibleDraggableOptions.ariaRole,

This is duplicated with updateScaleIndicator,

const fullPhysicalWidthMM = metersToMillimeters(
  getDetectorScreenHalfWidthForScaleIndex( detectorScreenScaleIndex ) * 2
);

Layout in this file uses the first node in each scene array, assuming identical bounds across scenes. Is that OK? Alternatively, could use a layout container avoid this assumption. Or compute positioning each time by looking over all scenes.

import BooleanProperty from '../../../../axon/js/BooleanProperty.js';
import Node from '../../../../scenery/js/nodes/Node.js';
import Tandem from '../../../../tandem/js/Tandem.js';

Use detectorScreenLeft()?

  return this.detectorScreenNodes[ 0 ].x + ExperimentConstants.DETECTOR_SCREEN_WIDTH / 2;
}

Use detectorScreenLeft()?

  return this.detectorScreenNodes[ 0 ].x + ExperimentConstants.DETECTOR_SCREEN_WIDTH;
}

Assigned but never read, remove?

private readonly maxHitsReachedPanel: OverheadEmitterNode['maxHitsReachedPanel'];

Parens or a flat table would make both of these easier to undersetand, but we allow nested = ternary in this project.

return graphExpanded ?
       increased ?
       'detectorScreenBrighterGraphAmplitudeIncreases' :

I think this is supposed to link to bounds, linking to teh scene introduces a listener order dependency.

model.sceneProperty.link( updateSourceControlPanelPosition );

Should this list be using createDetectorScreenRenderStateFromSceneModel? It seems to mirror that manually,

sceneModel.hitsChangedEmitter.addListener( invalidateDetectorScreen );
sceneModel.detectionModeProperty.link( invalidateDetectorScreen );
sceneModel.isEmittingProperty.link( invalidateDetectorScreen );

What does "previous slit height" mean?

const BASE_SLIT_HEIGHT = 130; // Previous slit height, used to derive padding behavior
const BASE_SLIT_VERTICAL_PADDING = ( VIEW_HEIGHT - BASE_SLIT_HEIGHT ) / 2;
const SLIT_VERTICAL_PADDING = BASE_SLIT_VERTICAL_PADDING / 2;

This interface is difficult to understand and requires confusing code in the function. I would recommend another argument or named option. Also, no caller uses options. Can isDetectorScreenTextureOptions be deleted?

  detectorScreenScaleIndexPropertyOrOptions?: TReadOnlyProperty<number> | DetectorScreenTextureOptions,
  providedOptions?: DetectorScreenTextureOptions
): HTMLCanvasElement {

This constructor is pretty large, do you think it would be more maintainable if the graph or other responsibilities were factored out?

public constructor( sceneModel: SceneModel, providedOptions: GraphAccordionBoxOptions ) {

This updates the graph ~10 times during construction, consider lazyLink

sceneModel.hitsChangedEmitter.addListener( updateGraph );
sceneModel.isEmittingProperty.link( () => updateGraph() );
sceneModel.detectionModeProperty.link( () => updateGraph() );

Why does the beam Node own detector-pattern invalidation? This couples an unrelated visual to another Node's full rendering dependency list, which is also maintained in FrontFacingDetectorScreenNode and getDetectorScreenTexture. Could OverheadDetectorPatternNode observe its own scene and dependencies?

const updateOverheadPattern = () => {
  detectorScreenNode.overheadPatternNode.updatePattern( sceneProperty.value );
};

Why are these exported?

export const DETECTOR_DX = BASE_DETECTOR_DX * OVERHEAD_SCALE;
export const DETECTOR_DY = BASE_DETECTOR_DX * ( BASE_DOUBLE_SLIT_SKEW_DY / BASE_DOUBLE_SLIT_SKEW_DX ) * OVERHEAD_SCALE * OVERHEAD_SKEW_SCALE;
export const DETECTOR_LEFT_HEIGHT = 48 * OVERHEAD_SCALE;

There are many magic number scaling factors used in this file, chained together in a way that would make maintenance/tuning difficult. Could the scaling be simplified at this point? Or could scaling factors be named so that it is clear what they are doing?

const LABEL_Y = 24;
const BASE_PARALLELOGRAM_X = 365;
const BASE_PARALLELOGRAM_DX = 51;

What are these numbers and how do they relate to BASE_PARALLELOGRAM_DX?

public readonly skewDx = 51 * OVERHEAD_SCALE;
public readonly skewDy = 21 * OVERHEAD_SCALE * OVERHEAD_SKEW_SCALE;

Move these to other file level constants?

const MIN_VISUAL_SLIT_SPACING = OVERHEAD_SCALE;
const MAX_VISUAL_SLIT_SPACING = 8 * OVERHEAD_SCALE;

This file could use DetectorSide for readability if you want. The one usage of it has that ready to go.

public getDetectorAnchorPoint( isLeftDetector: boolean ): Vector2 {
  return isLeftDetector ? new Vector2(
    this.parallelogramNode.x + this.leftSlitDetectorOverlay.x,

Why is this exported?

export function getParallelogramCorners( dx: number, dy: number, leftHeight: number ): Vector2[] {
  return [
    new Vector2( 0, 0 ),

Duplicate JSDoc with constructor

export default class ParallelogramNode extends Path {

Documentation would help here, it requires context to understand why graphExpandedProperty is here.

public constructor(
  visibleBoundsProperty: TReadOnlyProperty<Bounds2>,
  sceneProperty: TReadOnlyProperty<SceneModel>,

graphExpandedProperty's value is ignored, so it appears to be a signal that AccordionBox has changed its layout. Does that rely on listener order? Could this observe the actual graph bounds or use the expanded value so the drag range cannot be computed from stale/collapsed geometry?

const detectorRectCenterX = detectorScreenNode.x + ExperimentConstants.DETECTOR_SCREEN_WIDTH / 2;
const fixedLeft = detectorRectCenterX - rulerNode.width / 2 + RULER_X_OFFSET;

fixedLeft uses detectorScreenNode.x in its immediate parent coordinates, while the vertical bounds are converted into localRootNode coordinates. Could detectorScreenRectBounds.centerX be used so the calculation stays correct if the detector column acquires a transform?

const minTopFromScreen = detectorScreenRectBounds.top;
const graphChartBounds = localRootNode.globalToLocalBounds( graphAccordionBox.getChartAreaGlobalBounds() );
const maxTopFromGraph = graphChartBounds.bottom - rulerNode.height + graphAccordionBox.getChartAreaStrokeLineWidth();

This expression is difficult to understand, re-writing or different formatting or documentation might help.

const consequence: SliderContextResponseConsequence =
  change === null ||
  !scene.isEmittingProperty.value ||

There are several string patterns in this file that are not Fluent patterns and not localized. Should they be moved into the yaml? If not, document why.

import type PhetioProperty from '../../../../axon/js/PhetioProperty.js';
import { TReadOnlyProperty } from '../../../../axon/js/TReadOnlyProperty.js';
import PickRequired from '../../../../phet-core/js/types/PickRequired.js';

I think this is stale if I understand correctly - ranges are static, each scene gets its own controls.

import BooleanProperty from '../../../../axon/js/BooleanProperty.js';
import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
import Property from '../../../../axon/js/Property.js';

This file is very similar to SlitSeparationNumberControl and I wondered if commonalities should be factored out.

import { TReadOnlyProperty } from '../../../../axon/js/TReadOnlyProperty.js';
import Dimension2 from '../../../../dot/js/Dimension2.js';
import Range from '../../../../dot/js/Range.js';

Is this number supposed to match the scale factor for "fast" in ExperimentModel? If it is important that they are the same, factor out.

const FAST_TIME_SPEED_FLASH_DURATION_FACTOR = 4;

This doesn't use dt, variable frame rates could change the detector pattern. I am not sure if that is a problem worth solving.

for ( let iy = 0; iy < this.gridHeight; iy++ ) {
  this.detectorAccumulator[ iy ] += this.detectorDistribution[ iy ];
}

Should each solver accept its own state type? Why would this take a SingleParticleSolverState?

public setState( state: WaveSolverState ): void {
  this.time = state.time;
  this.sourceOnTime = 'sourceOnTime' in state ? state.sourceOnTime : null;

See https://github.com/phetsims/quantum-wave-interference/issues/426 and

const isMeaningful =
  effectiveAction.type === 'sourceChanged' ? before.isEmitting !== after.isEmitting :
  effectiveAction.type === 'particleTypeChanged' ? before.sourceType !== after.sourceType :

Is createContextResponseAlert best for this? This looks like endCallback work.

const interaction = this.sliderInteraction;

This adds an indirect API and makes callers look here for types that this file does not define. I would have callers import them from their actual owners.

export type {
  QuantumWaveInterferenceClockSpeedDescription,
  QuantumWaveInterferenceIntensityPatternFormation,

This is not in the list from createDependencies, is this a problem? Is it working? See https://github.com/phetsims/quantum-wave-interference/issues/426 and

  return model.sceneProperty.value.detectorPatternFormationFactorProperty.value;
}

This file and function is named like a class, rename -

export default function HighIntensityExperimentSetupSequenceItems( model: HighIntensityModel ): AccessibleListItem[] {
  return [
    createItem(

Why this fallback?

photonColor: state.wavelengthColorZone || 'red',
wavefrontSpacing: state.wavefrontSpacing,
waveDisplayMode: state.waveDisplayMode,

This is unused, remove.

export function formatSlitDescription( state: HighIntensityDescriptionState ): string {
  return QuantumWaveInterferenceFluent.a11y.waveExperimentState.slits.format( {
    slitSetting: state.slitConfiguration,

This is not a template anymore. Also, it is only used by HighIntensityExperimentSetupSequenceItems. Move it there or rename?

export default class QuantumWaveInterferenceAccessibleStateTemplate {

This seems fragile. But also, this list is duplicated with createHighIntensityDescriptionState. https://github.com/phetsims/quantum-wave-interference/issues/426

return Array.from( new Set( [
  model.sceneProperty,
  model.currentIsEmittingProperty,

These layout values are duplicated with SingleParticlesScreenView, is it important that they are the same accross screen views? If so, factor out.

const CONTENT_VERTICAL_OFFSET = 12;
const SOURCE_BEAM_RIGHT_PANEL_GAP = 10;

Why not factor out this duplication?

const slitFraction = scene.barrierPositionFractionProperty.value;
const slitWindow = 0.04;
const slitSeparationFraction = scene.slitSeparationProperty.value * 1e-3 / scene.regionWidth;

This class has many responsibilities and is nearly 1000 lines, could some of it be factored into smaller components?

export default class HighIntensityScreenView extends ScreenView {

Why does this method create particleMassAnnotation but not add it like the other Nodes?

const particleMassAnnotation = new ParticleMassAnnotationNode( model.sceneProperty, model.currentParticleSpeedProperty );

This layout code is also duplicated in the SingleParticlesScreenView (around line 171 at time of writing.

private createWaveRegionLayout( leftColumnWidth: number ): WaveRegionLayout {
  const waveRegionLeft = QuantumWaveInterferenceConstants.SCREEN_VIEW_X_MARGIN + leftColumnWidth + 20;
  const baseWaveRegionTop = QuantumWaveInterferenceConstants.SCREEN_VIEW_Y_MARGIN + SOURCE_BEAM_THUMBNAIL_CENTER_Y + THUMBNAIL_GAP;

link is already calling this, remove this?

  updateParticleMassAnnotationPosition();
  this.addChild( particleMassAnnotation );
}

Regarding the similar logic, should it be factored out? It seems reasonable to put "detector screen status" as its own model Property?

const detectorScreenStatusProperty = DerivedProperty.deriveAny(
  [
    model.sceneProperty,

Is this really a "thumbnail"? It is much longer and has more responsibilities than I expected. I expected it to be an icon, but is this the actual full view assembly?

import Multilink from '../../../../axon/js/Multilink.js';
import TProperty from '../../../../axon/js/TProperty.js';
import { TReadOnlyProperty } from '../../../../axon/js/TReadOnlyProperty.js';

What do the three chained scale factors represent? There are several lines like this and it is difficult to understand. Could they be simplified now?

const HIGH_OPACITY_BEAM_STACK_HEIGHT = EMITTER_NOZZLE_HEIGHT * 0.85 * 0.9 * 0.85;
const HIGH_OPACITY_BEAM_ALPHA_SCALE = 0.6;
const DIM_RIGHT_BEAM_ALPHA_SCALE = 0.3;

Structural typing: See https://github.com/phetsims/quantum-wave-interference/issues/411

type SourceBeamThumbnailSceneLike = {
  sourceType: SourceType;
  slitSeparationDisplayRange: Range;

Most of this is is duplicated with OverheadEmitterNode,

const emitterChildren: LaserPointerNode[] = scenes.map( scene => {
  const palette = getParticleEmitterPalette( scene.sourceType );
  return new LaserPointerNode( currentIsEmittingProperty, {

Why do you need this, and why doesn;t the dependency list of the ManualConstraint cover it?

  emitterConstraint.updateLayout();
} );

This multilink immediately invalidates a lot of the beam geometry above. I suspect that means some of the layout code above could be removed or factored out.

Multilink.multilink(
  [
    sceneProperty,

Regarding the radius must be the same across all scenes - should this be enforced? This is a deep reach. What do you think about a shared constant or different way to provide this value?

this.radiusRange = sceneProperty.value.detectorProbe.radiusProperty.range;

I could not find anything that sets this, does it need to be bidirectional?

  bidirectional: true
} );

This is controlled by the model. Should it be phetioReadOnly like probabilityProperty?

  phetioFeatured: true
} );

This is assigned but never read. Can it be removed?

private readonly photonsScene: SingleParticlesSceneModel;

At first, I was concerned that tuning in pixels would result in solver behavior changing with layout. . But that is not the case here. Perhaps rename to "view coordinates" instead of pixels.

const MEASUREMENT_BITE_EDGE_FEATHER_PIXELS = 5;

This is constant and only used by view code. Should it be a WaveVisualizationNode option instead of a model Property? I don't see it changing, why is it a Property?

const SINGLE_PARTICLES_WAVE_AMPLITUDE_SCALE = 2;

Why 100 here?

for ( let i = 0; i < 100; i++ ) {
  const candidate = parameters.startWeight +
                    dotRandom.nextDouble() * ( parameters.endWeight - parameters.startWeight );

The function name is clear but the math is not, consider adding doc for the maintainer.

private getScreenDetectionCurveDensity( weight: number ): number {
  const parameters = SCREEN_DETECTION_TIMING_PARAMETERS;

Doc for this would be helpful.

private getDeterministicSlitArrivalTime(): number {
  const propagationSpeed = this.waveSolver.getDisplayPropagationSpeed();
  if ( propagationSpeed <= 0 ) {

doc?

private detectPacket(): void {
  if ( !this.isPacketActiveProperty.value ) {
    return;

Some fields are missing here like targetDetectionTime, targetOnSlitDetectionTime, deterministicOnSlitArrivalTime

public override reset(): void {
  super.reset();
  this.autoFireModeProperty.reset();

References like this aren't really "snapshot" - they will be current. Is that OK?

scene: SingleParticlesSceneModel;
isPlaying: boolean;
clockSpeedDescription: QuantumWaveInterferenceClockSpeedDescription;

Why this fallback?

photonColor: state.wavelengthColorZone || 'red',
waveDisplayMode: state.waveDisplayMode,
slitSetting: state.slitConfiguration,

Why this fallback?

photonColor: state.wavelengthColorZone || 'red',
waveDisplayMode: state.waveDisplayMode,
slitSetting: state.slitConfiguration,

This mapping is duplicated in HighIntensityExperimentSetupSequenceItems. Consider a new abstraction for these descriptions?

return patternKind === 'doubleSlitInterference' ? 'interferingAfterSlits' :
       patternKind === 'singleSlitDiffraction' ? 'diffractingAfterSlits' :
       patternKind === 'whichPathDiffraction' ? 'whichPathAfterSlits' :

should this be localized? See

`${configurationResponse} ${hitsClearedResponse}`;

This class is pretty long and has a lot of responsibilities. It creates the draggable probe, control panel, slider, and readout. constrainControlPanelTo was the biggest red flag to me. I think it would be clearer to break this up and manage the panel’s placement in SingleParticlesScreenView.

import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
import Multilink from '../../../../axon/js/Multilink.js';
import { type TReadOnlyProperty } from '../../../../axon/js/TReadOnlyProperty.js';

Why is this 5 here? How does this relate to the model's range (in DetectorProbe.ts)? That min is 0.06, WAVE_REGION_WIDTH is 420, so viewRadius is at least 25.2?

circleNode.radius = Math.max( viewRadius, 5 );
const circleLabelMaxWidth = 2 * circleNode.radius - 4;
probabilityText.maxWidth = circleLabelMaxWidth;

I could not find the controlPanel.localBoundsProperty link described above.

if ( controlPanel.bounds.isFinite() ) {
  const panelCX = controlPanel.centerX;
  const panelTY = controlPanel.top;

This seems like a good candidate for one SoundRichDragListener that can handle both pointer and keyboard input.

const dragListener = new SoundDragListener( {
  positionProperty: currentDetectorProbe.positionProperty,
  transform: detectorProbeModelViewTransform,

Only sourceType is used. It would be simpler and more decoupled to accept a TReadOnlyProperty<SourceType> See https://github.com/phetsims/quantum-wave-interference/issues/411 and

type SceneLike = { sourceType: SourceType };

Changing this maxWidth could change the bounds of the dependency checkbox and trigger an infinite loop. Why does this need to be dynamic anyway, could it be simplified? I read getAutoFireModeCheckboxTextMaxWidth but wonder if it is worth it.

const autoFireModeCheckboxTextMaxWidth = getAutoFireModeCheckboxTextMaxWidth( emitterProxy.width );
if ( autoFireModeCheckboxText.maxWidth !== autoFireModeCheckboxTextMaxWidth ) {
  autoFireModeCheckboxText.maxWidth = autoFireModeCheckboxTextMaxWidth;