KatieWoe
commented
3 years ago Closed KatieWoe closed 3 years ago
KatieWoe
commented
3 years ago 
jbphet
commented
3 years ago This is occurring due to the changes to observable arrays that happened under https://github.com/phetsims/axon/issues/336. It is caused because a list of biomolecules is being shuffled, and when that occurs the addItemAddedListeners is being fired for an observable array. This doesn't seem quite correct - it should be possible to shuffle an array without firing this listener, since nothing is really being added. Over to @samreid to comment and to continue the investigation.
samreid
commented
3 years ago Can you please help me identify where this is being called? phet.joist.random.shuffle doesn't mutate the passed-in array. And I don't see "shuffle" in the stack trace.
Additionally createObservableArray overrides shuffle so that it doesn't trigger notifications.
jbphet
commented
3 years ago The call to shuffle is not in the stack traces above because the erroneous state is caused in prior moments. Below is the stack trace that I was seeing where shuffle seemed to be triggering the addItemAddedListeners to fire, which in turn creates an inconsistent state in the model:
addBiomoleculeView (MessengerRnaProductionScreenView.js:184)
(anonymous) (MessengerRnaProductionScreenView.js:230)
emit (TinyEmitter.js:71)
(anonymous) (Emitter.js:35)
execute (Action.js:225)
emit (Emitter.js:60)
set (createObservableArray.js:123)
shuffle (createObservableArray.js:365)
stepInTime (MessengerRnaProductionModel.js:227)
step (MessengerRnaProductionModel.js:205)
(anonymous) (Sim.js:297)
execute (Action.js:225)
stepSimulation (Sim.js:988)
stepOneFrame (Sim.js:978)
(lots of calls to runAnimationLoop and requestAnimationFrame removed)I just looked at createObservableArray.shuffle, and it does indeed look like it clears out the array and then adds things back, which certainly seems like it could trigger addItemAddedListener. Here's the current implementation:
shuffle: function( random ) {
assert && assert( random, 'random must be supplied' );
// preserve the same _array reference in case any clients got a reference to it with getArray()
const shuffled = random.shuffle( this );
this.length = 0;
Array.prototype.push.apply( this, shuffled );
},@samreid - Is this enough for you to go on?
samreid
commented
3 years ago Thanks @jbphet, that was exactly the problem. I added a unit test to check for that, and resolved it in the commit. Fuzzing is lasting much longer for GEE in my testing. Please review and close if all is well.
jbphet
commented
3 years ago Thanks @samreid - the changes seem reasonable to me and CT is clear for this sim. Closing.