phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
11 stars 8 forks source link

PatternStringProperty is missing some dependencies in the Studio tree. #439

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Related to https://github.com/phetsims/phet-io/issues/1942 ...

Do we have an instrumentation bug in PatternStringProperty? (or DerivedProperty?) I'm not seeing all of the dependencies in the Studio tree.

For example, if I add this fourier-making-waves-main.ts:

const STRINGS_TANDEM = Tandem.GENERAL_MODEL.createTandem( 'strings' ).createTandem( 'fourierMakingWaves' );

const xMetersStringProperty = new PatternStringProperty( FourierMakingWavesStrings.symbolUnitsStringProperty, {
  symbol: FMWSymbols.xStringProperty,
  units: FourierMakingWavesStrings.units.metersStringProperty
}, {
  tandem: STRINGS_TANDEM.createTandem( 'xMetersStringProperty' )
} );

Then I see this in the Studio tree. Note that there should be 3 dependencies. The FMWSymbols.xStringProperty dependency is missing.

screenshot_2566
pixelzoom commented 1 year ago

Here's another example:

const tmpTandem = Tandem.ROOT.createTandem( 'tmp' );
const patternStringProperty = new StringProperty( '{{a}}{{b}}{{c}}', {
  tandem: tmpTandem.createTandem( 'patternStringProperty' )
} )
const aStringProperty = new StringProperty( 'a', {
  tandem: tmpTandem.createTandem( 'aStringProperty' )
} );
const bStringProperty = new StringProperty( 'b', {
  tandem: tmpTandem.createTandem( 'bStringProperty' )
} );
const cStringProperty = new StringProperty( 'c', {
  tandem: tmpTandem.createTandem( 'cStringProperty' )
} );
const abcStringProperty = new PatternStringProperty( patternStringProperty, {
  a: aStringProperty,
  b: bStringProperty,
  c: cStringProperty
}, {
  tandem: tmpTandem.createTandem( 'abcStringProperty' )
} );

And I see all of the expected depenendences in Studio:

screenshot_2567

So I suspect that the problem with the example in https://github.com/phetsims/axon/issues/439#issue-1742377597 is that FMWSymbols.xStringProperty is not instrumented. Assigning this back to me to investigate.

zepumph commented 1 year ago

Yes, I see that the intermediate derivedProperties are not instrumented.

https://github.com/phetsims/scenery-phet/blob/1c113fb1247da45c7d74f01faa00acfb8e0f0acb/js/MathSymbolFont.ts#L63

I will also say that we knew these sneaky cases would result in trouble. In https://github.com/phetsims/axon/commit/4d4d1a1750ad6f6b45371a2be850ce1bc25d29cb over in https://github.com/phetsims/phet-io/issues/1943 we decided that it was worth some extra logic to detect a string derivedProperty case like this for whether or not to auto feature it, but we didn't apply that same logic to if we should require instrumentation from it. I could go either way. If you would like some attempt at forcing Tandem.REQUIRED in cases like this please reopen that issue over there.

pixelzoom commented 1 year ago

This was indeed due to not instrumenting a dependency, and I addressed it in the sim. I don't feel the need to reopen https://github.com/phetsims/phet-io/issues/1943.

Closing.