Add review comments and address minor issues, see #123

js/common/DensityBuoyancyCommonConstants.ts

@@ -45,7 +45,6 @@ export const chooseDecimalPlaces = ( value: number ): number => {
 
 const LITERS_IN_CUBIC_METER = 1000;
 
-
 const FLUID_DENSITY_RANGE_PER_L = new Range( 0.5, 15 );
 
 const DensityBuoyancyCommonConstants = {

js/common/view/ABControlsNode.ts

@@ -29,12 +29,12 @@ export default class ABControlsNode extends ABPanelsNode {
    * @param massA
    * @param massB
    * @param popupLayer
-   * @param options - Applied to each BlockControlNode
+   * @param blockControlNodeOptions - Applied to each BlockControlNode
    */
-  public constructor( massA: Cuboid, massB: Cuboid, popupLayer: Node, options: ABControlsNodeOptions ) {
+  public constructor( massA: Cuboid, massB: Cuboid, popupLayer: Node, blockControlNodeOptions: ABControlsNodeOptions ) {
 
-    const tandem = options.tandem;
-    const omittedOptions = _.omit( options, [ 'tandem' ] );
+    const tandem = blockControlNodeOptions.tandem;
+    const omittedOptions = _.omit( blockControlNodeOptions, [ 'tandem' ] );
 
     const controlANode = new BlockControlNode( massA, popupLayer, true, combineOptions<BlockControlNodeOptions>( {
       labelNode: ABPanelsNode.getTagALabelNode(),

js/common/view/BackgroundEventTargetListener.ts

@@ -127,6 +127,8 @@ export default class BackgroundEventTargetListener implements TInputListener {
         this.releaseSoundPlayer.play();
         this.endDragAction.execute( mass );
       };
+
+      // TODO: Add a type annotation so we can track down documentation for createPanTargetBounds, see https://github.com/phetsims/density-buoyancy-common/issues/123
       const listener = {
         // end drag on either up or cancel (not supporting full cancel behavior)
         up: endDrag,

js/common/view/BlocksModeRadioButtonGroup.ts

@@ -27,6 +27,7 @@ const bMaterial = new THREE.MeshLambertMaterial();
 
 const ICON_SCALE = 0.15;
 
+// TODO: Add helpful documentation please, see https://github.com/phetsims/density-buoyancy-common/issues/257
 // These live for the lifetime of the simulation, so we don't need to remove this listener
 DensityBuoyancyCommonColors.tagAProperty.link( labelColor => {
   aMaterial.color = ThreeUtils.colorToThree( labelColor );

js/common/view/BlocksPanel.ts

@@ -27,6 +27,8 @@ export default class BlocksPanel extends Panel {
         createNode: tandem => new Text( blockSet.stringProperty, {
           font: DensityBuoyancyCommonConstants.RADIO_BUTTON_FONT,
           maxWidth: 160,
+
+          // TODO: This looks like a rare occasion where we instrumented a Text, but normally we don't instrument those. Can this be uninstrumented? See https://github.com/phetsims/density-buoyancy-common/issues/123
           tandem: tandem.createTandem( 'labelText' )
         } ),
         value: blockSet,
@@ -58,7 +60,6 @@ export default class BlocksPanel extends Panel {
         phetioFeatured: true
       }
     }, DensityBuoyancyCommonConstants.PANEL_OPTIONS ) );
-
   }
 }
 

js/common/view/ComboNumberControl.ts

@@ -38,6 +38,7 @@ type SelfOptions<T extends Material | Gravity> = {
 
   comboItems: ComboBoxItem<T>[];
 
+  // TODO: What is a fallback node? When is it shown? https://github.com/phetsims/density-buoyancy-common/issues/257
   getFallbackNode?: ( t: T ) => Node | null;
 
   numberControlOptions?: NumberControlOptions;

js/common/view/ComparisonNumberControl.ts

@@ -1,7 +1,7 @@
 // Copyright 2021-2024, University of Colorado Boulder
 
 /**
- * Supertype for the NumberControls in the Compare screen
+ * Subtype for the NumberControls in the Compare screen, see BlocksValuePanel.
  *
  * @author Jonathan Olson (PhET Interactive Simulations)
  */

js/common/view/DensityBuoyancyScreenView.ts

@@ -2,6 +2,8 @@
 
 /**
  * The main base ScreenView for all Density/Buoyancy screens.
+ * TODO: Elaborate on the main responsibilities of this class, see https://github.com/phetsims/density-buoyancy-common/issues/257
+ * TODO: Currently at 871 lines, this file is more complex. Can it be simplified or modularized? See https://github.com/phetsims/density-buoyancy-common/issues/123
  *
  * @author Jonathan Olson (PhET Interactive Simulations)
  */
@@ -89,7 +91,7 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
   private readonly backgroundLayer: Node;
   protected readonly resetAllButton: Node;
 
-  // The sky background, in a unit 0-to-1 rectangle (so we can scale it to match)
+  // The sky background
   private readonly backgroundNode: Rectangle;
 
   protected readonly sceneNode: ThreeIsometricNode;
@@ -100,6 +102,7 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
 
   private readonly debugView?: DebugView;
 
+  // TODO: What are these for? See https://github.com/phetsims/density-buoyancy-common/issues/257
   // Subtypes can provide their own values to control the barrier sizing.
   private readonly leftBarrierViewPointPropertyProperty: Property<TReadOnlyProperty<Vector2>>;
   protected readonly rightBarrierViewPointPropertyProperty: Property<TReadOnlyProperty<Vector2>>;
@@ -154,6 +157,7 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
     } );
     this.addChild( this.backgroundNode );
 
+    // TODO: The backgroundNode is behind the backgroundLayer, please document, see https://github.com/phetsims/density-buoyancy-common/issues/257
     this.backgroundLayer = new Node();
     this.addChild( this.backgroundLayer );
 
@@ -205,7 +209,7 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
       this.massViews,
       this.getMassViewUnderPointer.bind( this ),
       this.sceneNode.getRayFromScreenPoint.bind( this.sceneNode ),
-      ( point: Vector3 ) => this.localToGlobalPoint( this.modelToViewPoint( point ) ),
+      point => this.localToGlobalPoint( this.modelToViewPoint( point ) ),
       updateCursor,
       this.tandem.createTandem( 'backgroundEventTargetListener' )
     );
@@ -350,7 +354,7 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
       topColorArray[ offset + 8 ] = topColorArray[ offset + 14 ] = topColorArray[ offset + 17 ] = grassFarColor.b / 255;
       topGeometry.attributes.color.needsUpdate = true;
     } );
-    // @ts-expect-error - THREE.js version incompat?
+    // @ts-expect-error - THREE.js version incompatibility?
     const topMaterial = new THREE.MeshBasicMaterial( { vertexColors: THREE.VertexColors } );
     const topMesh = new THREE.Mesh( topGeometry, topMaterial );
     this.sceneNode.stage.threeScene.add( topMesh );
@@ -453,6 +457,7 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
         opacity: 0.5
       } );
 
+      // TODO: Document the purpose of this link, see https://github.com/phetsims/density-buoyancy-common/issues/257
       // This instance lives for the lifetime of the simulation, so we don't need to remove this listener
       model.invisibleBarrierBoundsProperty.link( bounds => {
         let index = 0;
@@ -503,7 +508,6 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
     model.masses.forEach( onMassAdded );
 
     const onMassRemoved = ( mass: Mass ) => {
-      // Mass view
       const massView = _.find( this.massViews, massView => massView.mass === mass )!;
 
       // Remove the mass view
@@ -515,6 +519,8 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
 
     const fluidLevelIndicator = new FluidLevelIndicator( model.pool.fluidLevelVolumeProperty );
     this.addChild( fluidLevelIndicator );
+
+    // TODO: Document the reason for this link: https://github.com/phetsims/density-buoyancy-common/issues/257
     // This instance lives for the lifetime of the simulation, so we don't need to remove this listener
     model.pool.fluidYInterpolatedProperty.link( fluidY => {
       const modelPoint = new Vector3( model.poolBounds.minX, fluidY, model.poolBounds.maxZ );
@@ -831,7 +837,6 @@ export default class DensityBuoyancyScreenView<Model extends DensityBuoyancyMode
     return array;
   }
 
-
   /**
    * Fills the positionArray with an X,Z cross-section of the fluid around a boat at a given y value (for a given liters
    * value).

js/common/view/EllipsoidView.ts

@@ -17,17 +17,15 @@ import DisplayProperties from '../../buoyancy/view/DisplayProperties.js';
 
 export default class EllipsoidView extends MeasurableMassView {
 
-  private readonly ellipsoid: Ellipsoid;
   private readonly ellipsoidGeometry: THREE.SphereGeometry;
   private readonly updateListener: ( newSize: Bounds3 ) => void;
 
-  public constructor( ellipsoid: Ellipsoid, modelViewTransform: THREEModelViewTransform, displayProperties: DisplayProperties ) {
+  public constructor( private readonly ellipsoid: Ellipsoid, modelViewTransform: THREEModelViewTransform, displayProperties: DisplayProperties ) {
 
     const ellipsoidGeometry = new THREE.SphereGeometry( 0.5, 30, 24 );
 
     super( ellipsoid, ellipsoidGeometry, modelViewTransform, displayProperties );
 
-    this.ellipsoid = ellipsoid;
     this.ellipsoidGeometry = ellipsoidGeometry;
 
     const positionTag = () => {

js/common/view/FluidLevelIndicator.ts

@@ -15,7 +15,6 @@ import Panel from '../../../../sun/js/Panel.js';
 import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
 import DensityBuoyancyCommonConstants from '../DensityBuoyancyCommonConstants.js';
 import DensityBuoyancyCommonColors from './DensityBuoyancyCommonColors.js';
-import Tandem from '../../../../tandem/js/Tandem.js';
 
 export default class FluidLevelIndicator extends Node {
 
@@ -29,19 +28,16 @@ export default class FluidLevelIndicator extends Node {
     } );
     this.addChild( highlightPath );
 
-    // This instance lives for the lifetime of the simulation, so we don't need to remove this listener
     const readoutText = new RichText( new PatternStringProperty( DensityBuoyancyCommonConstants.VOLUME_PATTERN_STRING_PROPERTY, {
       value: volumeProperty
     }, {
-      tandem: Tandem.OPT_OUT,
       decimalPlaces: 2
     } ), {
       font: new PhetFont( { size: 18 } ),
       maxWidth: 70
     } );
 
     const readoutPanel = new Panel( readoutText, {
-      // Not using the typical margins for UI panels in this sim
       cornerRadius: DensityBuoyancyCommonConstants.CORNER_RADIUS
     } );
     this.addChild( readoutPanel );

js/common/view/MassDecorationLayer.ts

@@ -1,7 +1,6 @@
 // Copyright 2024, University of Colorado Boulder
 
 /**
- *
  * A containing Node for all decoration layers that are set on top of the THREE stage Node. This pattern was developed
  * because it is much easier to super-impose scenery content onto THREE graphics in an overlay, than it is to try to
  * re-render the scenery content into the THREE state. See https://github.com/phetsims/density-buoyancy-common/issues/112

js/common/view/MassLabelNode.ts

@@ -22,7 +22,6 @@ import Utils from '../../../../dot/js/Utils.js';
 
 export default class MassLabelNode extends Node {
 
-  private readonly mass: Mass;
   private readonly showMassValuesProperty: TReadOnlyProperty<boolean>;
   private readonly showMassesListener: ( n: boolean ) => void;
   private readonly readoutStringProperty: TReadOnlyProperty<string>;
@@ -65,7 +64,6 @@ export default class MassLabelNode extends Node {
 
     this.addChild( readoutPanel );
 
-    this.mass = mass;
     this.showMassValuesProperty = showMassValuesProperty;
 
     // Keep it centered

js/common/view/MassTagNode.ts

@@ -32,7 +32,6 @@ export default class MassTagNode extends Node {
   public constructor( massTag: MassTag, maxTextWidth = 100 ) {
 
     assert && assert( massTag !== MassTag.NONE, 'MassTagNode must have a provided MassTag' );
-
     assert && assert( !massTag.nameProperty.isDisposed, 'do not dispose a nameProperty' );
 
     const visibleProperty = new DerivedProperty( [ massTag.nameProperty ], name => name.length > 0 );
@@ -68,7 +67,6 @@ export default class MassTagNode extends Node {
       visibleProperty.dispose();
       backgroundNode.dispose();
     } );
-
   }
 }
 

js/common/view/MassView.ts

@@ -28,7 +28,6 @@ const INVERT_Y_TRANSFORM = ModelViewTransform2.createSinglePointScaleInvertedYMa
 
 export default abstract class MassView extends Disposable {
 
-  public readonly mass: Mass;
   public readonly massMesh: MassThreeMesh;
 
   private readonly positionListener: () => void;
@@ -39,15 +38,14 @@ export default abstract class MassView extends Disposable {
   public readonly focusablePath: Path | null;
   public readonly focusableShapeProperty = new Property( new Shape() );
 
-  protected constructor( mass: Mass, initialGeometry: THREE.BufferGeometry,
+  protected constructor( public readonly mass: Mass,
+                         initialGeometry: THREE.BufferGeometry,
                          protected readonly modelViewTransform: THREEModelViewTransform ) {
 
     super();
 
-    this.mass = mass;
     this.massMesh = new MassThreeMesh( mass, initialGeometry );
 
-
     const repositionMassTagNode = () => {
       assert && assert( this.massTagNode, 'do not reposition massTagNode if you do not have a massTag' );
       this.massTagNode!.translation = modelViewTransform.modelToViewPoint( mass.matrix.translation.toVector3().plus( this.tagOffsetProperty.value ).plusXYZ( 0, 0, 0.0001 ) );

js/common/view/MaterialControlNode.ts

@@ -47,7 +47,6 @@ export default class MaterialControlNode extends VBox {
                       listParent: Node,
                       providedOptions: MaterialControlNodeOptions ) {
 
-
     const options = optionize<MaterialControlNodeOptions, SelfMaterialControlNodeOptions, VBoxOptions>()( {
       syncCustomMaterialDensity: true,
       ownsCustomDensityRange: true,

js/common/view/MaterialMassVolumeControlNode.ts

@@ -81,6 +81,8 @@ export default class MaterialMassVolumeControlNode extends MaterialControlNode {
                       materials: Material[],
                       setVolume: ( volume: number ) => void,
                       listParent: Node,
+
+                      // TODO: Remove unused, see https://github.com/phetsims/density-buoyancy-common/issues/123
                       numberControlMassPropertyFeatured: boolean,
                       providedOptions: MaterialMassVolumeControlNodeOptions ) {
 
@@ -105,6 +107,7 @@ export default class MaterialMassVolumeControlNode extends MaterialControlNode {
     assert && assert( options.minVolumeLiters <= options.minCustomVolumeLiters, 'This seems to be a requirement, I hope you never hit this' );
 
     // If we will be creating a high density mass NumberControl in addition to the normal one.
+    // TODO: Change from !! to !== null ? Since it has different semantics for 0 which is numeric but falsy. See https://github.com/phetsims/density-buoyancy-common/issues/123
     const supportTwoMassNumberControls = !!options.highDensityMaxMass;
 
     // Mass-related elements should not be instrumented if showing as a Density control instead of Mass control.
@@ -175,6 +178,7 @@ export default class MaterialMassVolumeControlNode extends MaterialControlNode {
       units: 'L'
     } );
 
+    // TODO: Document the purpose of this lazyLink, see https://github.com/phetsims/density-buoyancy-common/issues/257
     // This instance lives for the lifetime of the simulation, so we don't need to remove this listener
     numberControlVolumeProperty.lazyLink( liters => {
       if ( !modelVolumeChanging && !userMassChanging ) {
@@ -192,12 +196,13 @@ export default class MaterialMassVolumeControlNode extends MaterialControlNode {
       }
     } );
 
+    // TODO: Document the purpose of this lazyLink, see https://github.com/phetsims/density-buoyancy-common/issues/257
     // This instance lives for the lifetime of the simulation, so we don't need to remove this listener
     volumeProperty.lazyLink( cubicMeters => {
       if ( !userVolumeChanging ) {
         modelVolumeChanging = true;
 
-        // If the value is close to min/max, massage it to the exact value, see https://github.com/phetsims/density/issues/46
+        // If the value is close to min/max, set it to the exact min/max, see https://github.com/phetsims/density/issues/46
         let volumeLiters = cubicMeters * LITERS_IN_CUBIC_METER;
         if ( volumeLiters > options.minVolumeLiters && volumeLiters < options.minVolumeLiters + 1e-10 ) {
           volumeLiters = options.minVolumeLiters;
@@ -212,6 +217,7 @@ export default class MaterialMassVolumeControlNode extends MaterialControlNode {
       }
     } );
 
+    // TODO: Document the purpose of this lazyLink, see https://github.com/phetsims/density-buoyancy-common/issues/257
     // This instance lives for the lifetime of the simulation, so we don't need to remove this listener
     numberControlMassProperty.lazyLink( mass => {
       if ( !modelMassChanging && !userVolumeChanging ) {
@@ -231,6 +237,7 @@ export default class MaterialMassVolumeControlNode extends MaterialControlNode {
       }
     } );
 
+    // TODO: Document the purpose of this lazyLink, see https://github.com/phetsims/density-buoyancy-common/issues/257
     // This instance lives for the lifetime of the simulation, so we don't need to remove this listener
     massProperty.lazyLink( mass => {
       if ( !userMassChanging ) {
@@ -479,6 +486,8 @@ const getMassReadoutLayoutFunction = ( normalLayoutFunction: LayoutFunction ) =>
   return ( titleNode: Node, numberDisplay: NumberDisplay, slider: Slider, decrementButton: ArrowButton | null, incrementButton: ArrowButton | null ) => {
     const tempNode = normalLayoutFunction( titleNode, numberDisplay, slider, decrementButton, incrementButton );
     const width = tempNode.width;
+
+    // TODO: Safe to dispose the parent before detaching the children? See https://github.com/phetsims/density-buoyancy-common/issues/123
     tempNode.dispose();
     titleNode.detach();
     numberDisplay.detach();