From ea5927f441f9cfb3f1166f470896455832d22e1d Mon Sep 17 00:00:00 2001 From: pixelzoom Date: Mon, 4 Mar 2024 12:34:21 -0700 Subject: [PATCH] REVIEW comments, https://github.com/phetsims/projectile-data-lab/issues/32 --- js/common/PDLConstants.ts | 1 + js/common/PDLUtils.ts | 4 ++++ js/common/view/LaunchButton.ts | 1 + js/common/view/LauncherIconNode.ts | 3 ++- js/common/view/LauncherNode.ts | 6 ++++++ js/common/view/MysteryLauncherIcon.ts | 2 +- js/common/view/MysteryLauncherRadioButtonGroupWrapper.ts | 2 ++ js/common/view/PDLCanvasNode.ts | 3 +++ js/common/view/PDLLaunchPanel.ts | 1 + js/common/view/PDLPanelSection.ts | 4 ++-- js/common/view/PDLRectangularRadioButtonGroup.ts | 4 ++-- js/common/view/PDLScreenView.ts | 3 ++- 12 files changed, 27 insertions(+), 7 deletions(-) diff --git a/js/common/PDLConstants.ts b/js/common/PDLConstants.ts index 46423f3a..4f6272bf 100644 --- a/js/common/PDLConstants.ts +++ b/js/common/PDLConstants.ts @@ -11,6 +11,7 @@ import projectileDataLab from '../projectileDataLab.js'; import PhetFont from '../../../scenery-phet/js/PhetFont.js'; import { BooleanProperty } from '../../../axon/js/imports.js'; +//REVIEW Constants should be uppercase. const maxFieldDistance = 100; const fieldWidth = 850; const pixelsToDistance = fieldWidth / maxFieldDistance; diff --git a/js/common/PDLUtils.ts b/js/common/PDLUtils.ts index 739477b7..c2465e5e 100644 --- a/js/common/PDLUtils.ts +++ b/js/common/PDLUtils.ts @@ -15,6 +15,8 @@ import { VSMFieldIdentifierValues } from '../common-vsm/model/VSMFieldIdentifier import { SAMPLE_SIZES } from '../sampling/model/SamplingModel.js'; export default class PDLUtils { + + //REVIEW document public static transformField( point: Vector2 ): Vector2 { const pointY = point.y + PDLConstants.FIELD_SCALING_FACTOR_VERTICAL * Math.abs( point.y ); const horizontalDistanceFactor = point.x / ( 0.5 * PDLConstants.FIELD_WIDTH ); @@ -24,11 +26,13 @@ export default class PDLUtils { pointY ); } + //REVIEW document public static colorForFieldIndex( index: number ): Color { return PDLColors.fieldLightFillProperty.value.blend( PDLColors.fieldDarkFillProperty.value, index / ( VSMFieldIdentifierValues.length - 1 ) ); } + //REVIEW document public static colorForSampleSize( sampleSize: number ): Color { const index = SAMPLE_SIZES.indexOf( sampleSize ); diff --git a/js/common/view/LaunchButton.ts b/js/common/view/LaunchButton.ts index cde35a1a..bf3b289a 100644 --- a/js/common/view/LaunchButton.ts +++ b/js/common/view/LaunchButton.ts @@ -1,5 +1,6 @@ // Copyright 2024, University of Colorado Boulder +//REVIEW This class feels under-documented. There's a lot going on here with how this button changes based on Properties. /** * Launch button for the projectile data lab. * diff --git a/js/common/view/LauncherIconNode.ts b/js/common/view/LauncherIconNode.ts index 6e6fbeae..87d522dc 100644 --- a/js/common/view/LauncherIconNode.ts +++ b/js/common/view/LauncherIconNode.ts @@ -1,5 +1,6 @@ // Copyright 2024, University of Colorado Boulder +//REVIEW This doc looks seems sort of incorrect, looks like it was copied from LauncherNode and sort of changed. /** * The LauncherIconNode is the visual representation of the projectile launcher used in the UI of Projectile Data Lab. * @@ -25,7 +26,7 @@ type LauncherIconNodeOptions = SelfOptions & NodeOptions; export default class LauncherIconNode extends Node { - // Create adapters for that will reflect the values for the selected launcher. + // Create adapters that will reflect the values for the selected launcher. private launcherConfigurationProperty = new Property( 'angle30' ); private launchAngleProperty = new NumberProperty( 30 ); private launchHeightProperty = new NumberProperty( 0 ); diff --git a/js/common/view/LauncherNode.ts b/js/common/view/LauncherNode.ts index c51f846f..5bda3d87 100644 --- a/js/common/view/LauncherNode.ts +++ b/js/common/view/LauncherNode.ts @@ -199,6 +199,7 @@ export default class LauncherNode extends Node { this.y = this.modelViewTransform.modelToViewY( height ); } + //REVIEW Unnecessary method. No members are accessed, could be moved outside class definition. private getCachedDarkerColorProperty( colorProperty: TReadOnlyProperty ): TReadOnlyProperty { if ( !darkerColorCacheMap.has( colorProperty ) ) { darkerColorCacheMap.set( colorProperty, new DerivedProperty( [ colorProperty ], color => color.darkerColor( 0.8 ) ) ); @@ -206,6 +207,7 @@ export default class LauncherNode extends Node { return darkerColorCacheMap.get( colorProperty )!; } + //REVIEW document private launcherBarrelGraphicsForType( mysteryLauncherNumber: number, isIcon: boolean ): Node[] { const barrelColorProperty = PDLColors.mysteryLauncherColorProfiles[ mysteryLauncherNumber - 1 ].barrelFillProperty; const barrelDarkColorProperty = this.getCachedDarkerColorProperty( barrelColorProperty ); @@ -282,6 +284,7 @@ export default class LauncherNode extends Node { return [ barrel, ...( patternImage ? [ patternImage ] : [] ), barrelBorder, ...( isIcon ? [] : [ this.labelNode ] ), launcherEndRect ]; } + //REVIEW document private launcherFrameBackGraphicsForType( mysteryLauncher: number, isIcon: boolean ): Node[] { const frameBackground = new Path( this.guideRailInnerShape(), { fill: PDLColors.launcherBackFillProperty @@ -331,6 +334,7 @@ export default class LauncherNode extends Node { return [ supportBar, frameBackground, frameBarTop, frameBarBottom ]; } + //REVIEW document protected launcherFrameFrontGraphicsForType( mysteryLauncher: number, outerRadiusCutoff = 0 ): Node[] { const fillColorProperty = PDLColors.mysteryLauncherColorProfiles[ mysteryLauncher - 1 ].frameFillProperty; @@ -354,11 +358,13 @@ export default class LauncherNode extends Node { return [ guideRail ]; } + //REVIEW Unnecessary method. No members are accessed, could be moved outside class definition. private guideRailInnerShape(): Shape { return new Shape().arc( 0, 0, GUIDE_RAIL_INNER_RADIUS, GUIDE_RAIL_MIN_ANGLE, GUIDE_RAIL_MAX_ANGLE ) .lineTo( 0, 0 ).close(); } + //REVIEW Unnecessary method. No members are accessed, could be moved outside class definition. private guideRailOuterShape( outerRadiusCutoff = 0 ): Shape { return new Shape().arc( 0, 0, GUIDE_RAIL_OUTER_RADIUS - outerRadiusCutoff, GUIDE_RAIL_MIN_ANGLE, GUIDE_RAIL_MAX_ANGLE ) .lineTo( 0, 0 ).close(); diff --git a/js/common/view/MysteryLauncherIcon.ts b/js/common/view/MysteryLauncherIcon.ts index 5aa4b01a..976d9331 100644 --- a/js/common/view/MysteryLauncherIcon.ts +++ b/js/common/view/MysteryLauncherIcon.ts @@ -19,7 +19,7 @@ export class MysteryLauncherIcon extends Node { const mysteryLauncherIcon = new LauncherNode( ModelViewTransform2.createIdentity(), new Property( 45 ), new Property( 0 ), new Property( mysteryLauncher ), null, { isIcon: true } ).rasterized( { - resolution: 2.22 + resolution: 2.22 //REVIEW NodeOptions.resolution is undocumented, this was new to me, so I can't grok 2.22. } ); super( { children: [ mysteryLauncherIcon ], diff --git a/js/common/view/MysteryLauncherRadioButtonGroupWrapper.ts b/js/common/view/MysteryLauncherRadioButtonGroupWrapper.ts index 746d8023..1e1d5472 100644 --- a/js/common/view/MysteryLauncherRadioButtonGroupWrapper.ts +++ b/js/common/view/MysteryLauncherRadioButtonGroupWrapper.ts @@ -1,5 +1,7 @@ // Copyright 2023-2024, University of Colorado Boulder +//REVIEW MysteryLauncherControl might be a better name for this. +//REVIEW Not clear how you achieved a grid layout, which is presumably the reason for this 'wrapper', elaborate with comments. /** * MysteryLauncherRadioButtonGroupWrapper is a control that shows the radio buttons that choose between the different * mystery launchers. A wrapper is needed to help the layout wrap. diff --git a/js/common/view/PDLCanvasNode.ts b/js/common/view/PDLCanvasNode.ts index 9a73be50..98f368bf 100644 --- a/js/common/view/PDLCanvasNode.ts +++ b/js/common/view/PDLCanvasNode.ts @@ -77,6 +77,7 @@ export default abstract class PDLCanvasNode extends CanvasNode this.modelViewTransform = modelViewTransform; } + //REVIEW document protected drawOutlierGraphicsForLandedProjectiles( landedProjectiles: Projectile[], context: CanvasRenderingContext2D ): void { // Force field graphics for landed outliers @@ -92,6 +93,7 @@ export default abstract class PDLCanvasNode extends CanvasNode } ); } + //REVIEW document protected drawPathForProjectile( context: CanvasRenderingContext2D, projectile: Projectile ): void { context.beginPath(); let pathStarted = false; @@ -117,6 +119,7 @@ export default abstract class PDLCanvasNode extends CanvasNode context.stroke(); } + //REVIEW document protected drawProjectile( context: CanvasRenderingContext2D, projectile: Projectile, isLanded: boolean, isSelected: boolean ): void { const viewPoint = this.modelViewTransform.modelToViewXY( projectile.x, projectile.y ); diff --git a/js/common/view/PDLLaunchPanel.ts b/js/common/view/PDLLaunchPanel.ts index cb4a3f3c..d5f94c32 100644 --- a/js/common/view/PDLLaunchPanel.ts +++ b/js/common/view/PDLLaunchPanel.ts @@ -1,5 +1,6 @@ // Copyright 2023-2024, University of Colorado Boulder +//REVIEW Incorrect doc, copy-pasted from PDLPanel. /** * The PDLPanel defines the default look and feel for panels in the Projectile Data Lab. * diff --git a/js/common/view/PDLPanelSection.ts b/js/common/view/PDLPanelSection.ts index 69014063..19514800 100644 --- a/js/common/view/PDLPanelSection.ts +++ b/js/common/view/PDLPanelSection.ts @@ -1,8 +1,8 @@ // Copyright 2023-2024, University of Colorado Boulder /** - * Shows a title and content in a VBox. For the first 3 screens, the content is rectangular radio buttons. - * For the Sampling screen, there are other content types. + * PDLPanelSection is a base class for showing a title and content in a VBox. For the first 3 screens, the content is + * rectangular radio buttons. For the Sampling screen, there are other content types. * * @author Matthew Blackman (PhET Interactive Simulations) * @author Sam Reid (PhET Interactive Simulations) diff --git a/js/common/view/PDLRectangularRadioButtonGroup.ts b/js/common/view/PDLRectangularRadioButtonGroup.ts index 540a7d99..abb44de7 100644 --- a/js/common/view/PDLRectangularRadioButtonGroup.ts +++ b/js/common/view/PDLRectangularRadioButtonGroup.ts @@ -1,8 +1,8 @@ // Copyright 2024, University of Colorado Boulder /** - * PDLRectangularRadioButtonGroup is a RectangularRadioButtonGroup with a default look that is used in the majority of radio buttons - * within Projectile Data Lab. + * PDLRectangularRadioButtonGroup is a RectangularRadioButtonGroup with a default look that is used in the majority of + * radio buttons within Projectile Data Lab. * * @author Sam Reid (PhET Interactive Simulations) */ diff --git a/js/common/view/PDLScreenView.ts b/js/common/view/PDLScreenView.ts index 9f17d378..573bad2b 100644 --- a/js/common/view/PDLScreenView.ts +++ b/js/common/view/PDLScreenView.ts @@ -45,8 +45,9 @@ export type PDLScreenViewOptions = SelfOptions & WithRequired extends ScreenView { protected readonly modelViewTransform; - protected readonly canvasBounds: Bounds2; + protected readonly canvasBounds: Bounds2;//REVIEW document - what canvas? + //REVIEW launcherLayer is no longer needed. Now that PDLScreenView creates launcherNode, you just put launcherNode in the correct rendering order. protected readonly launcherLayer = new Node(); protected readonly behindProjectilesLayer = new Node(); protected readonly projectileLayer = new Node();