NumberDisplay is missing dependencies related to numberFormatter.

[pixelzoom]

Discovered in phetsims/axon#441, in the context of gas-properties (and gases-intro, diffusion) and keplers-laws ...

NumberDisplay has a problem with missing dependencies for these DerivedProperties:

    const minStringProperty = new DerivedProperty( [ numberFormatterProperty ], numberFormatter => {
      return valueToString( displayRange.min, options.noValueString, numberFormatter );
    } );
    const maxStringProperty = new DerivedProperty( [ numberFormatterProperty ], numberFormatter => {
      return valueToString( displayRange.max, options.noValueString, numberFormatter );
    } );
...
    const valueStringProperty = new DerivedStringProperty(
      [ numberProperty, noValuePatternProperty, valuePatternProperty, numberFormatterProperty ],
      ( value, noValuePattern, valuePatternValue, numberFormatter ) => {
        const valuePattern = ( value === null && noValuePattern ) ? noValuePattern : valuePatternValue;
        // NOTE: this.numberFormatter could change, so we support a recomputeText() below that recomputes this derivation
        const stringValue = valueToString( value, options.noValueString, numberFormatter );
        const rval = StringUtils.fillIn( valuePattern, {
          value: stringValue
        } );
        return rval;
      }, {
        tandem: valueTextTandem.createTandem( Text.STRING_PROPERTY_TANDEM_NAME )
      } );

In all 3 of these Derivations, valueToString uses a numberFormatter that may rely on other Properties.

