Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the apple movement more efficient #219

Closed
marlitas opened this issue Apr 29, 2024 · 6 comments
Closed

Make the apple movement more efficient #219

marlitas opened this issue Apr 29, 2024 · 6 comments

Comments

@marlitas
Copy link
Contributor

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
Copy link
Contributor

jbphet commented May 15, 2024

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.

@jbphet jbphet assigned amanda-phet and unassigned jbphet May 15, 2024
@marlitas
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

Looks good to me!

@jbphet
Copy link
Contributor

jbphet commented Jun 5, 2024

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.

@jbphet jbphet assigned marlitas and unassigned jbphet Jun 5, 2024
@marlitas
Copy link
Contributor Author

marlitas commented Jun 5, 2024

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

@marlitas marlitas closed this as completed Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants