phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Address `strictAxonDependencies: false` in NumberAccumulator #831

Closed pixelzoom closed 8 months ago

pixelzoom commented 8 months ago

For https://github.com/phetsims/axon/issues/441, @samreid added 1 occurrence of strictAxonDependencies: false to NumberAccumulator:

    this.valueProperty = new DerivedProperty( [ this.stringProperty ], stringValue => {
      return this.stringToInteger( stringValue );
    }, {
      tandem: options.tandem.createTandem( 'valueProperty' ),
      phetioValueType: NullableIO( NumberIO ),
      strictAxonDependencies: false
    } );

I attempted to address this in https://github.com/phetsims/scenery-phet/commit/73a70fe598c05279e87d94b83ed04ad8c8120ea6, and it seemed to work fine. But with ?listOrderRandom, CT ran into trouble, like:

collision-lab : fuzz : unbuilt : listenerOrderRandom
http://127.0.0.1/continuous-testing/ct-snapshots/1705628023535/collision-lab/collision-lab_en.html?continuousTest=%7B%22test%22%3A%5B%22collision-lab%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1705628023535%22%2C%22timestamp%22%3A1705629385967%7D&brand=phet&ea&fuzz&listenerOrder=random
Query: brand=phet&ea&fuzz&listenerOrder=random
Error: Assertion failed: reentry detected, value=DECIMAL,0,6, oldValue=DECIMAL,0
window.assertions.assertFunction<@http://127.0.0.1/continuous-testing/ct-snapshots/1705628023535/assert/js/assert.js:28:13
at window.assertions.assertFunction< (http://127.0.0.1/continuous-testing/ct-snapshots/1705628023535/assert/js/assert.js:28:13)
at assert (ReadOnlyProperty.ts:324:14)
at _notifyListeners (ReadOnlyProperty.ts:275:13)
at unguardedSet (ReadOnlyProperty.ts:259:11)
at set (Property.ts:65:10)
at set (AbstractKeyAccumulator.ts:81:33)
at updateKeys (NumberAccumulator.ts:124:42)
...

id: "Sparky Node Firefox"
Snapshot from 1/18/2024, 6:33:43 PM

So I reverted https://github.com/phetsims/scenery-phet/commit/73a70fe598c05279e87d94b83ed04ad8c8120ea6.

I have no idea why this should behave differently with listOrderRandom.

@samreid thoughts?

samreid commented 8 months ago

The code review and runtime behavior looked good, so I committed this revert commit: https://github.com/phetsims/scenery-phet/commit/2bf5d4cb0052e9ddd8a0a6deb3f4fc8e33a0cff6

When I fuzzed with a random listener order, I got NaNs in the number pad. Maybe the sim is not set up for a random listener order? But collision-lab is not listed in REPOS_EXCLUDED_FROM_LISTENER_ORDER_RANDOM.

samreid commented 8 months ago

I'm not planning to work on this until I meet up with @pixelzoom. I reached out on slack.

samreid commented 8 months ago

Alternate listener order and strictAxonDependencies don't seem important to check together, in https://github.com/phetsims/faradays-electromagnetic-lab/issues/57#issuecomment-1909089735 I proposed turning off the strictness check if an alternate listener order is specified. @pixelzoom how does that sound?

pixelzoom commented 8 months ago

In https://github.com/phetsims/faradays-electromagnetic-lab/issues/57#issuecomment-1910945878, we turned off strictAxonDependencies when listener order is changed. Since the failing test here "fuzz : unbuilt : listenerOrderRandom", we can therefore close this issue because it's no longer relevant.