search

phetsims / soccer-common

"Soccer Common" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 1 forks source link

`SoccerSceneModel.numberOfIdleBallsProperty` makes 3 changes each time a ball is kicked #17

Open jbphet opened 3 months ago

jbphet commented 3 months ago

This problem was discovered when trying to make press-and-hold work for the "Kick" button on the 4th screen of "Mean Share and Balance" (issue https://github.com/phetsims/mean-share-and-balance/issues/320). The button was becoming disabled after kicking the second-to-last ball instead of the last one. Some investigation revealed that the problem was due to the behavior of the numberOfIdleBallsProperty field. The value of this Property was changing three times each time a ball was kicked - once down by one, then down by two, and then down by one. When logging the values of this Property to the console, I'd see something like this for the first ball kicked:

numberOfIdleBalls = 6
SoccerSceneModel.ts:327 numberOfIdleBalls = 5
SoccerSceneModel.ts:327 numberOfIdleBalls = 6

On the second-to-last ball it would go through 0, and the 0 value would cause a listener to set the visibility of the "Kick" button to false, which would interrupt the button's input, thus stopping the press-on-hold behavior. The corresponding console output looked like this:

numberOfIdleBalls = 1
SoccerSceneModel.ts:327 numberOfIdleBalls = 0
SoccerSceneModel.ts:327 numberOfIdleBalls = 1

In https://github.com/phetsims/soccer-common/commit/1180554503bd107a17b73527550745a470a39b77 I changed the order of updates to the number of kicked balls versus the remaining queued balls. This resolves the immediate problem because the updates now go down, up, down instead of down, down, up; for example:

numberOfIdleBalls = 1
SoccerSceneModel.ts:327 numberOfIdleBalls = 2
SoccerSceneModel.ts:327 numberOfIdleBalls = 1

Since it doesn't go to zero for the second-to-last ball, the "Kick" button doesn't end up becoming invisible, and press-and-hold works for all balls.

While it's great to solve the immediate problem, this solution seems quite fragile, and I'm not sure if the change could have some unanticipated side effects. I discussed this with @marlitas and she was concerned too. Ideally we should figure out a way to make numberOfIdleBallsProperty less "glitchy", and only change once when a ball is kicked.

jbphet commented 3 months ago

I looked at this for a few minutes to see if I could come up with something easy to fix this, but didn't see anything glaringly obvious. Since I don't know this library very well, I'm assigning the issue to @marlitas to see if she has some ideas. Up to her whether to try to address this or defer it.

marlitas commented 3 months ago

I looked at this for a bit, and though I agree that the current solution isn't very robust I think spending more time here goes into a deeper dive than is worth right now. I added documentation so that future maintainers understand the peril of moving the one line of code around and pointed to this issue. I think that's enough for now.

Let's defer.