CT: sampleMeanProperty should not be null in showingCompleteSampleWithoutMean phase #329
Comments
|
Experimental patch: Subject: [PATCH] Only consider landeded projectiles for determining if a sample is complete, see https://github.com/phetsims/projectile-data-lab/issues/329
---
Index: js/sampling/model/SamplingField.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/sampling/model/SamplingField.ts b/js/sampling/model/SamplingField.ts
--- a/js/sampling/model/SamplingField.ts (revision 4d9eec05c31fd71b49a92004e4f6cc5e2c344b66)
+++ b/js/sampling/model/SamplingField.ts (date 1715707819562)
@@ -177,6 +177,10 @@
this.updateComputedProperties();
} );
+ this.projectileLandedEmitter.addListener( () => {
+ this.updateComputedProperties();
+ } );
+
Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => {
this.updateComputedProperties();
} );
@@ -210,7 +214,7 @@
this.numberOfStartedSamplesProperty.value - 1;
// Update the sample mean
- const projectilesInSelectedSample = this.getProjectilesInSelectedSample();
+ const projectilesInSelectedSample = this.getLandedProjectilesInSelectedSample();
// This multilink is called during transient intermediate phases, so we must guard and make sure we truly have a complete sample
const isComplete = this.phaseProperty.value === 'showingCompleteSampleWithMean' && projectilesInSelectedSample.length === this.sampleSize; |
|
This is pretty rare on CT. Showing once in the last 20x2 = 40 columns (2x multiplier is for 2x sims). @matthew-blackman and I investigated this on 3 occasions and do not have any significant leads. We do not believe the problem is likely to happen on a client machine, since nothing related to this was discovered during the QA process. We are also skeptical about a significant change to the architecture or logic, as it would risk breaking other behavior and warrant QA retesting. We last discussed that we should invite a consultant (perhaps @zepumph ?) to help us look at it with a fresh perspective. @matthew-blackman and @zepumph how should we proceed? |
|
I don't know the issue and I don't know how to reproduce it, but I have some thoughts! In SamplingField, you have multiple Properties that aren't stateful, one of which is sampleMeanProperty. So at a general level this assertion doesn't surprise me at all, since I don't see any guarantee that sampleMeanProperty looks or behaves appropriately after a state set. I also see that numberOfStartedSamplesProperty is also unstateful. I can only assume that this was purposeful for some good reason, so assuming that, would you like to try out recomputing these non stateful Property values from whatever stateful data we after upon set state completion? My first pass would be this line, but perhaps it will be buggy because it also will change numberOfCompletedSamplesProperty, which is stateful so perhaps shouldn't be changed. Subject: [PATCH] blarg
---
Index: js/sampling/model/SamplingField.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/sampling/model/SamplingField.ts b/js/sampling/model/SamplingField.ts
--- a/js/sampling/model/SamplingField.ts (revision 7a9b72093c43407b527510b427db662700409f45)
+++ b/js/sampling/model/SamplingField.ts (date 1715976241831)
@@ -186,6 +186,10 @@
this.isContinuousLaunchingProperty.value = false;
}
} );
+
+ Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => {
+ this.updateComputedProperties();
+ } );
}
/**Also, I'd like to note that wiring assertions into the stateSetEmitter is @pixelzoom and my favorite new state-testing tool. For example here. Let me know if I can assist any further. |
In both PDL and PSD:
The text was updated successfully, but these errors were encountered: