phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

Make the apple movement more efficient #219

Closed marlitas closed 1 month ago

marlitas commented 2 months ago

We have noticed that apples tend to cross each other and take inefficient paths while animating. This seems like a polish fix we'd like to address.

jbphet commented 1 month ago

I've addressed the two cases where this was the most dramatic, namely the Collect->Sync and Collect->Share transitions. It seems like an improvement to me because it feels less chaotic. There are still some cases where the apples can cross in mid-flight, such as Sync->Collect when there are lots of apples on the plates, but these don't bother me. In fact, they make it seem a little more fun (in my opinion).

@amanda-phet - Please review the revised behavior and, if you're good with it, I think this can be closed. If you'd like to compare it against the previous behavior you can use https://phet-dev.colorado.edu/html/mean-share-and-balance/1.1.0-dev.5/phet/mean-share-and-balance_en_phet.html.

marlitas commented 1 month ago

@jbphet The code change:

// Move the whole apples to the appropriate plates.
          if ( i < numberOfWholeApplesPerActivePlate * activePlates.length ) {
            const plate = activePlates[ i % activePlates.length ];
            plate.addSnackToTop( apple );
          }
          else {

to:

        // Move the whole apples to the active plates.
        this.getActivePlates().forEach( plate => {
          _.times( numberOfWholeApplesPerActivePlate, () => {
            const apple = this.appleCollection.pop();
            assert && assert( apple, 'There should be enough apples to add the wholes ones to each plate.' );
            plate.addSnackToTop( apple! );
          } );
        } );

Causes an error in the state wrapper. I believe this is because the appleCollection observable array has already been set in state so it has no apples to pull from when we try to pop. We need to iterate over the apple collection (which will be empty when state setting) rather than the active plates. We can talk about it more, since I'm not sure I described it the best here.

marlitas commented 1 month ago

This is the assertion it hits in the state wrapper:

mean-share-and-balance : phet-io-state-fuzz : unbuilt
URL: http://128.138.93.172/continuous-testing/ct-snapshots/1715818531832/phet-io-wrappers/state/?sim=mean-share-and-balance&locales=*&phetioWrapperDebug=true&fuzz&phetioDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22mean-share-and-balance%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1715818531832%22%2C%22timestamp%22%3A1715825902255%7D
ERROR: Assertion failed: There should be enough apples to add the wholes ones to each plate.
amanda-phet commented 1 month ago

Looks good to me!

jbphet commented 1 month ago

After a bunch of investigation and discussion on this issue with both @zepumph and @marlitas, a fix has been added that better tolerates the situation where the state change handler for the distribution mode is called but the apples are already positioned and allocated to the plates (due to the phet-io state setting of the more fine-grained properties) as needed by the distribution mode.

Back to @marlitas for review.

marlitas commented 1 month ago

I think that looks great and love the use of while there. Thanks @jbphet I think this can be closed!