phetsims / keplers-laws

"Kepler's Laws" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 1 forks source link

The position and velocity of the sun should be read-only #229

Closed arouinfar closed 9 months ago

arouinfar commented 10 months ago

Discovered when creating examples.md for #223

The positionProperty and velocityProperty of the sun is currently settable in Studio, but doesn't appear to do anything. The simulation was not designed to support a moving or repositioned sun and there is no reason for clients to edit these values.

Please make model.bodies.sun.positionProperty and model.bodies.sun.velocityProperty read-only.

arouinfar commented 10 months ago

On a similar note, there doesn't seem to be any need for view.orbitalSystemNodes.sunNode.inputEnabledProperty. Can we uninstrument it?

pixelzoom commented 10 months ago

Please make model.bodies.sun.positionProperty and model.bodies.sun.velocityProperty read-only.

'sun' is described in KeplersLawsModel.ts at line 150:

        new BodyInfo( {
          isActive: true,
          mass: KeplersLawsConstants.MASS_OF_OUR_SUN,
          massRange: new Range( 0.5 * KeplersLawsConstants.MASS_OF_OUR_SUN, 2 * KeplersLawsConstants.MASS_OF_OUR_SUN ),
          position: new Vector2( 0, 0 ),
          velocity: new Vector2( 0, 0 ),
          tandemName: 'sun'
        } ),

BodyInfo is used elsewhere to instantiate the Body instances that have positionProperty and velocityProperty. Unfortunately, BodyInfo does not support describing the PhET-iO metadata for those Properties. I was going to suggest adding something like this to BodyInfoOptions:

 positionPropertyOptions?: Vector2PropertyOptions;
 velocityPropertyOptions?: Vector2PropertyOptions;

... but that is likely to cause serious problems with BodyInfoIO and PhET-iO serialization.

Let me think on this some more.

pixelzoom commented 10 months ago

On a similar note, there doesn't seem to be any need for view.orbitalSystemNodes.sunNode.inputEnabledProperty. Can we uninstrument it?

This one is easier, but I don't have time to commit or test. In KeplersLawsScreenView:

    const sunNode = new BodyNode( sun, this.modelViewTransformProperty, {
      draggable: false,
      focusable: false,
      pickable: false,
      tandem: orbitalSystemNodesTandem.createTandem( 'sunNode' ),
+     phetioInputEnabledPropertyInstrumented: false
    } );
    this.bodiesLayer.addChild( sunNode );
pixelzoom commented 10 months ago

Please make model.bodies.sun.positionProperty and model.bodies.sun.velocityProperty read-only.

Here's a patch that I've tested lightly.

Subject: [PATCH] fixes for author-annotation lint rule, https://github.com/phetsims/chipper/issues/1414
---
Index: solar-system-common/js/model/BodyInfo.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/solar-system-common/js/model/BodyInfo.ts b/solar-system-common/js/model/BodyInfo.ts
--- a/solar-system-common/js/model/BodyInfo.ts  (revision 4b7f726dcbafda373f71e901ccc42b1b8bfec498)
+++ b/solar-system-common/js/model/BodyInfo.ts  (date 1704475106966)
@@ -16,13 +16,16 @@
 import NullableIO from '../../../tandem/js/types/NullableIO.js';
 import StringIO from '../../../tandem/js/types/StringIO.js';
 import SolarSystemCommonConstants from '../SolarSystemCommonConstants.js';
+import { Vector2PropertyOptions } from '../../../dot/js/Vector2Property.js';

 type SelfOptions = {
   isActive: boolean;
   mass: number;
   massRange?: Range;
   position: Vector2;
+  positionPropertyOptions?: Vector2PropertyOptions; // no PhET-iO serialization needed because all Body instances are created at startup
   velocity: Vector2;
+  velocityPropertyOptions?: Vector2PropertyOptions; // no PhET-iO serialization needed because all Body instances are created at startup
   tandemName?: string | null;
 };

@@ -44,7 +47,9 @@
   public readonly mass: number;
   public readonly massRange: Range;
   public readonly position: Vector2;
+  public readonly positionPropertyOptions?: Vector2PropertyOptions;
   public readonly velocity: Vector2;
+  public readonly velocityPropertyOptions?: Vector2PropertyOptions;
   public readonly tandemName: string | null;

   public constructor( providedOptions: BodyInfoOptions ) {
@@ -52,8 +57,9 @@
     this.mass = providedOptions.mass;
     this.massRange = providedOptions.massRange || SolarSystemCommonConstants.DEFAULT_MASS_RANGE;
     this.position = providedOptions.position;
+    this.positionPropertyOptions = providedOptions.positionPropertyOptions;
     this.velocity = providedOptions.velocity;
-    this.velocity = providedOptions.velocity;
+    this.velocityPropertyOptions = providedOptions.velocityPropertyOptions;
     this.tandemName = providedOptions.tandemName || null;
   }

