search

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

PhET-iO API is broken for ObservableArray #437

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

I encountered this problem when migrating natural-selection-overrides.js into code for https://github.com/phetsims/natural-selection/issues/331.

natural-selection-overrides.js contains:

    "naturalSelection.introScreen.model.graphs.proportionsModel.previousCounts": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.proportionsModel.previousCounts": {
      "phetioFeatured": true
    },

... where previousCounts is an ObservableArray, defined in ProportionsModel.ts. Here's how I expected to be able to move the above override into code:

    const previousCounts = createObservableArray<ProportionsCounts>( {
      tandem: options.tandem.createTandem( 'previousCounts' ),
+     phetioFeatured: true
      phetioType: createObservableArray.ObservableArrayIO( ProportionsCounts.ProportionsCountsIO ),
      phetioDocumentation: 'Start and End counts for previous generations, indexed by generation number'
    } );

This did not go well. The phetioFeatured option to createObservableArray has a serious problem. It's not only applied to the ObservableArrayPhetioObject. It's also applied to 3 related Properties, which we do not want to feature in Natural Selection. Here's the relevant code in createObservableArray.ts: 

const createObservableArray = <T>( providedOptions?: ObservableArrayOptions<T> ): ObservableArray<T> => {
  ...
  const options = optionize<ObservableArrayOptions<T>, SpecifiedObservableArrayOptions<T>>()( {
    ...
    phetioFeatured: false
  }, providedOptions );
  ...
  const elementAddedEmitter = new Emitter<[ T ]>( {
    ...
    phetioFeatured: options.phetioFeatured
  } );

  const elementRemovedEmitter = new Emitter<[ T ]>( {
    ...
    phetioFeatured: options.phetioFeatured
  } );

  const lengthProperty = new NumberProperty( 0, {
    ...
    phetioFeatured: options.phetioFeatured
  } );
  ...
    observableArray._observableArrayPhetioObject = new ObservableArrayPhetioObject( observableArray, options );
  ...
  return observableArray;
};

So... It's currently impossible to properly feature an ObservableArray via code.

pixelzoom commented 1 year ago

Here's the complete set of ObservableArrays that are featured in natural-selection-overrides.js. None of these can currently be migrated.

    "naturalSelection.introScreen.model.graphs.proportionsModel.previousCounts": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.proportionsModel.previousCounts": {
      "phetioFeatured": true
    },
    "naturalSelection.introScreen.model.graphs.populationModel.dataPoints.brownFurPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.introScreen.model.graphs.populationModel.dataPoints.totalPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.introScreen.model.graphs.populationModel.dataPoints.whiteFurPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.populationModel.dataPoints.brownFurPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.populationModel.dataPoints.floppyEarsPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.populationModel.dataPoints.longTeethPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.populationModel.dataPoints.shortTeethPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.populationModel.dataPoints.straightEarsPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.populationModel.dataPoints.totalPoints": {
      "phetioFeatured": true
    },
    "naturalSelection.labScreen.model.graphs.populationModel.dataPoints.whiteFurPoints": {
      "phetioFeatured": true
    },
samreid commented 1 year ago

@arouinfar can you please comment on the desired instrumentation capabilities/defaults/flexibility for ObservableArray? Specifically:

Once we hear about the design recommendations, we can adjust the implementation accordingly.

pixelzoom commented 1 year ago

Two days ago in "When should Emitters be instrumented?", @zepumph summarized in https://github.com/phetsims/phet-io/issues/1896#issuecomment-1544537238:

