zepumph
commented
1 year ago Closed zepumph closed 10 months ago
zepumph
commented
1 year ago 
samreid
commented
1 year ago I was able to trigger this by fuzzing circuit-constrcution-kit-virtual-lab in the migration wrapper. It is pretty rare though. But it still shows on CT. The outer phetioID selectionProperty still exists downstream, so it must be a problem with the referenceIO. Probably would be good to get help from @zepumph
samreid
commented
1 year ago I was able to trigger this in the state wrapper, and by using this snippet:
this.window.parent.document.getElementById('source').contentWindow.phet.phetio.phetioEngine.phetioObjectMap['circuitConstructionKitDc.introScreen.model.circuit.vertexGroup.vertex_2']I was able to see that vertex_2 doesn't exist in either the upstream or downstream simulation. Other vertices, such as vertex_4 do exist in both. So perhaps we need an earlier assertion that a captured state doesn't reflect that a non-existent vertex is selected. Or perhaps when a vertex is deleted, make sure it is not selected.
samreid
commented
1 year ago This patch moves the problem to the upstream sim, detecting when a non-existent vertex is selected:
Subject: [PATCH] Apply no-strict mode to another pre-hydrogen sim, see https://github.com/phetsims/phet-io-wrappers/issues/547
---
Index: js/PhetioStateEngine.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/PhetioStateEngine.ts b/js/PhetioStateEngine.ts
--- a/js/PhetioStateEngine.ts (revision 3f1cced78a6cd74ea313e537148362304e2b738c)
+++ b/js/PhetioStateEngine.ts (date 1695238719550)
@@ -169,6 +169,19 @@
state[ phetioID ] = this.getValueJSON( phetioID );
}
} );
+
+ if ( state[ 'circuitConstructionKitDc.introScreen.model.circuit.selectionProperty' ] &&
+ state[ 'circuitConstructionKitDc.introScreen.model.circuit.selectionProperty' ].value !== null ) {
+ const elm = state[ 'circuitConstructionKitDc.introScreen.model.circuit.selectionProperty' ].value.phetioID;
+ console.log( elm );
+
+ const elmState = state[ elm ];
+ console.log( elmState );
+ assert && assert( elmState, 'vertex could not be found: ' + elm, state[ elm ] );
+ }
+ // console.log( state[ 'circuitConstructionKitDc.introScreen.model.circuit.selectionProperty' ] );
+ // state[ 'circuitConstructionKitDc.introScreen.model.circuit.vertexGroup.vertex_21' ]
+
return state;
}
I found that the VertexNode drag listener end was being triggered on disposal, and being close enough to the last mouse click was causing it to become marked as selected. So I added a guard to not select during disposal.
samreid
commented
1 year ago This looks fixed in CT, nice work, team!
zepumph
commented
11 months ago I'm still seeing this in recent rounds of CT:
samreid
commented
11 months ago I wrote this patch to detect if we ever get a state that has a non-existent selected vertex.
However, the problem is that the inconsistent state is in the old version. I believe the changes above solve it in main but the reason we still have a CT bug is that it didn't get maintenance released back into the prior version, which is still running during migration.
Ways forward:
@zepumph what would you recommend?
zepumph
commented
11 months ago My first preference would be to solve through migration. It seems easiest and cheapest. Wanna start there?
samreid
commented
11 months ago OK I pushed a proposal to test dc (but not dc-virtual-lab). So let's check after a few columns and see how it looks.
UPDATE: Actually the change is in common code shared by both, so we can't use the "is dc fixed but virtual lab still failing" as a verification step.
samreid
commented
11 months ago OK checking CT with maxColumns=40. I see:
The Fisher's Exact Test yields a p-value of approximately 0.0712. So we would need a few more cells to get below a 0.05 (if that is what we are aiming for). But so far, so good.
@zepumph can you please review? We can check this synchronously if you would like.
zepumph
commented
10 months ago @samreid and I went over this Processor today, and it makes sense to me. We ran into the same kind of problem with https://github.com/phetsims/phet-io-wrappers/issues/607, but your sim-specific case feels like you solved this perfectly with your processor. Ready to close?
samreid
commented
10 months ago Everything looks great here, thanks! Closing.
@samreid let me know if you would like help, but I don't want to be the first one to poke around your sim.