Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sometimes ammeter doesn't disappear in State Wrapper #974

Closed
Nancy-Salpepi opened this issue Feb 20, 2023 · 11 comments
Closed

Sometimes ammeter doesn't disappear in State Wrapper #974

Nancy-Salpepi opened this issue Feb 20, 2023 · 11 comments

Comments

@Nancy-Salpepi
Copy link

Nancy-Salpepi commented Feb 20, 2023

Test device
MacBook Air M1 chip and iPad 9th generation

Operating System
13.1 and iOS 16.3

Browser
Safari

Problem description
For phetsims/qa#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

ammeteronResetAll.mp4
@Nancy-Salpepi
Copy link
Author

with phetioDebug=true, I see this assertion error:
Screenshot 2023-02-20 at 4 24 54 PM

@matthew-blackman
Copy link

@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
Copy link
Member

samreid commented Feb 21, 2023

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 ' +

@samreid samreid assigned zepumph and unassigned samreid Feb 21, 2023
@zepumph
Copy link
Member

zepumph commented Feb 21, 2023

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
Copy link
Member

zepumph commented Feb 21, 2023

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
Copy link
Member

zepumph commented Feb 21, 2023

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
Copy link
Member

zepumph commented Feb 21, 2023

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
Copy link
Member

samreid commented Feb 22, 2023

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
Copy link
Author

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.
Screenshot 2023-02-22 at 10 09 35 AM

@samreid
Copy link
Member

samreid commented Feb 22, 2023

@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.

@samreid samreid closed this as completed Feb 22, 2023
@Nancy-Salpepi
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants