Setting negative semimajor axis target prevents further setting of target orbits

[KatieWoe]

Test device Dell Operating System Win 11 Browser Firefox Problem description For https://github.com/phetsims/qa/issues/1023 I noticed that, when setting a target of an orbit, it is oriented in the opposite direction of the default orbit. I'm not sure of a legitimate way of changing this. If I make the semimajor axis value negative it throws assertions/errors, and then the target orbit looks how I intended. However, any further attempts to change the target orbit fail. Steps to reproduce

1. In studio, make a target orbit visible and begin to change it's values
2. Set the eccentricity to 0.3 and semimajor axis to -1.5
3. Try to set these values
4. Remove the negative value and try to set the value again

Visuals

[pixelzoom]

@AgustinVallejo let me know if you need help with this.

[zepumph]

Here is some investigation I did in a patch. I know nothing about Kepler's Laws, but is seems like a target orbit cannot have a negative semi major axis. I wasn't sure about if it is allowed to be zero. With the new approach to validation in the patch, here is the error I get in studio (non fatal this time) when trying to set a negative semi major axis. It is challenging to internalize how Property validation will be client-facing for cases like these, but validationMessage can help communicate things. I also recommend adding the phetioDocumentation about the supported values of a target orbit.

I know that in the past we have differed on opinion about how thoroughly to add assertions, but in my opinion the assertions added in the patch (if correct) are no-brainers. I don't like assuming that values passed in are as expected, especially when they can be provided by phet-io clients or coworkers untrained in astronomical physics (me). The "Should be a finite Vector2" was about 4 steps downstream of the actual problem. Let me know if you want to discuss further.

```diff Subject: [PATCH] fdsafd --- Index: js/common/view/ThirdLawGraph.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/ThirdLawGraph.ts b/js/common/view/ThirdLawGraph.ts --- a/js/common/view/ThirdLawGraph.ts (revision 4c7a5d1507146c3ab5af2271429f9cdca1cf71f5) +++ b/js/common/view/ThirdLawGraph.ts (date 1706724648363) @@ -50,7 +50,7 @@ const periodPower = model.selectedPeriodPowerProperty.value; const axisPower = model.selectedAxisPowerProperty.value; - return new Vector2( + const vector2 = new Vector2( axisLength * Math.pow( Utils.linear( 0, maxSemiMajorAxis, 0, 1, @@ -61,6 +61,8 @@ period ), periodPower ) ); + assert && assert( vector2.isFinite(), `should be a finite position given axis: ${semiMajorAxis}` ); + return vector2; }; const xAxis = new ArrowNode( 0, 0, axisLength, 0, { Index: js/common/view/TargetOrbitComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/TargetOrbitComboBox.ts b/js/common/view/TargetOrbitComboBox.ts --- a/js/common/view/TargetOrbitComboBox.ts (revision 4c7a5d1507146c3ab5af2271429f9cdca1cf71f5) +++ b/js/common/view/TargetOrbitComboBox.ts (date 1706725253422) @@ -52,7 +52,7 @@ maxWidth: 120 } ), comboBoxListItemNodeOptions: { - visible: !isPhetioConfigurable + visible: true // TODO: Just for easier testing, do not commit }, a11yName: nameProperty, tandemName: tandemName Index: js/common/model/TargetOrbitInfoProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/TargetOrbitInfoProperty.ts b/js/common/model/TargetOrbitInfoProperty.ts --- a/js/common/model/TargetOrbitInfoProperty.ts (revision 4c7a5d1507146c3ab5af2271429f9cdca1cf71f5) +++ b/js/common/model/TargetOrbitInfoProperty.ts (date 1706725033172) @@ -71,7 +71,13 @@ public constructor( targetOrbit: TargetOrbit, tandem: Tandem ) { super( new TargetOrbitInfo( targetOrbit.eccentricity, targetOrbit.semiMajorAxis ), { - isValidValue: targetOrbitInfo => ( targetOrbitInfo.eccentricity >= 0 && targetOrbitInfo.eccentricity < 1 ), + validators: [ { + isValidValue: targetOrbitInfo => ( targetOrbitInfo.eccentricity >= 0 && + targetOrbitInfo.eccentricity < 1 && + targetOrbitInfo.semiMajorAxis >= 0 ), + validationMessage: 'eccentricity must be [0,1) and semiMajorAxis must [0,)' + } ], + tandem: tandem, phetioValueType: TargetOrbitInfo.TargetOrbitInfoIO, phetioDocumentation: 'Client-configurable preset for Target Orbit, available only via PhET-iO', Index: js/common/model/EllipticalOrbitEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/EllipticalOrbitEngine.ts b/js/common/model/EllipticalOrbitEngine.ts --- a/js/common/model/EllipticalOrbitEngine.ts (revision 4c7a5d1507146c3ab5af2271429f9cdca1cf71f5) +++ b/js/common/model/EllipticalOrbitEngine.ts (date 1706725253428) @@ -390,6 +390,7 @@ // Kepler's Third Law, when this.mu == INITIAL_MU (solar system), then it's T = a^(3/2) // Returns the period in model units public thirdLaw( a: number ): number { + assert && assert( a >= 0, 'axis cannot be negative. Not sure about zero though. . . ????? Help AV' ); return Math.pow( a * a * a * INITIAL_MU / this.mu, 1 / 2 ); } ```

---

@KatieWoe. A note about how studio error handling works for the future: In studio there are two types of error alerts that you can receive from trying to set a value.

1. If it starts with "Invalid value when trying to set value", then it is not a fatal error. Studio validated the provided value before trying to set it, so studio is still usable.
2. Any other error, likely including the word "uncaught" or "assertion". These are fatal errors, and will require a restart to the page. They will also in all likelihood be accompanied by red error logging in the console.

In your post above, each addition error that was alerted ("mismatch. . ." and "reentry") were because the sim was already broken and unusable from the first setting error. Please update the QA book/communicate this studio feature to the QA team as you see fit. Feel free to unassign yourself once noting this.

[KatieWoe]

To clarify, fatal errors are bugs and should be reported, correct?

I looked on main, and I don't see this pop-up. Studio crashes and the assertion errors are in the console, but nothing shows up visually in studio

[zepumph]

To clarify, fatal errors are bugs and should be reported, correct?

Yes correct. I will say though fatal errors should be reported, there are many ways to break PhET-iO that we don't necessarily need to fix. An issue will help us determine if it is worth supporting/solving.

[zepumph]

I looked on main, and I don't see this pop-up. Studio crashes and the assertion errors are in the console, but nothing shows up visually in studio

@KatieWoe told me on slack that she was testing with this link:

https://bayes.colorado.edu/dev/phettest/studio/?sim=keplers-laws&locales=*&phetioWrapperDebug=true&phetioElementsDisplay=all

What you described is expected because your link includes phetioWrapperDebug=true, which will just fail out on fatal errors instead of reporting them back to studio for the alert dialog.

[AgustinVallejo]

Changed a bit @zepumph's patch and committed. Assigning back for review, thanks!

[KatieWoe]

Things look pretty good on main,