In mirror inputs wrapper, top particles different from bottom particles #325
Comments
|
I was able to duplicate this on my machine, in Chrome, using the 1.2.0-rc.1 release, through the following fairly simply process:
The two start off in sync, but then diverge over time. Here is a screenshot:
There must still be some usages of unseeded random number generation in the code, or some sensitive dependence on initial conditions going on. |
|
I reviewed this with @kathy-phet and @arouinfar, and they said that the customer for whom the 1.2 release is being created is not planning to use input event mirroring, at least not that they know of. Therefore, this is not blocking for the 1.2 release. I'm not sure how important this is in the phet-io scheme of things, and I'm not sure who to even ask. I'm going to mark this for the phet-io meeting and perhaps we can collectively decide whether this is something I, or someone else, should spend time on. |
|
In phetsims/qa#531, noting that this can lead to a larger problem. |
|
Good to know, and to discuss. Still not blocking for publication at this time. |
|
@jbphet the primary reason for this kind of bug is that something in the sim using random is not tied to phet.joist.random. (which is seeded to be the same in both sims). Since this is platform specific, it feels less likely that this is the exact problem. @jbphet said:
I think that he has a windows machine, and I was able to reproduce on mine using his steps. Thanks for the guide! Looking through the SOM repo, I see that all usages of random are using phet.joist.random. @jbphet you know the sim better than I, can you think of any non-reproducible parts of the particle model that may be causing this deviation? Likely not since this sim has been code-reviewed, but it is my primary thought on the cause of this bug. |
|
@samreid do you have any other ideas on why these may be getting off? |
This feels like an important bug to me. Without reproducible playbacks we lose a large feature of this sim, and publishing it without this feature will not be tracked (see https://github.com/phetsims/phet-io/issues/1609) and it is likely that we will forget about this, and potentially try to use this for a data study in the future. |
|
At today's meeting, @kathy-phet and @jbphet indicated this may need to be fixed after the initial 1.0 release. |
|
|
In another issue, Steele found the restore lid button or something similar also appears a few frames off in time in the playback. Maybe related?
…Sent from my iPhone
On Aug 20, 2020, at 8:18 PM, Sam Reid ***@***.***> wrote:
I tried setting up a unit test, creating two models with identical Random seeds and running them side-by-side, and that gave the same results.
I tried setting the frame sizes to be the same width, and saw the same problem
I printed out the dts and found the downstream sim is running a few dts before it starts processing the upstream dts. This could be throwing it out of whack.
I tried starting the model paused, and that seemed to fix the problem
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
This patch appears to have reproducible playbacks, but starts the model paused: Index: js/common/model/MultipleParticleModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/model/MultipleParticleModel.js (revision ba4746a10c695b35de1cf3a0bcc77db642b20fd4)
+++ js/common/model/MultipleParticleModel.js (date 1597983534925)
@@ -26,6 +26,7 @@
import EnumerationProperty from '../../../../axon/js/EnumerationProperty.js';
import NumberProperty from '../../../../axon/js/NumberProperty.js';
import ObservableArray from '../../../../axon/js/ObservableArray.js';
+import Random from '../../../../dot/js/Random.js';
import Range from '../../../../dot/js/Range.js';
import Utils from '../../../../dot/js/Utils.js';
import Vector2 from '../../../../dot/js/Vector2.js';
@@ -133,6 +134,8 @@
tandem: tandem,
phetioType: MultipleParticleModelIO
} );
+ // this.random = new Random( { seed: 123 } );
+ this.random = phet.joist.random;
//-----------------------------------------------------------------------------------------------------------------
// observable model properties
@@ -176,7 +179,7 @@
} );
// @public (read-write)
- this.isPlayingProperty = new BooleanProperty( true, {
+ this.isPlayingProperty = new BooleanProperty( false, {
tandem: tandem.createTandem( 'isPlayingProperty' )
} );
@@ -647,15 +650,16 @@
'injection attempted when there is no room in the data set'
);
+ const random = this.random;
// Choose an injection angle with some amount of randomness.
- const injectionAngle = ( phet.joist.random.nextDouble() - 0.5 ) * INJECTED_MOLECULE_ANGLE_SPREAD;
+ const injectionAngle = ( random.nextDouble() - 0.5 ) * INJECTED_MOLECULE_ANGLE_SPREAD;
// Set the molecule's velocity.
const xVel = Math.cos( injectionAngle ) * INJECTED_MOLECULE_SPEED;
const yVel = Math.sin( injectionAngle ) * INJECTED_MOLECULE_SPEED;
// Set the rotational velocity to a random value within a range (will be ignored for single atom cases).
- const moleculeRotationRate = ( phet.joist.random.nextDouble() - 0.5 ) * ( Math.PI / 4 );
+ const moleculeRotationRate = ( random.nextDouble() - 0.5 ) * ( Math.PI / 4 );
// Set the position(s) of the atom(s).
const atomsPerMolecule = this.moleculeDataSet.atomsPerMolecule;
@@ -679,7 +683,7 @@
// randomize the rotational angle of multi-atom molecules
this.moleculeDataSet.moleculeRotationAngles[ this.moleculeDataSet.getNumberOfMolecules() - 1 ] =
- phet.joist.random.nextDouble() * 2 * Math.PI;
+ random.nextDouble() * 2 * Math.PI;
}
// Position the atoms that comprise the molecules.
@@ -726,7 +730,7 @@
case SubstanceType.WATER:
this.scaledAtoms.push( new ScaledAtom( AtomType.OXYGEN, 0, 0 ) );
this.scaledAtoms.push( new HydrogenAtom( 0, 0, true ) );
- this.scaledAtoms.push( new HydrogenAtom( 0, 0, phet.joist.random.nextDouble() > 0.5 ) );
+ this.scaledAtoms.push( new HydrogenAtom( 0, 0, this.random.nextDouble() > 0.5 ) );
break;
default:
@@ -827,6 +831,7 @@
* @public
*/
stepInTime( dt ) {
+ console.log( window.phet.chipper.queryParameters.frameTitle + ', dt = ' + dt );
this.moleculeInjectedThisStep = false;
@@ -1089,7 +1094,7 @@
this.atomPositionUpdater = DiatomicAtomPositionUpdater;
this.moleculeForceAndMotionCalculator = new DiatomicVerletAlgorithm( this );
this.isoKineticThermostat = new IsokineticThermostat( this.moleculeDataSet, this.minModelTemperature );
- this.andersenThermostat = new AndersenThermostat( this.moleculeDataSet, this.minModelTemperature );
+ this.andersenThermostat = new AndersenThermostat( this.moleculeDataSet, this.minModelTemperature, this.random );
const numberOfMolecules = numberOfAtoms / 2;
const atomPositionInVector = new Vector2( 0, 0 );
@@ -1145,7 +1150,7 @@
this.atomPositionUpdater = WaterAtomPositionUpdater;
this.moleculeForceAndMotionCalculator = new WaterVerletAlgorithm( this );
this.isoKineticThermostat = new IsokineticThermostat( this.moleculeDataSet, this.minModelTemperature );
- this.andersenThermostat = new AndersenThermostat( this.moleculeDataSet, this.minModelTemperature );
+ this.andersenThermostat = new AndersenThermostat( this.moleculeDataSet, this.minModelTemperature, this.random );
// Create the individual atoms and add them to the data set.
const atomPositionInVector = new Vector2( 0, 0 );
@@ -1222,7 +1227,7 @@
this.atomPositionUpdater = MonatomicAtomPositionUpdater;
this.moleculeForceAndMotionCalculator = new MonatomicVerletAlgorithm( this );
this.isoKineticThermostat = new IsokineticThermostat( this.moleculeDataSet, this.minModelTemperature );
- this.andersenThermostat = new AndersenThermostat( this.moleculeDataSet, this.minModelTemperature );
+ this.andersenThermostat = new AndersenThermostat( this.moleculeDataSet, this.minModelTemperature, this.random );
// Create the individual atoms and add them to the data set.
const atomPositions = [];
Index: js/common/model/engine/MonatomicPhaseStateChanger.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/model/engine/MonatomicPhaseStateChanger.js (revision ba4746a10c695b35de1cf3a0bcc77db642b20fd4)
+++ js/common/model/engine/MonatomicPhaseStateChanger.js (date 1597983118654)
@@ -25,7 +25,7 @@
constructor( multipleParticleModel ) {
super( multipleParticleModel );
this.positionUpdater = MonatomicAtomPositionUpdater;
- this.random = phet.joist.random;
+ this.random = multipleParticleModel.random;
}
/**
Index: js/common/model/engine/kinetic/AndersenThermostat.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/model/engine/kinetic/AndersenThermostat.js (revision ba4746a10c695b35de1cf3a0bcc77db642b20fd4)
+++ js/common/model/engine/kinetic/AndersenThermostat.js (date 1597982494964)
@@ -25,7 +25,7 @@
* @param {MoleculeForceAndMotionDataSet} moleculeDataSet - Data set on which operations will be performed.
* @param {number} minTemperature - The temperature that should be considered absolute zero, below which motion should cease
*/
- constructor( moleculeDataSet, minTemperature ) {
+ constructor( moleculeDataSet, minTemperature,random ) {
// @public target temperature in normalized model units
this.targetTemperature = SOMConstants.INITIAL_TEMPERATURE;
@@ -37,7 +37,7 @@
this.moleculeDataSet = moleculeDataSet;
// @private - pseudo-random number generator
- this.random = phet.joist.random;
+ this.random = random;
// @private {Vector2} - reusable vector used for calculating velocity changes
this.previousParticleVelocity = new Vector2( 0, 0 );
Index: js/common/model/engine/AbstractPhaseStateChanger.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/model/engine/AbstractPhaseStateChanger.js (revision ba4746a10c695b35de1cf3a0bcc77db642b20fd4)
+++ js/common/model/engine/AbstractPhaseStateChanger.js (date 1597982494968)
@@ -29,7 +29,7 @@
// @private
this.multipleParticleModel = multipleParticleModel;
this.moleculePosition = new Vector2( 0, 0 );
- this.random = phet.joist.random;
+ this.random = multipleParticleModel.random;
this.reusableVector = new Vector2( 0, 0 );
}
|
|
This patch shows that the random number generator is being called a different number of times in the upstream and downstream simulations: Index: js/Random.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Random.js (revision 60312e5cc6023257d4734a61001edbd439685fc2)
+++ js/Random.js (date 1597983759969)
@@ -135,6 +135,10 @@
* @returns {number} - the random number
*/
nextDouble() {
+
+ window.randCount = window.randCount || 0;
+ window.randCount++;
+ console.log( phet.preloads.phetio.queryParameters.frameTitle, window.randCount );
return this.seed === null ? Math.random() : this.seedrandom(); // eslint-disable-line bad-sim-text
}
|
|
Using the above patch that counts random calls, I confirmed that putting I should also mention many sightings of where the destination value prints out before the source value does:
|
|
This patch appears to resolve the problem, but I am confused why, especially after viewing the list of listeners in the Index: js/Sim.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Sim.js (revision e10aa8a8701e863ac52d050e6c3071c802f65d10)
+++ js/Sim.js (date 1597986974133)
@@ -879,9 +879,7 @@
// If the sim is in playback mode, then run a single frame. This makes sure that anything kicked to the next
// frame with timer.runOnNextFrame during startup can clear out before beginning playback.
if ( phet.joist.playbackModeEnabledProperty.value ) {
-
- // Mark this as a "root" event so that events caused by this don't duplicate in the playback
- self.stepOneFrame();
+ timer.emit( 0 );
}
// After the application is ready to go, remove the splash screen and progress bar. Note the splash
|
|
We are close to a commit! I have to run though: Index: dot/js/Random.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- dot/js/Random.js (revision 60312e5cc6023257d4734a61001edbd439685fc2)
+++ dot/js/Random.js (date 1598028641316)
@@ -46,6 +46,11 @@
// Math.seedrandom is provided by seedrandom.js, see https://github.com/davidbau/seedrandom.
// @private {function:number|null} initialized via setSeed below
this.seedrandom = ( this.seed !== null ) ? new Math.seedrandom( this.seed + '' ) : null;
+
+ // @public (read-only)
+ this.randCount = 0;
+
+ Random.allRandomInstances.push( this );
}
/**
@@ -135,6 +140,10 @@
* @returns {number} - the random number
*/
nextDouble() {
+ if( window.blarg){
+ debugger;
+ }
+ this.randCount++;
return this.seed === null ? Math.random() : this.seedrandom(); // eslint-disable-line bad-sim-text
}
@@ -182,6 +191,8 @@
}
}
+Random.allRandomInstances = [];
+
dot.register( 'Random', Random );
export default Random;
\ No newline at end of file
Index: phet-io-wrappers/mirror-inputs/mirrorInputs.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- phet-io-wrappers/mirror-inputs/mirrorInputs.js (revision 3e8368293628108337895ec1b3d8e011bbc8b7ff)
+++ phet-io-wrappers/mirror-inputs/mirrorInputs.js (date 1598027457495)
@@ -44,6 +44,14 @@
// Clickable only once initialized, see https://github.com/phetsims/phet-io-wrappers/issues/359
sourceClient.addSimInitializedListener( () => {
sourceFrame.classList.remove( 'unclickable' );
+
+ assert && setTimeout( () => {
+ sourceClient.invoke( 'phetioEngine', 'getRandomSeed', [], sourceRandomSeed => {
+ destinationClient.invoke( 'phetioEngine', 'getRandomSeed', [], destinationRandomSeed => {
+ assert && assert( sourceRandomSeed === destinationRandomSeed, 'random seeds must match' );
+ } );
+ } );
+ }, 10 );
} );
// Cannot use phetioClient.launchSim because we need to launch sim 1 to get its seed, then launch sim 2 to set the seed, and
Index: joist/js/Sim.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/Sim.js (revision e10aa8a8701e863ac52d050e6c3071c802f65d10)
+++ joist/js/Sim.js (date 1598030075100)
@@ -22,6 +22,7 @@
import timer from '../../axon/js/timer.js';
import Bounds2 from '../../dot/js/Bounds2.js';
import Dimension2 from '../../dot/js/Dimension2.js';
+import Random from '../../dot/js/Random.js';
import DotUtils from '../../dot/js/Utils.js';
import inherit from '../../phet-core/js/inherit.js';
import merge from '../../phet-core/js/merge.js';
@@ -42,16 +43,16 @@
import Heartbeat from './Heartbeat.js';
import HomeScreen from './HomeScreen.js';
import HomeScreenView from './HomeScreenView.js';
+import joist from './joist.js';
+import joistStrings from './joistStrings.js';
import LookAndFeel from './LookAndFeel.js';
import MemoryMonitor from './MemoryMonitor.js';
import NavigationBar from './NavigationBar.js';
+import packageJSON from './packageJSON.js';
import Profiler from './Profiler.js';
import QueryParametersWarningDialog from './QueryParametersWarningDialog.js';
import ScreenIO from './ScreenIO.js';
import ScreenshotGenerator from './ScreenshotGenerator.js';
-import joist from './joist.js';
-import joistStrings from './joistStrings.js';
-import packageJSON from './packageJSON.js';
import selectScreens from './selectScreens.js';
import LegendsOfLearningSupport from './thirdPartySupport/LegendsOfLearningSupport.js';
import updateCheck from './updateCheck.js';
@@ -876,12 +877,15 @@
// Schedules animation updates and runs the first step()
self.boundRunAnimationLoop();
- // If the sim is in playback mode, then run a single frame. This makes sure that anything kicked to the next
- // frame with timer.runOnNextFrame during startup can clear out before beginning playback.
+ // If the sim is in playback mode, then flush the timer's listeners. This makes sure that anything kicked
+ // to the next frame with `timer.runOnNextFrame` during startup (like every notification about a PhET-iO
+ // instrumented element in phetioEngine.phetioObjectAdded()) can clear out before beginning playback.
if ( phet.joist.playbackModeEnabledProperty.value ) {
-
- // Mark this as a "root" event so that events caused by this don't duplicate in the playback
- self.stepOneFrame();
+ const beforeCounts = Random.allRandomInstances.map( n => n.randCount );
+ self.timer.emit( 0 );
+ const afterCounts = Random.allRandomInstances.map( n => n.randCount );
+ assert && assert( _.isEqual( beforeCounts, afterCounts ),
+ `Random was called more times in the playback sim on startup, before: ${beforeCounts}, after: ${afterCounts}` );
}
// After the application is ready to go, remove the splash screen and progress bar. Note the splash
Index: joist/js/Heartbeat.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/Heartbeat.js (revision e10aa8a8701e863ac52d050e6c3071c802f65d10)
+++ joist/js/Heartbeat.js (date 1598030183256)
@@ -13,6 +13,9 @@
// variables
let started = false;
+// a unique number to make sure safari doesn't get sleepy, see usage.
+let counter = 0;
+
const Heartbeat = {
/**
@@ -41,7 +44,7 @@
// prevent Safari from going to sleep, see https://github.com/phetsims/joist/issues/140
sim.frameStartedEmitter.addListener( function() {
if ( sim.frameCounter % 1000 === 0 ) {
- heartbeatDiv.innerHTML = phet.joist.random.nextDouble();
+ heartbeatDiv.innerHTML = ++counter;
}
} );
}
|
|
@samreid @zepumph and I worked on this. We ended up finding the source of the mismatched number of calls to Random.nextDouble() - it was the downstream sim using its first random number for Heartbeat.js, which was slightly earlier than the upstream was. This causes the sims to start using random number that were misaligned by one. To fix, we used a different method for writing unique content every 1000 frames, which resulted in a matching number of calls to Random in both sims. We also are now tracking the number of random calls from both sims and are asserting that they stay in sync. Regardless, we kept the proposed fix in from #325 (comment) since it seemed safer. @jbphet the following commits are ready to be cherry-picked: phetsims/joist@1ffb7b6 |
|
Unassigning until the maintenance release is designated as one of my priorities. |
Test Device
MacBook Air
Operating System
macOS 10.15.5
Browser
Safari 13.1.1
Problem Description
For phetsims/qa#525. This bug seems to be platform-specific. That is, I don't see this bug on macOS + Firefox or macOS + Chrome. In the mirror inputs wrapper, the particles in the top iframe have different positions/velocities from the particles in the bottom iframe. Easy to reproduce; just mess around with the heater in the states screen. The two iframes should diverge quickly.
Visuals
See, I'm not crazy:
The text was updated successfully, but these errors were encountered: