"max balls" feature is broken in Lab screen

pixelzoom commented 6 years ago

The sim crashes when the "max balls" limit is reached in the Lab screen. This problem is almost certainly independent of platform, but was noticed on master with Chrome 65 on macOS.

To reproduce:

Run sim with ?maxBallsLab=10 (no additional info with ?ea)
Go to Lab screen
Select radio button for continuous balls
Press green start button.
Sim will crash with this stack trace:

Uncaught TypeError: LabScreenView.js?bust=1521841641354:162
playPanel.setPlayButtonVisible is not a function
    at Array.<anonymous> (LabScreenView.js?bust=1521841641354:162)
    at Emitter.emit2 (Emitter.js?bust=1521841641354:180)
    at Property._notifyListeners (Property.js?bust=1521841641354:282)
    at Property._setAndNotifyListeners (Property.js?bust=1521841641354:269)
    at Property.set (Property.js?bust=1521841641354:216)
    at LabModel.addNewBall (LabModel.js?bust=1521841641354:132)
    at LabModel.step (LabModel.js?bust=1521841641354:75)
    at Sim.stepSimulation (Sim.js?bust=1521841641354:844)
    at Sim.stepOneFrame (Sim.js?bust=1521841641354:785)
    at Sim.runAnimationLoop (Sim.js?bust=1521841641354:768)

More info:

Query parameter maxBallsLab was added to test the "Out of Balls" dialog. From PlinkoProbabilityQueryParameters.js:

    // Maximum number of balls that can be in any 1 bin in the Lab screen, e.g. maxBallsLab=10
    // Use this to test the 'Out of Balls!' dialog without having to wait an eternity.
    maxBallsLab: {
      type: 'number',
      defaultValue: 9999,
      isValidValue: function( value ) {
        return value > 0 && Util.isInteger( value );
      }
    },

Running with maxBallsLab=10 and maxBallsLab=100, I experience the same failure described above. So I suspect that the same failure will occur without setting maxBallsLab query parameter. And that means that the production sim will (eventually) crash when this limit is reached.

Assigning to @ariel-phet to prioritize and assign.

pixelzoom commented 6 years ago

This was broken by @jessegreenberg on 10/27/17 in https://github.com/phetsims/plinko-probability/commit/e1a13e89815fcfe5c3067994d90daa80ee7a7c19 while adding keyboard navigation (#14). LabPlayPanel setPlayButtonVisible was for some reason deleted, but is still called in LabScreenView when the "out of balls" condition occurs. So when the limit is exceed, the sim is calling a function that no longer exists.

Version 1.1.2 on the PhET website was published on 7/10/2017, so production version is not affected.

Assigning to @jessegreenberg.

pixelzoom commented 6 years ago

Self-assigning since I'm listed as the responsible developer for this sim.

pixelzoom commented 6 years ago

I'm going to fix this and have @jessegreenberg review it.

pixelzoom commented 6 years ago

In addition to the missing setPlayButtonVisible function.... There was no need for the togglePlayPauseButtonVisibility, and no need for playButton and pauseButton to be instance properties, and playButtonVisibleProperty should have been a BooleanProperty. Fixed all of this in the above commit. @jessegreenberg please review.

jessegreenberg commented 6 years ago

Thanks @pixelzoom, very sorry about that. The break in https://github.com/phetsims/plinko-probability/commit/e1a13e89815fcfe5c3067994d90daa80ee7a7c19 came from using ToggleNode instead of setPlayButtonVisible. The other changes in https://github.com/phetsims/plinko-probability/issues/108#issuecomment-376270869 should have been done at the same time. I reviewed https://github.com/phetsims/plinko-probability/commit/12df5bd0e2413b538f6561a72bda6eccd94623d7 and verified problem is fixed and sim is working. Closing.

phetsims / plinko-probability

"max balls" feature is broken in Lab screen #108