phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

PhET-iO metadata is only available in PhET-iO brand #304

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Apparently PhET-iO metadata (like phetioFeatured) cannot be accessed in PhET brand. I've run into this multiple times, and it's super inconvenient.

For example, I did this in acid-base-solutions AqueousSolution.ts, which seems perfectly reasonable, and was requested by the designer:

this.pHProperty = new DerivedProperty( [ this.strengthProperty, this.concentrationProperty ], ... , {
  ...
  // featured if either of its dependencies is featured
  phetioFeatured: this.strengthProperty.phetioFeatured || this.concentrationProperty.phetioFeatured
} );

And that resulted in CT failure https://github.com/phetsims/acid-base-solutions/issues/226, with assertion message:

assert.js:28 Uncaught Error: Assertion failed: phetioFeatured only accessible for instrumented objects in PhET-iO brands

So ... Why is PhET-iO metadata not avaible in PhET brand? And can that be relaxed?

pixelzoom commented 1 year ago

For the above example, I resorted to:

// featured if either of its dependencies is featured
phetioFeatured: Tandem.PHET_IO_ENABLED && ( this.strengthProperty.phetioFeatured || this.concentrationProperty.phetioFeatured )
samreid commented 1 year ago

https://github.com/phetsims/tandem/issues/224#issuecomment-725631174 says:

In the above commit, @samreid and I took this one step farther to not set PhetioObject keys in PhET brand. It involved only a couple of work arounds in PhetioDynamicElementContainer and Node. We also made exposing public fields more explicit with assertions making sure that you don't get an undefined when accessing fields in PhET brand.

So the values are not even being set in phet brand. The example in this issue seems like a fair reason to allow that metadata in phet brand, but it seems like this issue is very rare and may not warrant significant changes to PhetioObject.

Let's hear from @zepumph next.

zepumph commented 1 year ago

So ... Why is PhET-iO metadata not avaible in PhET brand?

As stated above by @samreid, the work was done over in https://github.com/phetsims/tandem/issues/224 during a memory optimizing sprint for PhET-iO. We spend a lot of energy making PhET-iO less heavy and controlling during that work, and I think it was good we did. I want to make sure we all feel the weight and benefit of that work fully before we move to discuss this issue.

And can that be relaxed?

I would recommend no changes. Like @samreid said above, it has been almost 3 years since we have done this work, and this is the first problem we've seen yet, and it comes with a relatively straight forward workaround (only use phet-io options in phet-io brand).

Is that alright with you @pixelzoom?

pixelzoom commented 1 year ago

👍🏻