From PhET-iO meeting today, we will add some technical-guide documentation and call it a day:

  1. Most likely you don't need to instrument Emitters during initial dev first pass.
  2. Some Emitters are important to the functionality of the sim (CCK circuitChangedEmitter or elementAddedEmitter).
  3. Emitters can be instrumented to assist in a more rich data stream (we aren't currently designing this too much)
  4. We aren't going to eagerly or consistently instrument emitters in common code.

The current implementation of ObservableArray doesn't seem to be at all consistent with those conclusions. Not only does featuring an ObservableArray cause its Emitters to be featured (with no way to opt out), but the Emitters also inherit the featured-ness of the ObservableArray (again with no way to opt out).

pixelzoom commented 1 year ago

Also noting that requiring the Emitters to be instrumented if the ObservableArray is instrumented doesn't "assist in a more rich data stream". It results in spamming of the data stream in cases where there are many dynamic elements - for example bunnies in Natural Selection, particles in Gas Properties, etc.

pixelzoom commented 1 year ago

@samreid asked:

  • Should you be able to feature the ObservableArray without featuring its elementAddedEmitter, elementRemovedEmitter and lengthProperty?

Absolutely. And PhET has an established pattern for doing that, which was not done here. Like Node's visiblePropertyOptions and enabledPropertyOptions, ObservableArray should have elementAddedEmitterOptions, elementRemovedEmitterOptions, and lengthPropertyOptions.

  • Should you be able to specify arbitrary featured-ness for all 4 of those things? Are any of them connected?

Yes, via elementAddedEmitterOptions, elementRemovedEmitterOptions, and lengthPropertyOptions. And no, they should not be connected.

  • What should the defaults be? Should any of the defaults be connected to options (like if the ObservableArray is featured, that would default the children to featured too? But maybe that is confusing and complicated and things should just be independent).

I defer to designers. But defaults should be designed. But again, the same pattern used by Node should be used here.

pixelzoom commented 1 year ago

I changed the title of this issue to reflect the fact that the "brokenness" of ObservableArray's API is not limited to phetioFeatured. It's a broader problem, including how elementAddedEmitter, elementRemovedEmitter, andlengthProperty` are instrumented, and how their metadata is specified.

samreid commented 1 year ago

I added observableArray.elementAddedEmitterOptions, elementRemovedEmitterOptions, and lengthPropertyOptions. In running precommit hooks, the only API that was altered by this was CCK, so I committed https://github.com/phetsims/circuit-construction-kit-common/commit/43cf0dfef5f5d035c983551b85b675f84170139b to keep those featured aspects the same. However, I wasn't sure that the circuitElements.lengthProperty should be featured, or if that was just by happenstance. @arouinfar can you please comment on that?

@pixelzoom can you review this change or do you want to run it past @zepumph first?

arouinfar commented 1 year ago

@arouinfar can you please comment on that?

I'm not familiar with ObservableArray, so it would be helpful to review in the context of a few sims. Can you point me to some phetioIDs of ObservableArray @samreid? I see some reference to Natural Selection in this issue, but I wouldn't say I know that sim very well. Examples from another sim or two would be helpful.

pixelzoom commented 1 year ago

@pixelzoom can you review this change or do you want to run it past @zepumph first?

Three problems with https://github.com/phetsims/axon/commit/93206adfad3b4df82459005ca2bfbf888b43f3f8.

(1) The types of elementAddedEmitterOptions, elementRemovedEmitterOptions, and lengthPropertyOptions are incorrect. They allow options that are the responsibility of ObservableArray.

(2) There is still no way to opt-out of instrumenting elementAddedEmitterOptions, elementRemovedEmitterOptions, and lengthPropertyOptions.

(3) Again... Based on decisions in https://github.com/phetsims/phet-io/issues/1896#issuecomment-1544537238, the default should be to NOT instrument the Emitters. And I don't believe that any designer has requested these Emitters to be instrumented -- I had separate conversations with @arouinfar and @amanda-phet today.

Here's a patch that I believe addresses (1) and (2). @samreid please apply, review, and (if you think it's OK) commit.

Addressing (3) would involve changing the defaults for elementAddedEmitterInstrumented, elementRemovedEmitterInstrumented, and lengthPropertyInstrumented, and would likely require API changes to some sims.

Patch ```diff Subject: [PATCH] feature previousCounts and populationModel.dataPoints.*Points in both screens, https://github.com/phetsims/natural-selection/issues/336 --- Index: js/createObservableArray.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/createObservableArray.ts b/js/createObservableArray.ts --- a/js/createObservableArray.ts (revision 93206adfad3b4df82459005ca2bfbf888b43f3f8) +++ b/js/createObservableArray.ts (date 1684197069620) @@ -44,10 +44,17 @@ phetioDocumentation?: string; phetioFeatured?: boolean; - elementAddedEmitterOptions?: EmitterOptions; - elementRemovedEmitterOptions?: EmitterOptions; - lengthPropertyOptions?: NumberPropertyOptions; + // Whether to instrument the array's child elements + elementAddedEmitterInstrumented?: boolean; + elementRemovedEmitterInstrumented?: boolean; + lengthPropertyInstrumented?: boolean; + + // Options for the array's child elements. Omitted options are the responsibility of the array. + elementAddedEmitterOptions?: StrictOmit; + elementRemovedEmitterOptions?: StrictOmit; + lengthPropertyOptions?: StrictOmit; }; + type ObservableArray = { get: ( index: number ) => T; addItemAddedListener: ( listener: ObservableArrayListener ) => void; @@ -103,7 +110,9 @@ elements: [], tandem: Tandem.OPTIONAL, phetioFeatured: false, - + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true, elementAddedEmitterOptions: {}, elementRemovedEmitterOptions: {}, lengthPropertyOptions: {} @@ -126,7 +135,7 @@ // notifies when an element has been added const elementAddedEmitter = new Emitter<[ T ]>( { - tandem: options.tandem.createTandem( 'elementAddedEmitter' ), + tandem: options.elementAddedEmitterInstrumented ? options.tandem.createTandem( 'elementAddedEmitter' ) : Tandem.OPT_OUT, parameters: [ emitterParameterOptions ], phetioReadOnly: true, hasListenerOrderDependencies: options.hasListenerOrderDependencies, @@ -135,7 +144,7 @@ // notifies when an element has been removed const elementRemovedEmitter = new Emitter<[ T ]>( { - tandem: options.tandem.createTandem( 'elementRemovedEmitter' ), + tandem: options.elementRemovedEmitterInstrumented ? options.tandem.createTandem( 'elementRemovedEmitter' ) : Tandem.OPT_OUT, parameters: [ emitterParameterOptions ], phetioReadOnly: true, hasListenerOrderDependencies: options.hasListenerOrderDependencies, @@ -145,7 +154,7 @@ // observe this, but don't set it. Updated when Array modifiers are called (except array.length=...) const lengthProperty = new NumberProperty( 0, { numberType: 'Integer', - tandem: options.tandem.createTandem( 'lengthProperty' ), + tandem: options.lengthPropertyInstrumented ? options.tandem.createTandem( 'lengthProperty' ) : Tandem.OPT_OUT, phetioReadOnly: true, ...options.lengthPropertyOptions } ); ```

EDIT: You may want to rename elementAddedEmitterInstrumented to phetioElementAddedEmitterInstrumented etc., to be consistent with Node's phetioVisiblePropertyInstrumented. On the other hand, I've never liked phetioVisiblePropertyInstrumented and would prefer visiblePropertyInstrumented.

pixelzoom commented 1 year ago

Another problem with https://github.com/phetsims/axon/commit/93206adfad3b4df82459005ca2bfbf888b43f3f8:

(4) Destructuring should not be used in place of combineOptions here, it is not type-safe. For example, this passes a bad option to new Emitter and results in no tsc errors:

  const junk = { foobar: true };
  const elementAddedEmitter = new Emitter<[ T ]>( {
    tandem: options.tandem.createTandem( 'elementAddedEmitter' ),
    parameters: [ emitterParameterOptions ],
    phetioReadOnly: true,
    hasListenerOrderDependencies: options.hasListenerOrderDependencies,
    ...options.elementAddedEmitterOptions,
    ...junk
  } );

Here's a patch that addresses that problem too.

Patch ```diff Subject: [PATCH] feature previousCounts and populationModel.dataPoints.*Points in both screens, https://github.com/phetsims/natural-selection/issues/336 --- Index: js/createObservableArray.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/createObservableArray.ts b/js/createObservableArray.ts --- a/js/createObservableArray.ts (revision 93206adfad3b4df82459005ca2bfbf888b43f3f8) +++ b/js/createObservableArray.ts (date 1684198215063) @@ -14,7 +14,7 @@ import StrictOmit from '../../phet-core/js/types/StrictOmit.js'; import assertMutuallyExclusiveOptions from '../../phet-core/js/assertMutuallyExclusiveOptions.js'; import merge from '../../phet-core/js/merge.js'; -import optionize from '../../phet-core/js/optionize.js'; +import optionize, { combineOptions } from '../../phet-core/js/optionize.js'; import PhetioObject from '../../tandem/js/PhetioObject.js'; import Tandem from '../../tandem/js/Tandem.js'; import ArrayIO from '../../tandem/js/types/ArrayIO.js'; @@ -44,10 +44,17 @@ phetioDocumentation?: string; phetioFeatured?: boolean; - elementAddedEmitterOptions?: EmitterOptions; - elementRemovedEmitterOptions?: EmitterOptions; - lengthPropertyOptions?: NumberPropertyOptions; + // Whether to instrument the array's child elements + elementAddedEmitterInstrumented?: boolean; + elementRemovedEmitterInstrumented?: boolean; + lengthPropertyInstrumented?: boolean; + + // Options for the array's child elements. Omitted options are the responsibility of the array. + elementAddedEmitterOptions?: StrictOmit; + elementRemovedEmitterOptions?: StrictOmit; + lengthPropertyOptions?: StrictOmit; }; + type ObservableArray = { get: ( index: number ) => T; addItemAddedListener: ( listener: ObservableArrayListener ) => void; @@ -103,7 +110,9 @@ elements: [], tandem: Tandem.OPTIONAL, phetioFeatured: false, - + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true, elementAddedEmitterOptions: {}, elementRemovedEmitterOptions: {}, lengthPropertyOptions: {} @@ -125,30 +134,30 @@ } // notifies when an element has been added - const elementAddedEmitter = new Emitter<[ T ]>( { - tandem: options.tandem.createTandem( 'elementAddedEmitter' ), + const elementAddedEmitter = new Emitter<[ T ]>( combineOptions( { + tandem: options.elementAddedEmitterInstrumented ? options.tandem.createTandem( 'elementAddedEmitter' ) : Tandem.OPT_OUT, parameters: [ emitterParameterOptions ], phetioReadOnly: true, - hasListenerOrderDependencies: options.hasListenerOrderDependencies, - ...options.elementAddedEmitterOptions - } ); + hasListenerOrderDependencies: options.hasListenerOrderDependencies + }, options.elementAddedEmitterOptions ) ); + + const foo = { junk: true }; // notifies when an element has been removed - const elementRemovedEmitter = new Emitter<[ T ]>( { - tandem: options.tandem.createTandem( 'elementRemovedEmitter' ), + const elementRemovedEmitter = new Emitter<[ T ]>( combineOptions( { + tandem: options.elementRemovedEmitterInstrumented ? options.tandem.createTandem( 'elementRemovedEmitter' ) : Tandem.OPT_OUT, parameters: [ emitterParameterOptions ], phetioReadOnly: true, hasListenerOrderDependencies: options.hasListenerOrderDependencies, - ...options.elementRemovedEmitterOptions - } ); + ...foo + }, options.elementRemovedEmitterOptions ) ); // observe this, but don't set it. Updated when Array modifiers are called (except array.length=...) - const lengthProperty = new NumberProperty( 0, { + const lengthProperty = new NumberProperty( 0, combineOptions( { numberType: 'Integer', - tandem: options.tandem.createTandem( 'lengthProperty' ), - phetioReadOnly: true, - ...options.lengthPropertyOptions - } ); + tandem: options.lengthPropertyInstrumented ? options.tandem.createTandem( 'lengthProperty' ) : Tandem.OPT_OUT, + phetioReadOnly: true + }, options.lengthPropertyOptions ) ); // The underlying array which is wrapped by the Proxy const targetArray: T[] = []; ```
samreid commented 1 year ago

That patch looks good to me. I removed the "foo" example and will commit the patch shortly. After that, we can investigate (3) from https://github.com/phetsims/axon/issues/437#issuecomment-1548797660

samreid commented 1 year ago

I opened a few side issues for related topics. See above.

samreid commented 1 year ago

When I change (3) above like so:

```diff Subject: [PATCH] message --- Index: js/createObservableArray.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/createObservableArray.ts b/js/createObservableArray.ts --- a/js/createObservableArray.ts (revision 80443ff9f7dd3a45711c013efa5074870f83fc26) +++ b/js/createObservableArray.ts (date 1684403891766) @@ -110,9 +110,9 @@ elements: [], tandem: Tandem.OPTIONAL, phetioFeatured: false, - elementAddedEmitterInstrumented: true, - elementRemovedEmitterInstrumented: true, - lengthPropertyInstrumented: true, + elementAddedEmitterInstrumented: false, + elementRemovedEmitterInstrumented: false, + lengthPropertyInstrumented: false, elementAddedEmitterOptions: {}, elementRemovedEmitterOptions: {}, lengthPropertyOptions: {} ```

The changes in CCK seem undesirable--it uninstruments the circuitElementAdded and removed emitters, which seem important for clients that want to customize objects on creation. But I could compensate for this by opting in to that instrumentation. Not sure if we want to instrument circuitElements.lengthProperty or not.

There are other changes in Natural Selection which I'm unfamiliar with.

@pixelzoom how do you want to proceed?

pixelzoom commented 1 year ago

The changes in CCK seem undesirable--it uninstruments the circuitElementAdded and removed emitters, which seem important for clients that want to customize objects on creation.

What is "undesirable" about it?

Is it that you need to override the defaults and you don't like having to add the extra options? What's not typical about that? I spend an enormous amount of time adding verbose options to override PHET-iO defaults, why should CCK be different?

Is it that you think the default should be "true"? If that's the case, please discuss with designers. My discussions with @arouinfar and @amanda-phet suggest that the defaults should be false. My experience with sims also suggests that the defaults should be false. And (again) https://github.com/phetsims/phet-io/issues/1896#issuecomment-1544537238 specifically says that Emitters should NOT be instrumented by default, and instrumentation should be opt-in.

@pixelzoom how do you want to proceed?

Again from https://github.com/phetsims/axon/issues/437#issuecomment-1546705710:

What should the defaults be? ...

I defer to designers. But defaults should be designed.

So... Discuss the defaults with designers. And if true is an acceptable default for elementAddedEmitterInstrumented and elementRemovedEmitterInstrumented document why that exception is appropriate for the decisions made in https://github.com/phetsims/phet-io/issues/1896#issuecomment-1544537238 .

samreid commented 1 year ago

I believe we are in agreement with the desirable defaults and it does not need to be discussed with designers. I'll commit momentarily.

samreid commented 1 year ago

I wrote this patch to make the element added/removed and length property uninstrumented by default:

```diff Subject: [PATCH] Change default instrumentation level to false for observableArray.elementAdded/removed/lengthProperty, see https://github.com/phetsims/axon/issues/437 --- Index: main/natural-selection/js/common/model/PopulationModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/natural-selection/js/common/model/PopulationModel.ts b/main/natural-selection/js/common/model/PopulationModel.ts --- a/main/natural-selection/js/common/model/PopulationModel.ts (revision b54b163b9c65d9c00f4c504ed64f9bde9b1d0ebb) +++ b/main/natural-selection/js/common/model/PopulationModel.ts (date 1684518992405) @@ -117,43 +117,64 @@ tandem: dataPointsTandem.createTandem( 'totalPoints' ), phetioFeatured: true, phetioType: createObservableArray.ObservableArrayIO( Vector2.Vector2IO ), - phetioDocumentation: 'Population data points for all bunnies' + phetioDocumentation: 'Population data points for all bunnies', + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true } ); this.whiteFurPoints = createObservableArray( { tandem: dataPointsTandem.createTandem( 'whiteFurPoints' ), phetioFeatured: true, phetioType: createObservableArray.ObservableArrayIO( Vector2.Vector2IO ), - phetioDocumentation: 'Population data points for bunnies with white fur' + phetioDocumentation: 'Population data points for bunnies with white fur', + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true } ); this.brownFurPoints = createObservableArray( { tandem: dataPointsTandem.createTandem( 'brownFurPoints' ), phetioFeatured: true, phetioType: createObservableArray.ObservableArrayIO( Vector2.Vector2IO ), - phetioDocumentation: 'Population data points for bunnies with brown fur' + phetioDocumentation: 'Population data points for bunnies with brown fur', + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true } ); this.straightEarsPoints = createObservableArray( { tandem: dataPointsTandem.createTandem( 'straightEarsPoints' ), phetioFeatured: true, phetioType: createObservableArray.ObservableArrayIO( Vector2.Vector2IO ), - phetioDocumentation: 'Population data points for bunnies with straight ears' + phetioDocumentation: 'Population data points for bunnies with straight ears', + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true } ); this.floppyEarsPoints = createObservableArray( { tandem: dataPointsTandem.createTandem( 'floppyEarsPoints' ), phetioFeatured: true, phetioType: createObservableArray.ObservableArrayIO( Vector2.Vector2IO ), - phetioDocumentation: 'Population data points for bunnies with floppy ears' + phetioDocumentation: 'Population data points for bunnies with floppy ears', + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true } ); this.shortTeethPoints = createObservableArray( { tandem: dataPointsTandem.createTandem( 'shortTeethPoints' ), phetioFeatured: true, phetioType: createObservableArray.ObservableArrayIO( Vector2.Vector2IO ), - phetioDocumentation: 'Population data points for bunnies with short teeth' + phetioDocumentation: 'Population data points for bunnies with short teeth', + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true } ); this.longTeethPoints = createObservableArray( { tandem: dataPointsTandem.createTandem( 'longTeethPoints' ), phetioFeatured: true, phetioType: createObservableArray.ObservableArrayIO( Vector2.Vector2IO ), - phetioDocumentation: 'Population data points for bunnies with long teeth' + phetioDocumentation: 'Population data points for bunnies with long teeth', + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true } ); // visibility of each data set, on the graph and data probe Index: main/axon/js/createObservableArray.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/createObservableArray.ts b/main/axon/js/createObservableArray.ts --- a/main/axon/js/createObservableArray.ts (revision 80443ff9f7dd3a45711c013efa5074870f83fc26) +++ b/main/axon/js/createObservableArray.ts (date 1684519280675) @@ -110,9 +110,9 @@ elements: [], tandem: Tandem.OPTIONAL, phetioFeatured: false, - elementAddedEmitterInstrumented: true, - elementRemovedEmitterInstrumented: true, - lengthPropertyInstrumented: true, + elementAddedEmitterInstrumented: false, + elementRemovedEmitterInstrumented: false, + lengthPropertyInstrumented: false, elementAddedEmitterOptions: {}, elementRemovedEmitterOptions: {}, lengthPropertyOptions: {} Index: main/natural-selection/js/common/model/ProportionsModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/natural-selection/js/common/model/ProportionsModel.ts b/main/natural-selection/js/common/model/ProportionsModel.ts --- a/main/natural-selection/js/common/model/ProportionsModel.ts (revision b54b163b9c65d9c00f4c504ed64f9bde9b1d0ebb) +++ b/main/natural-selection/js/common/model/ProportionsModel.ts (date 1684519081568) @@ -124,7 +124,10 @@ tandem: options.tandem.createTandem( 'previousCounts' ), phetioFeatured: true, phetioType: createObservableArray.ObservableArrayIO( ProportionsCounts.ProportionsCountsIO ), - phetioDocumentation: 'Start and End counts for previous generations, indexed by generation number' + phetioDocumentation: 'Start and End counts for previous generations, indexed by generation number', + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true } ); this.hasDataProperty = new DerivedProperty( Index: main/circuit-construction-kit-common/js/model/Circuit.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/circuit-construction-kit-common/js/model/Circuit.ts b/main/circuit-construction-kit-common/js/model/Circuit.ts --- a/main/circuit-construction-kit-common/js/model/Circuit.ts (revision 43cf0dfef5f5d035c983551b85b675f84170139b) +++ b/main/circuit-construction-kit-common/js/model/Circuit.ts (date 1684518599459) @@ -193,6 +193,9 @@ tandem: tandem.createTandem( 'circuitElements' ), phetioDocumentation: 'All Circuit Elements, used for state save/restore', phetioFeatured: true, + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true, + lengthPropertyInstrumented: true, elementAddedEmitterOptions: { phetioFeatured: true }, Index: main/natural-selection/js/common/model/createBunnyArray.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/natural-selection/js/common/model/createBunnyArray.ts b/main/natural-selection/js/common/model/createBunnyArray.ts --- a/main/natural-selection/js/common/model/createBunnyArray.ts (revision b54b163b9c65d9c00f4c504ed64f9bde9b1d0ebb) +++ b/main/natural-selection/js/common/model/createBunnyArray.ts (date 1684518840965) @@ -38,7 +38,10 @@ // ObservableArrayOptions phetioType: createObservableArray.ObservableArrayIO( ReferenceIO( Bunny.BunnyIO ) ), - phetioState: false + phetioState: false, + lengthPropertyInstrumented: true, + elementAddedEmitterInstrumented: true, + elementRemovedEmitterInstrumented: true }, providedOptions ); // We want to add countsProperty later, so do a little TypeScript hackery here to make that possible. ```

I also updated CCK and NS to keep the APIs the same. However, before committing, we should schedule a short 10 minute meeting with @samreid @zepumph and @pixelzoom to make sure we are in agreement about how to proceed.

samreid commented 1 year ago

See also https://github.com/phetsims/phet-io/issues/1896#issuecomment-1557832042 where @pixelzoom said:

We decided to delete elementAddedEmitterInstrumented, elementRemovedEmitterInstrumented, and lengthPropertyInstrumented.

I ran into other problems with export type ObservableArrayOptions<T>, where PhetioObjectOptions were duplicated. @zepumph volunteeered to take the lead on finishing this off, see patch below.

. . .

zepumph commented 1 year ago

Ok excellent, @pixelzoom will you please review. I updated the options pattern, and removed those options. Please review.

pixelzoom commented 1 year ago

Looks great. Closing.