Index: keplers-laws/js/common/model/KeplersLawsModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/keplers-laws/js/common/model/KeplersLawsModel.ts b/keplers-laws/js/common/model/KeplersLawsModel.ts
--- a/keplers-laws/js/common/model/KeplersLawsModel.ts  (revision c477e6b7c3a23aaa311ff2e5d50c68e30e713037)
+++ b/keplers-laws/js/common/model/KeplersLawsModel.ts  (date 1704475671072)
@@ -152,7 +152,15 @@
           mass: KeplersLawsConstants.MASS_OF_OUR_SUN,
           massRange: new Range( 0.5 * KeplersLawsConstants.MASS_OF_OUR_SUN, 2 * KeplersLawsConstants.MASS_OF_OUR_SUN ),
           position: new Vector2( 0, 0 ),
+          positionPropertyOptions: {
+            phetioReadOnly: true,
+            phetioFeatured: false
+          },
           velocity: new Vector2( 0, 0 ),
+          velocityPropertyOptions: {
+            phetioReadOnly: true,
+            phetioFeatured: false
+          },
           tandemName: 'sun'
         } ),
         new BodyInfo( {
Index: solar-system-common/js/model/Body.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/solar-system-common/js/model/Body.ts b/solar-system-common/js/model/Body.ts
--- a/solar-system-common/js/model/Body.ts  (revision 4b7f726dcbafda373f71e901ccc42b1b8bfec498)
+++ b/solar-system-common/js/model/Body.ts  (date 1704474817529)
@@ -11,7 +11,7 @@
 import solarSystemCommon from '../solarSystemCommon.js';
 import NumberProperty from '../../../axon/js/NumberProperty.js';
 import BooleanProperty from '../../../axon/js/BooleanProperty.js';
-import Vector2Property from '../../../dot/js/Vector2Property.js';
+import Vector2Property, { Vector2PropertyOptions } from '../../../dot/js/Vector2Property.js';
 import TReadOnlyProperty from '../../../axon/js/TReadOnlyProperty.js';
 import { Color } from '../../../scenery/js/imports.js';
 import TinyEmitter from '../../../axon/js/TinyEmitter.js';
@@ -26,6 +26,7 @@
 import SolarSystemCommonConstants from '../SolarSystemCommonConstants.js';
 import NumberIO from '../../../tandem/js/types/NumberIO.js';
 import isSettingPhetioStateProperty from '../../../tandem/js/isSettingPhetioStateProperty.js';
+import { combineOptions } from '../../../phet-core/js/optionize.js';

 export type BodyStateObject = ReferenceIOState; // because BodyIO is a subtype of ReferenceIO

@@ -90,20 +91,22 @@
     this.radiusProperty = new DerivedProperty( [ this.massProperty ], mass => Body.massToRadius( mass ) );

     // bodyInfo.position.copy() because velocityProperty's value may be mutated directly
-    this.positionProperty = new Vector2Property( bodyInfo.position.copy(), {
-      units: 'AU',
-      tandem: tandem.createTandem( 'positionProperty' ),
-      phetioFeatured: true,
-      reentrant: true
-    } );
+    this.positionProperty = new Vector2Property( bodyInfo.position.copy(),
+      combineOptions<Vector2PropertyOptions>( {
+        units: 'AU',
+        tandem: tandem.createTandem( 'positionProperty' ),
+        phetioFeatured: true,
+        reentrant: true
+      }, bodyInfo.positionPropertyOptions ) );

     // bodyInfo.velocity.copy() because velocityProperty's value may be mutated directly
-    this.velocityProperty = new Vector2Property( bodyInfo.velocity.copy(), {
+    this.velocityProperty = new Vector2Property( bodyInfo.velocity.copy(),
+      combineOptions<Vector2PropertyOptions>( {
       units: 'km/s',
       tandem: tandem.createTandem( 'velocityProperty' ),
       phetioFeatured: true,
       reentrant: true
-    } );
+    }, bodyInfo.velocityPropertyOptions ) );

     this.speedProperty = new DerivedProperty( [ this.velocityProperty ], velocity => velocity.magnitude, {
       units: 'km/s',
AgustinVallejo commented 10 months ago

Applied changes from the patch. Since this involves SSC, we have to Cherry Pick this into MSS-RC