diff --git a/js/common/DensityBuoyancyCommonConstants.ts b/js/common/DensityBuoyancyCommonConstants.ts index a1809972..e2d4ed64 100644 --- a/js/common/DensityBuoyancyCommonConstants.ts +++ b/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 = { diff --git a/js/common/view/ABControlsNode.ts b/js/common/view/ABControlsNode.ts index f1447df7..b6b145bf 100644 --- a/js/common/view/ABControlsNode.ts +++ b/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( { labelNode: ABPanelsNode.getTagALabelNode(), diff --git a/js/common/view/BackgroundEventTargetListener.ts b/js/common/view/BackgroundEventTargetListener.ts index 617832ac..9feb292d 100644 --- a/js/common/view/BackgroundEventTargetListener.ts +++ b/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, diff --git a/js/common/view/BlocksModeRadioButtonGroup.ts b/js/common/view/BlocksModeRadioButtonGroup.ts index 750ef112..25b3cf88 100644 --- a/js/common/view/BlocksModeRadioButtonGroup.ts +++ b/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 ); diff --git a/js/common/view/BlocksPanel.ts b/js/common/view/BlocksPanel.ts index 9930f94f..efde5a63 100644 --- a/js/common/view/BlocksPanel.ts +++ b/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 ) ); - } } diff --git a/js/common/view/ComboNumberControl.ts b/js/common/view/ComboNumberControl.ts index 071baad2..eca5488b 100644 --- a/js/common/view/ComboNumberControl.ts +++ b/js/common/view/ComboNumberControl.ts @@ -38,6 +38,7 @@ type SelfOptions = { comboItems: ComboBoxItem[]; + // 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; diff --git a/js/common/view/ComparisonNumberControl.ts b/js/common/view/ComparisonNumberControl.ts index 933aa429..62b769d6 100644 --- a/js/common/view/ComparisonNumberControl.ts +++ b/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) */ diff --git a/js/common/view/DensityBuoyancyScreenView.ts b/js/common/view/DensityBuoyancyScreenView.ts index 5853a5df..90b90e9d 100644 --- a/js/common/view/DensityBuoyancyScreenView.ts +++ b/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>; protected readonly rightBarrierViewPointPropertyProperty: Property>; @@ -154,6 +157,7 @@ export default class DensityBuoyancyScreenView this.localToGlobalPoint( this.modelToViewPoint( point ) ), + point => this.localToGlobalPoint( this.modelToViewPoint( point ) ), updateCursor, this.tandem.createTandem( 'backgroundEventTargetListener' ) ); @@ -350,7 +354,7 @@ export default class DensityBuoyancyScreenView { let index = 0; @@ -503,7 +508,6 @@ export default class DensityBuoyancyScreenView { - // Mass view const massView = _.find( this.massViews, massView => massView.mass === mass )!; // Remove the mass view @@ -515,6 +519,8 @@ export default class DensityBuoyancyScreenView { const modelPoint = new Vector3( model.poolBounds.minX, fluidY, model.poolBounds.maxZ ); @@ -831,7 +837,6 @@ export default class DensityBuoyancyScreenView 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 = () => { diff --git a/js/common/view/FluidLevelIndicator.ts b/js/common/view/FluidLevelIndicator.ts index 6164a8e4..f651f741 100644 --- a/js/common/view/FluidLevelIndicator.ts +++ b/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,11 +28,9 @@ 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 } ), @@ -41,7 +38,6 @@ export default class FluidLevelIndicator extends Node { } ); const readoutPanel = new Panel( readoutText, { - // Not using the typical margins for UI panels in this sim cornerRadius: DensityBuoyancyCommonConstants.CORNER_RADIUS } ); this.addChild( readoutPanel ); diff --git a/js/common/view/LabelTexture.ts b/js/common/view/LabelTexture.ts deleted file mode 100644 index cf45aeeb..00000000 --- a/js/common/view/LabelTexture.ts +++ /dev/null @@ -1,40 +0,0 @@ -// Copyright 2020-2024, University of Colorado Boulder - -/** - * A wrapper for NodeTexture that scales it up and uses power-of-2 dimensions. - * - * @author Jonathan Olson (PhET Interactive Simulations) - */ - -import NodeTexture from '../../../../mobius/js/NodeTexture.js'; -import { Node } from '../../../../scenery/js/imports.js'; -import densityBuoyancyCommon from '../../densityBuoyancyCommon.js'; - -export default class LabelTexture extends NodeTexture { - - private readonly containerNode: Node; - - public constructor( labelNode: Node ) { - const containerNode = new Node( { - children: [ labelNode ], - scale: 2 - } ); - - super( containerNode, { - calculateDimensionFromNode: true - } ); - - this.containerNode = containerNode; - } - - /** - * Releases references - */ - public override dispose(): void { - this.containerNode.dispose(); - - super.dispose(); - } -} - -densityBuoyancyCommon.register( 'LabelTexture', LabelTexture ); \ No newline at end of file diff --git a/js/common/view/MassDecorationLayer.ts b/js/common/view/MassDecorationLayer.ts index 60116ce1..9b0992b3 100644 --- a/js/common/view/MassDecorationLayer.ts +++ b/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 diff --git a/js/common/view/MassLabelNode.ts b/js/common/view/MassLabelNode.ts index cd910af5..b7937e4e 100644 --- a/js/common/view/MassLabelNode.ts +++ b/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; private readonly showMassesListener: ( n: boolean ) => void; private readonly readoutStringProperty: TReadOnlyProperty; @@ -65,7 +64,6 @@ export default class MassLabelNode extends Node { this.addChild( readoutPanel ); - this.mass = mass; this.showMassValuesProperty = showMassValuesProperty; // Keep it centered diff --git a/js/common/view/MassTagNode.ts b/js/common/view/MassTagNode.ts index 950fdd30..b3dc8606 100644 --- a/js/common/view/MassTagNode.ts +++ b/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(); } ); - } } diff --git a/js/common/view/MassView.ts b/js/common/view/MassView.ts index 711ff6f9..1f5024b5 100644 --- a/js/common/view/MassView.ts +++ b/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 ) ); diff --git a/js/common/view/MaterialControlNode.ts b/js/common/view/MaterialControlNode.ts index 2d3f6aeb..36898f41 100644 --- a/js/common/view/MaterialControlNode.ts +++ b/js/common/view/MaterialControlNode.ts @@ -47,7 +47,6 @@ export default class MaterialControlNode extends VBox { listParent: Node, providedOptions: MaterialControlNodeOptions ) { - const options = optionize()( { syncCustomMaterialDensity: true, ownsCustomDensityRange: true, diff --git a/js/common/view/MaterialMassVolumeControlNode.ts b/js/common/view/MaterialMassVolumeControlNode.ts index 639b1bd2..f6bc7588 100644 --- a/js/common/view/MaterialMassVolumeControlNode.ts +++ b/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(); diff --git a/js/common/view/MaterialView.ts b/js/common/view/MaterialView.ts index 542311e5..c23994dd 100644 --- a/js/common/view/MaterialView.ts +++ b/js/common/view/MaterialView.ts @@ -53,15 +53,9 @@ import densityBuoyancyCommon from '../../densityBuoyancyCommon.js'; import Material from '../model/Material.js'; import ReadOnlyProperty from '../../../../axon/js/ReadOnlyProperty.js'; -// MaterialView definition - class MaterialView { - public readonly material: T; - - public constructor( material: T ) { - this.material = material; - } + public constructor( public readonly material: T ) { } /** * Releases references @@ -71,7 +65,6 @@ class MaterialView { } } - // constants function toWrappedTexture( image: HTMLImageElement ): THREE.Texture { diff --git a/js/common/view/PoolScaleHeightControl.ts b/js/common/view/PoolScaleHeightControl.ts index 62a2ffe2..a9580462 100644 --- a/js/common/view/PoolScaleHeightControl.ts +++ b/js/common/view/PoolScaleHeightControl.ts @@ -72,6 +72,8 @@ export default class PoolScaleHeightControl extends NumberControl { // NumberControl does not support accessibleName, so we pass the tandem name of the NumberControl to the slider accessibleName: Tandem.toAccessibleName( providedOptions, 'Control' ) }, + + // TODO: Aren't these optional by default? Can we omit the OPT_OUT? See https://github.com/phetsims/density-buoyancy-common/issues/123 titleNodeOptions: { tandem: Tandem.OPT_OUT }, numberDisplayOptions: { tandem: Tandem.OPT_OUT }, delta: DEFAULT_RANGE.getLength() / 2000, diff --git a/js/common/view/PrecisionSliderThumb.ts b/js/common/view/PrecisionSliderThumb.ts index 1ef9c316..6f1210ec 100644 --- a/js/common/view/PrecisionSliderThumb.ts +++ b/js/common/view/PrecisionSliderThumb.ts @@ -62,7 +62,6 @@ export default class PrecisionSliderThumb extends Node { super( options ); - // highlight thumb on pointer over const pressListener = new PressListener( { attach: false, diff --git a/js/common/view/ScaleReadoutNode.ts b/js/common/view/ScaleReadoutNode.ts index e162138f..e3d5bee0 100644 --- a/js/common/view/ScaleReadoutNode.ts +++ b/js/common/view/ScaleReadoutNode.ts @@ -2,6 +2,7 @@ /** * Shows a readout in front of a scale, for its measured mass/weight. + * Not dependent on a Scale model instance. * * @author Jonathan Olson (PhET Interactive Simulations) */ @@ -27,7 +28,6 @@ type GeneralScaleReadoutNodeSelfOptions = { }; type GeneralScaleReadoutNodeOptions = NodeOptions & GeneralScaleReadoutNodeSelfOptions; -// Not dependent on a Scale model instance. export class GeneralScaleReadoutNode extends Node { private readonly stringProperty: TReadOnlyProperty; @@ -91,18 +91,17 @@ export class GeneralScaleReadoutNode extends Node { } } +// TODO: https://github.com/phetsims/density-buoyancy-common/issues/257 document how this differs from GeneralScaleReadoutNode +// TODO: How can a client choose which one they need? See https://github.com/phetsims/density-buoyancy-common/issues/257 export default class ScaleReadoutNode extends GeneralScaleReadoutNode { - public readonly mass: Scale; - - public constructor( mass: Scale, gravityProperty: TReadOnlyProperty ) { + // TODO: Rename mass => scale, see https://github.com/phetsims/density-buoyancy-common/issues/123 + public constructor( public readonly mass: Scale, gravityProperty: TReadOnlyProperty ) { const blendedProperty = new BlendedNumberProperty( mass.measuredWeightInterpolatedProperty.value ); mass.stepEmitter.addListener( () => blendedProperty.step( mass.measuredWeightInterpolatedProperty.value ) ); super( blendedProperty, gravityProperty, mass.displayType ); - - this.mass = mass; } } diff --git a/js/common/view/VolumeUnitsControl.ts b/js/common/view/VolumeUnitsControl.ts index 1fb1f4f6..bb1d6388 100644 --- a/js/common/view/VolumeUnitsControl.ts +++ b/js/common/view/VolumeUnitsControl.ts @@ -52,6 +52,7 @@ export default class VolumeUnitsControl extends HBox { }; } + // TODO: Are preference controls ever disposed? https://github.com/phetsims/density-buoyancy-common/issues/123 public override dispose(): void { super.dispose(); this.disposeVolumeUnitsControl(); diff --git a/js/density/view/DensityIntroScreenView.ts b/js/density/view/DensityIntroScreenView.ts index ffdf7415..ae05dbe1 100644 --- a/js/density/view/DensityIntroScreenView.ts +++ b/js/density/view/DensityIntroScreenView.ts @@ -71,7 +71,6 @@ export default class DensityIntroScreenView extends DensityBuoyancyScreenView { + // We might not have a box, see https://github.com/phetsims/density/issues/110 return new Vector2( isFinite( boxBounds.left ) ? boxBounds.left : visibleBounds.right, visibleBounds.centerY ); } ); @@ -132,6 +132,7 @@ export default class DensityIntroScreenView extends DensityBuoyancyScreenView number; - private markerNodes: Node[] = []; public constructor( providedOptions?: DensityNumberLineNodeOptions ) { + // TODO: DensityNumberLineNode only has one instantiation site, so why are things like materials in the defaults? See https://github.com/phetsims/density-buoyancy-common/issues/123 const options = optionize()( { materials: [ Material.HUMAN, @@ -227,6 +227,8 @@ export default class DensityNumberLineNode extends Node { isHiddenProperty ], ( density, isVisible, isHidden ) => { marker.x = this.modelViewTransform( density ); + + // TODO: what is undefined + 1E-5? (Hint: it is NaN). Is that a good way to do this? See https://github.com/phetsims/density-buoyancy-common/issues/257 marker.visible = isVisible && !isHidden && density < options.maxDensity! + 1e-5; } ); } );