PhET-iO Instrumented DerivedProperties should link to instrumented Dependencies

[samreid]

From today's phet-io meeting, we discussed that PhET-iO Instrumented DerivedProperties should link to instrumented Dependencies. This will help track back across all kinds of Properties, including the context of derived StringProperties for text.

[samreid]

Here's a working Prototype that has several good examples in Geometric Optics:

```diff Index: js/DerivedProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/DerivedProperty.ts b/js/DerivedProperty.ts --- a/js/DerivedProperty.ts (revision c0be2ed0c8e9e1668fbbed4d086e17d509b1e1e7) +++ b/js/DerivedProperty.ts (date 1660597242009) @@ -20,6 +20,7 @@ import optionize, { EmptySelfOptions } from '../../phet-core/js/optionize.js'; import { Dependencies, RP1, RP10, RP11, RP12, RP13, RP14, RP15, RP2, RP3, RP4, RP5, RP6, RP7, RP8, RP9 } from './Multilink.js'; import ReadOnlyProperty from './ReadOnlyProperty.js'; +import PhetioObject from '../../tandem/js/PhetioObject.js'; // constants const DERIVED_PROPERTY_IO_PREFIX = 'DerivedPropertyIO'; @@ -110,6 +111,12 @@ // @ts-ignore propertyStateHandlerSingleton.registerPhetioOrderDependency( dependency, PropertyStatePhase.UNDEFER, this, PropertyStatePhase.UNDEFER ); } + + if ( this.isPhetioInstrumented() && dependency instanceof PhetioObject && dependency.isPhetioInstrumented() ) { + this.addLinkedElement( dependency, { + tandem: options.tandem.createTandem( 'dependency,' + dependency.tandem.phetioID.split( '.' ).join( ',' ) ) + } ); + } } ); } ```

[samreid]

My first draft named them as dependency1 etc, but that didn't seem stable, so next I tried naming the child dependencies based on the dependency tandems. I used the same workaround @jonathanolson demonstrated by joining with commas. When I tried creating nested tandems, it looked awkward.

[zepumph]

We had good luck with this! @samreid and I decided it would look cool to convert then entire phetioID of the dependency and replace the . for - and then next each under a dependencies tandem name. It is working well! We had to leave before I felt good enough for a commit. We should decide if this is the right of implementation detail to expose. We have migration rules that can support changes in this, but perhaps it is dangerous. We also experimented with only adding this for string properties, but that felt like code that didn't belong in DerivedProperty.

```diff Index: axon/js/DerivedProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/DerivedProperty.ts b/axon/js/DerivedProperty.ts --- a/axon/js/DerivedProperty.ts (revision 4e0e958b0822c0d615bf561539006fb8ea66e3f6) +++ b/axon/js/DerivedProperty.ts (date 1660675003157) @@ -20,8 +20,8 @@ import optionize, { EmptySelfOptions } from '../../phet-core/js/optionize.js'; import { Dependencies, RP1, RP10, RP11, RP12, RP13, RP14, RP15, RP2, RP3, RP4, RP5, RP6, RP7, RP8, RP9 } from './Multilink.js'; import ReadOnlyProperty from './ReadOnlyProperty.js'; +import PhetioObject from '../../tandem/js/PhetioObject.js'; -// constants const DERIVED_PROPERTY_IO_PREFIX = 'DerivedPropertyIO'; type SelfOptions = EmptySelfOptions; @@ -110,6 +110,14 @@ // @ts-ignore propertyStateHandlerSingleton.registerPhetioOrderDependency( dependency, PropertyStatePhase.UNDEFER, this, PropertyStatePhase.UNDEFER ); } + + if ( this.isPhetioInstrumented() && dependency instanceof PhetioObject && dependency.isPhetioInstrumented() ) { + const dependenciesTandem = options.tandem.createTandem( 'dependencies' ); + + this.addLinkedElement( dependency, { + tandem: dependenciesTandem.createTandemFromPhetioID( dependency.tandem.phetioID ) + } ); + } } ); } Index: tandem/js/Tandem.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/Tandem.ts b/tandem/js/Tandem.ts --- a/tandem/js/Tandem.ts (revision 7b3fcb65fe7d3a4b2f4e47da62bb0c713fa3a4f5) +++ b/tandem/js/Tandem.ts (date 1660674967538) @@ -138,7 +138,7 @@ * characters or tildes. */ protected getTermRegex(): RegExp { - return /^[a-zA-Z0-9[\],]+$/; + return /^[a-zA-Z0-9[\],-]+$/; } /** @@ -401,6 +401,10 @@ // a list of PhetioObjects ready to be sent out to listeners, but can't because Tandem hasn't been launched yet. public static bufferedPhetioObjects: PhetioObject[] = []; + + public createTandemFromPhetioID( phetioID: string ): Tandem { + return this.createTandem( phetioID.split( window.phetio.PhetioIDUtils.SEPARATOR ).join( '-' ) ); + } } Tandem.addLaunchListener( () => {

[samreid]

I'll take the lead on this.

[samreid]

I committed an implementation for this which seems to be working well. I almost abandoned it by considering that we don't want dependencies to end up in the API, but since they don't seem that operational (i.e. clients should not call phetioInvoke on linkedElements, perhaps it is OK. If it causes API tracking headaches for detecting differences, we can re-evaluate. @zepumph can you please review?

[zepumph]

Seems reasonable to me, but don't we think that at some point LinkedElements will forward methods to their core elements?

[samreid]

This proposal is failing when a dynamic element appears as a dependency. In CCK DC studio, drag out a resistor:

Assertion failed: PhetioObject not found for circuitConstructionKitDc.introScreen.model.circuit.resistorGroup.archetype.powerDissipatedProperty.dependencies.circuitConstructionKitDc-introScreen-model-circuit-resistorGroup-resistor_0-resistanceProperty

It is looking for one with a dependency on resistor_0 in the archetype. Possibly it needs to look for one here??

circuitConstructionKitDc.introScreen.model.circuit.resistorGroup.archetype.powerDissipatedProperty.dependencies.circuitConstructionKitDc-introScreen-model-circuit-resistorGroup-archetype-resistanceProperty

[samreid]

In discovering this problem and writing up a working (but non-robust) solution, I considered whether we should abandon this feature, or opt-in to it. My current sense is that if we see a few additional problems of that nature we may need to abandon. But I'm ok to continue for now, but it would be great for @zepumph to review that problem and commit.

[samreid]

Today @kathy-phet said that allowing LinkedElements to forward API calls would create too much API burden and complexity for migration support. We do not have support for that but may reconsider it if it is important for a PhET-iO client.

@zepumph describes that this is a simpler kind of API change since it is only a pointer that clients cannot call. But changing these linked elements cannot break a client wrapper API calls. So yes, there will be more API changes, but they are safe.

@kathy-phet asks about the burden of identifying these cases in contrast to other actual breaking API changes. We want to make sure devs aren't burdened with having to worry about changing things in this area.

@zepumph describes that we need to start distinguishing between "all API changes" vs "breaking API changes", and that it would be OK to make API changes in a maintenance releases as long as it is non-breaking.

@kathy-phet and @arouinfar agreed this feature is useful for strings and may be helpful or at worst "uninteresting" for other derived properties.

@kathy-phet expressed concern: Are we free to change these implementation details without breaking things? But since it doesn't appear in the state or data stream or API calls, it seems safe.