Sometimes ammeter doesn't disappear in State Wrapper

[Nancy-Salpepi]

Test device MacBook Air M1 chip and iPad 9th generation

Operating System 13.1 and iOS 16.3

Browser Safari

Problem description For https://github.com/phetsims/qa/issues/900, in the State Wrapper, pressing 'Set State Now' doesn't always remove the ammeter in the downstream sim.

Steps to reproduce

1. Change Set State Rate to 0
2. In the upstream sim, create a simple circuit (ex. switch, wires, battery, bulb) on the intro screen
3. Close the circuit and attach an ammeter
4. Press Set State Now
5. In the Upstream Sim, press Reset All
6. Press Set State Now --the circuit is gone, but the ammeter is still there and it still is detecting current

Visuals

https://user-images.githubusercontent.com/87318828/220198621-4539ce9e-0d64-4fa3-b1bf-54b0ca5e90d5.mp4

[Nancy-Salpepi]

with phetioDebug=true, I see this assertion error:

[matthew-blackman]

@samreid suggested that we can likely fix this by implementing the same solution that we used for the Voltmeter, by turning it into a one-way stateful probe.

[samreid]

Here is a working patch that tells the ammeter readout to behave like a DerivedProperty (do not try to load the state). It works in testing, but it is unfortunate to lose the valueType: DerivedProperty part. @zepumph can you please advise?

Subject: [PATCH] Update credits, see https://github.com/phetsims/qa/issues/900
---
Index: main/circuit-construction-kit-common/js/model/Ammeter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-common/js/model/Ammeter.ts b/main/circuit-construction-kit-common/js/model/Ammeter.ts
--- a/main/circuit-construction-kit-common/js/model/Ammeter.ts  (revision 8eb8c7acd3c6bcce277b75ee9f22339f51625620)
+++ b/main/circuit-construction-kit-common/js/model/Ammeter.ts  (date 1677010731548)
@@ -18,6 +18,7 @@
 import AmmeterConnection from './AmmeterConnection.js';
 import CircuitElement from './CircuitElement.js';
 import ReferenceIO from '../../../tandem/js/types/ReferenceIO.js';
+import DerivedProperty from '../../../axon/js/DerivedProperty.js';

 export default class Ammeter extends Meter {

@@ -49,6 +50,7 @@
       phetioFeatured: true,
       phetioValueType: NullableIO( ReferenceIO( CircuitElement.CircuitElementIO ) ),
       phetioReadOnly: true,
+      phetioOuterType: DerivedProperty.DerivedPropertyIO,
       phetioDocumentation: 'The circuit element that the ammeter is connected to, or null if not connected to a circuit element'
     } );
   }
Index: main/axon/js/DerivedProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/DerivedProperty.ts b/main/axon/js/DerivedProperty.ts
--- a/main/axon/js/DerivedProperty.ts   (revision ecf09b3e5fca83283346330540d62836fd796b46)
+++ b/main/axon/js/DerivedProperty.ts   (date 1677010766457)
@@ -256,7 +256,7 @@

   if ( !cache.has( parameterType ) ) {
     cache.set( parameterType, new IOType( `${DERIVED_PROPERTY_IO_PREFIX}<${parameterType.typeName}>`, {
-      valueType: DerivedProperty,
+      // valueType: DerivedProperty,
       parameterTypes: [ parameterType ],
       supertype: Property.PropertyIO( parameterType ),
       documentation: 'Like PropertyIO, but not settable.  Instead it is derived from other DerivedPropertyIO or PropertyIO ' +

[zepumph]

You could also mark it as phetioState: false right?

When are we going to pull the trigger and make a new metadata flag called phetioSerializable for things that should not be settable?

We could also make a new IOType that wraps another IOType, but overwrites fromStateObject and applyState with a stub. That is a bit lame, but kinda what we want here.

We could also change the valueType: DerivedProperty to valueType: ReadOnlyProperty

[zepumph]

Also, why are we getting an unset state error? Shouldn't wire1 still exist as part of setting state?

Also, can we just opt out of calling updateAmmeter in the Node when setting state? This is likely more on the workaround side.

Let's talk!

[zepumph]

Ok. I think all the above are a bit downstream of the issue at hand. Here is what I see going on:

1. Go to studio, look up circuitConstructionKitDc.introScreen.model.meters.ammeter1.probeConnectionProperty, and pull it out and put the ammeter on something.
2. Note that the value of probeConnectionProperty says the light bulb/wire/etc.
3. Press reset all
4. Note that the probeConnectionProperty value is the same bulb etc that doesn't exist anymore.

In addition to this likely being a memory leak, it is causing this bug too.

[zepumph]

This code added to the Ammeter constructor worked well. Over to @samreid to do what is best here.

    this.isActiveProperty.link( ()=>{
      this.probeConnectionProperty.value = null;
    })

[samreid]

Good fix, thanks! I did the same thing in Voltmeter. I checked if that allowed me to remove VoltageConnectionIO.fromStateObject but I was unable to. I think everything is OK. My main concern is if the sim is loaded into a different aspect ratio then you will have to wait 1 frame for the ammeter sensed value to update. But I'm not too worried about it. But if we are worried about it, the solution would be to prevent it from loading in using one of the approaches listed above.

@Nancy-Salpepi can you please test in master?

@zepumph can you please review and comment?

[Nancy-Salpepi]

I was able to confirm this is fixed in master with mac + chrome. However, with mac + safari I can't open state--it doesn't get to the splash screen and I see these errors. Noting that I updated my macOS this morning to 13.2.1.

[samreid]

@matthew-blackman and I were not able to reproduce this locally or on phettest. On slack @Nancy-Salpepi is reporting other issues on Safari that may be local to that installation. So we think this issue can be closed. Let's reopen if this issue is discovered in RC.1.

[Nancy-Salpepi]

It was definitely my computer. Images were disabled somehow. Sorry for any trouble @samreid @matthew-blackman!