In the case of gas-properties, gases-intro, and diffusion, this problem manifests through StopwatchNode. Here's the relevant code for the numberFormatter, and the missing dependencies:

  public static createRichTextNumberFormatter( providedOptions?: FormatterOptions ): ( time: number ) => string {
...
      valueUnitsPattern: SceneryPhetStrings.stopwatchValueUnitsPatternStringProperty
...
      return StringUtils.fillIn( options.valueUnitsPattern, {

In keplers-laws, the problem manifests in EllipticalOrbitNode, where each numberFormatter has dependencies on StringProperties. Here's the relevant code:

      areaValueNumberDisplays.push( new NumberDisplay( areaValueProperty, areaValueRange, {
...
        numberFormatter: ( value: number ) => {
          return StringUtils.fillIn( KeplersLawsStrings.pattern.valueUnitsStringProperty, {
            value: Utils.toFixed( value, 2 ),
            units: KeplersLawsDerivedStrings.AU2StringProperty
          } );
        }
...
      timeValueNumberDisplays.push( new NumberDisplay( timeValueProperty, timeValueRange, {
        numberFormatter: ( value: number ) => {
          return StringUtils.fillIn( KeplersLawsStrings.pattern.valueUnitsStringProperty, {
            value: Utils.toFixed( value, 2 ),
            units: KeplersLawsStrings.units.yearsStringProperty
          } );
        }
      } ) );

[pixelzoom]

Resolving this problem with the current NumberDisplay API doesn't seem reasonable or desirable. It will result in more workarounds on top of an API that is long overdue for replacement. So I'll repeat what I said in #812 (comment):

I'll also note that NumberDisplay has become quite the mess. I was the original author, and I almost didn't recognize it. So much chaos has been introduced that it might be better to rewrite, rather than revise.

@samreid Let's discuss in the context of phetsims/axon#441.

[pixelzoom]

In 60364e4, @samreid temporarily silenced these problems by adding accessNonDependencies: true.

[samreid]

I'm planning to investigate this. Solo-assigning for now.

[samreid]

Issue #814 is likely a symptom of this.

[samreid]

I leveraged the patch in #814 and used the new tooling in phetsims/axon#441 to make sure coverage was complete. This is working well in Kepler's Laws.

Subject: [PATCH] Update API due to gravity change and ignore initial value changes, see https://github.com/phetsims/phet-core/issues/132
---
Index: keplers-laws/js/common/view/EllipticalOrbitNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/keplers-laws/js/common/view/EllipticalOrbitNode.ts b/keplers-laws/js/common/view/EllipticalOrbitNode.ts
--- a/keplers-laws/js/common/view/EllipticalOrbitNode.ts	(revision b44f1a18abd88ce751cf8183ced00297a68a512b)
+++ b/keplers-laws/js/common/view/EllipticalOrbitNode.ts	(date 1700091113843)
@@ -217,6 +217,9 @@
         scale: 0.7,
         opacity: 0.8,
         useRichText: true,
+        additionalPropertiesForUpdate: [
+          KeplersLawsStrings.pattern.valueUnitsStringProperty,
+          KeplersLawsDerivedStrings.AU2StringProperty ],
         numberFormatter: ( value: number ) => {
           return StringUtils.fillIn( KeplersLawsStrings.pattern.valueUnitsStringProperty, {
             value: Utils.toFixed( value, 2 ),
@@ -228,6 +231,10 @@
         scale: 0.7,
         opacity: 0.8,
         backgroundFill: KeplersLawsColors.timeDisplayBackgroundColorProperty,
+        additionalPropertiesForUpdate: [
+          KeplersLawsStrings.pattern.valueUnitsStringProperty,
+          KeplersLawsStrings.units.yearsStringProperty
+        ],
         numberFormatter: ( value: number ) => {
           return StringUtils.fillIn( KeplersLawsStrings.pattern.valueUnitsStringProperty, {
             value: Utils.toFixed( value, 2 ),
Index: scenery-phet/js/NumberDisplay.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/NumberDisplay.ts b/scenery-phet/js/NumberDisplay.ts
--- a/scenery-phet/js/NumberDisplay.ts	(revision 3fdea2c7a77cbcd9e1985373407ae954101e8761)
+++ b/scenery-phet/js/NumberDisplay.ts	(date 1700097343919)
@@ -21,8 +21,8 @@
 import MathSymbols from './MathSymbols.js';
 import PhetFont from './PhetFont.js';
 import sceneryPhet from './sceneryPhet.js';
-import DerivedStringProperty from '../../axon/js/DerivedStringProperty.js';
 import Tandem from '../../tandem/js/Tandem.js';
+import StringIO from '../../tandem/js/types/StringIO.js';
 
 // constants
 const DEFAULT_FONT = new PhetFont( 20 );
@@ -76,6 +76,8 @@
   noValueString?: string; // default is the PhET standard, do not override lightly.
   noValueAlign?: NumberDisplayAlign | null; // see ALIGN_VALUES. If null, defaults to options.align
   noValuePattern?: string | TReadOnlyProperty<string> | null; // If null, defaults to options.valuePattern
+
+  additionalPropertiesForUpdate?: TReadOnlyProperty<unknown>[];
 };
 export type NumberDisplayOptions = SelfOptions & StrictOmit<NodeOptions, 'children'>;
 
@@ -121,6 +123,8 @@
       noValueAlign: null,
       noValuePattern: null,
 
+      additionalPropertiesForUpdate: [],
+
       // phet-io
       tandem: Tandem.OPT_OUT,
       phetioType: NumberDisplay.NumberDisplayIO
@@ -188,15 +192,15 @@
       `missing value placeholder in options.noValuePattern: ${noValuePatternProperty.value}` );
 
     // determine the widest value
-    const minStringProperty = new DerivedProperty( [ numberFormatterProperty ], numberFormatter => {
-      return valueToString( displayRange.min, options.noValueString, numberFormatter );
+    const minStringProperty = DerivedProperty.deriveAny( [ numberFormatterProperty, ...options.additionalPropertiesForUpdate ], () => {
+      return valueToString( displayRange.min, options.noValueString, numberFormatterProperty.value );
     }, {
-      accessNonDependencies: true //TODO https://github.com/phetsims/scenery-phet/issues/824
+      // accessNonDependencies: true //TODO https://github.com/phetsims/scenery-phet/issues/824
     } );
-    const maxStringProperty = new DerivedProperty( [ numberFormatterProperty ], numberFormatter => {
-      return valueToString( displayRange.max, options.noValueString, numberFormatter );
+    const maxStringProperty = DerivedProperty.deriveAny( [ numberFormatterProperty, ...options.additionalPropertiesForUpdate ], () => {
+      return valueToString( displayRange.max, options.noValueString, numberFormatterProperty.value );
     }, {
-      accessNonDependencies: true //TODO https://github.com/phetsims/scenery-phet/issues/824
+      // accessNonDependencies: true //TODO https://github.com/phetsims/scenery-phet/issues/824
     } );
     const longestStringProperty = new DerivedProperty( [
       valuePatternProperty,
@@ -211,9 +215,14 @@
     // value
     const ValueTextConstructor = options.useRichText ? RichText : Text;
     const valueTextTandem = options.textOptions.tandem || options.tandem.createTandem( 'valueText' );
-    const valueStringProperty = new DerivedStringProperty(
-      [ numberProperty, noValuePatternProperty, valuePatternProperty, numberFormatterProperty ],
-      ( value, noValuePattern, valuePatternValue, numberFormatter ) => {
+    const valueStringProperty = DerivedProperty.deriveAny(
+      [ numberProperty, noValuePatternProperty, valuePatternProperty, numberFormatterProperty, ...options.additionalPropertiesForUpdate ],
+      () => {
+        const value = numberProperty.value;
+        const noValuePattern = noValuePatternProperty.value;
+        const valuePatternValue = valuePatternProperty.value;
+        const numberFormatter = numberFormatterProperty.value;
+
         const valuePattern = ( value === null && noValuePattern ) ? noValuePattern : valuePatternValue;
         // NOTE: this.numberFormatter could change, so we support a recomputeText() below that recomputes this derivation
         const stringValue = valueToString( value, options.noValueString, numberFormatter );
@@ -222,7 +231,10 @@
         } );
       }, {
         tandem: valueTextTandem.createTandem( Text.STRING_PROPERTY_TANDEM_NAME ),
-        accessNonDependencies: true //TODO https://github.com/phetsims/scenery-phet/issues/824
+        phetioFeatured: true, // featured by default, see https://github.com/phetsims/phet-io/issues/1943
+        phetioValueType: StringIO,
+        tandemNameSuffix: 'StringProperty' // Change only with caution
+        // accessNonDependencies: true //TODO https://github.com/phetsims/scenery-phet/issues/824
       } );
 
     const valueTextOptions = combineOptions<TextOptions | RichTextOptions>( {
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts	(revision d10979bb7dc9c8011e30031ed5b6e157921f08a2)
+++ b/axon/js/ReadOnlyProperty.ts	(date 1700090457693)
@@ -232,7 +232,7 @@
       if ( !currentDependencies.includes( this ) ) {
 
         // TODO: Re-enable assertion, see https://github.com/phetsims/axon/issues/441
-        // assert && assert( false, 'accessed value outside of dependency tracking' );
+        assert && assert( false, 'accessed value outside of dependency tracking' );
       }
     }
     return this.tinyProperty.get();
Index: keplers-laws/js/common/view/KeplersLawsScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/keplers-laws/js/common/view/KeplersLawsScreenView.ts b/keplers-laws/js/common/view/KeplersLawsScreenView.ts
--- a/keplers-laws/js/common/view/KeplersLawsScreenView.ts	(revision b44f1a18abd88ce751cf8183ced00297a68a512b)
+++ b/keplers-laws/js/common/view/KeplersLawsScreenView.ts	(date 1700097164343)
@@ -386,7 +386,11 @@
             numberOfDecimalPlaces: 2,
             valueUnitsPattern: KeplersLawsStrings.pattern.valueUnitsStringProperty,
             units: KeplersLawsStrings.units.yearsStringProperty
-          } )
+          } ),
+          additionalPropertiesForUpdate: [
+            KeplersLawsStrings.units.yearsStringProperty,
+            KeplersLawsStrings.pattern.valueUnitsStringProperty
+          ]
         },
         tandem: options.tandem.createTandem( 'stopwatchNode' )
       }
Index: scenery-phet/js/StopwatchNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/StopwatchNode.ts b/scenery-phet/js/StopwatchNode.ts
--- a/scenery-phet/js/StopwatchNode.ts	(revision 3fdea2c7a77cbcd9e1985373407ae954101e8761)
+++ b/scenery-phet/js/StopwatchNode.ts	(date 1700097582515)
@@ -146,7 +146,12 @@
         cornerRadius: 4,
         xMargin: 4,
         yMargin: 2,
-        pickable: false // allow dragging by the number display
+        pickable: false, // allow dragging by the number display
+        additionalPropertiesForUpdate: [
+
+          // Used in the numberFormatter above
+          SceneryPhetStrings.stopwatchValueUnitsPatternStringProperty
+        ]
       },
       dragBoundsProperty: null,
       dragListenerOptions: {

However, I feel we should check in with @pixelzoom before committing. Also, do not commit the full patch above because it is optimized for Kepler's Laws and a general commit probably would to specify accessNonDependencies: true until other cases are addressed.

[samreid]

I’m using the “dev:help-wanted” label to indicate a synchronous conversation would be best. Maybe we can schedule a short time after a coming standup. Or we could request help from another dev after check-in on Thursday's dev meeting.

[pixelzoom]

@samreid and I discussed the patch in #824 (comment). While it does the job, we both feel like it's a relatively ugly workaround that neither of us really wants to apply to NumberDisplay - see #828. What is really needed is a next-generation NumberDisplay, with an API that is better suited to displaying a string that is derived from a TReadOnlyProperty<number | null> and other dependencies (like translated strings, preferences, etc.)

So we decided to:

• NOT apply this patch
• Continue to opt out using accessNonDependencies: true in NumberDisplay
• Keep this issue open, in case anyone needs the patch for a "must be fixed" situation
• Unassign ourselves

[samreid]

At today's dev meeting, we agreed to go ahead with the patch. I'll update it and commit it, and @pixelzoom agreed to review.

[pixelzoom]

1/4/24 dev meeting summary:

• @samreid will apply the patch shown in NumberDisplay is missing dependencies related to numberFormatter. #824 (comment)
• @pixelzoom will review
• @zepumph and @pixelzoom suggested changing additionalPropertiesForUpdate to additionalDependencies, or something like what @zepumph has used for description options

[zepumph]

Looks like I called it a11yDependencies in AccessibleValueHandler.

https://github.com/phetsims/sun/blob/f26d1565a6ac82febe1720e2b0b9c7a4b7ad1b5a/js/accessibility/AccessibleValueHandler.ts#L210

Then additionalDescriptionDependencies for ISLC:

https://github.com/phetsims/gravity-force-lab/blob/04288edf2d20441b1805715c42b0cd58071edd0c/js/view/SpherePositionsDescriptionNode.js#L30

So I think just anything with dependencies sounds nice.

[samreid]

@pixelzoom and I reviewed my proposed changes and agreed it is ready to push.

We observed that all calls to setNumberFormatter look like a workaround trying to solve this same problem. @pixelzoom will look at BLL and Fourier to see if the new option is preferable. It could clean things up to get rid of setNumberFormatter.

[samreid]

OK the work is pushed. Over to @pixelzoom for a closer look.

[pixelzoom]

In the above commits, I replaced workarounds (typically link or multilink) with use of numberFormatterDependencies. This cleaned things up nicely.

It also made it possible to delete these methods:

• NumberDisplay setNumberFormatter and recomputeText
• NumberControl setNumberFormatter and redrawNumberDisplay
• StopwatchNode setNumberFormatter and redrawNumberDisplay

Assigning back to @samreid for a final look. Feel free to close if OK.

Note the changes to wave-interference in phetsims/wave-interference@a7fc9a3 and phetsims/wave-interference@6dd8dc0. The stopwatch does update when switching scenes. But I found it odd that switching scenes puts the stopwatch back in the toolbox.

[samreid]

I skimmed through the commits and they all look good. I think this issue can be closed.