phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

During setState, the PhetioStateEngine needs to delete items marked as DELETED #303

Closed samreid closed 3 years ago

samreid commented 3 years ago

Discovered in https://github.com/phetsims/natural-selection/issues/298 but it's a separate problem and will have a separate solution. The problem is if bunny_0 is deleted, then it is marked as DELETED in the state file. However, the state engine doesn't know to delete existing items which are marked for deletion.

zepumph commented 3 years ago

I don't think that this is how we designed PhET-iO dynamic elements. The container should clear every element before state setting, and then only add back in those that are in the state. I'm unsure how DELETED items fit into the actual state setting at all. I thought it was just an implementation detail about the initial/changed state storing for studio customization.

samreid commented 3 years ago

Here's a simpler test harness to reproduce the problem:

```diff Index: js/common/view/EnvironmentPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/EnvironmentPanel.js b/js/common/view/EnvironmentPanel.js --- a/js/common/view/EnvironmentPanel.js (revision 07eeff7c2c59754344369154e0123593cb44c2bb) +++ b/js/common/view/EnvironmentPanel.js (date 1626723199589) @@ -77,7 +77,7 @@ model.bunnyCollection.liveBunnies.lengthProperty, { // Callback for the 'Add a Mate' button - addAMate: () => model.addAMate(), + addAMate: () => model.bunnyCollection.bunnyGroup.clear(), // Callback for the 'Start Over' button, with performance profiling, // see https://github.com/phetsims/natural-selection/issues/140 ```
samreid commented 3 years ago

@zepumph identified the problem that the DELETED items which appear in the initial state are not subtracted from the computed state. We made a patch which corrects the problem nicely, and can be cherry-picked into NS

samreid commented 3 years ago

The commit https://github.com/phetsims/studio/commit/452fa2179eccec8ed01fef2ff5d167242126f19e is the one and only commit required to address this problem. @pixelzoom can you please test and cherry-pick?

pixelzoom commented 3 years ago

For https://github.com/phetsims/qa/issues/672

Cherry-picked and tested:

% grunt checkout-target --repo=natural-selection --target=1.4
% cd studio
% git checkout natural-selection-1.4
% cd natural-selection
% git cherry-pick 452fa2179eccec8ed01fef2ff5d167242126f19e
% grunt --brands=phet-io
// test using the steps below
% cd studio
% git push origin natural-selection-1.4
% cd natural-selection
% grunt
% cp build/phet/dependencies.json .
% git add dependencies.json
% git commit -m "patch for 1.4 branch, https://github.com/phetsims/natural-selection/issues/303"
% git push origin 1.4

Ready for verification in the next RC, using these steps:

  1. Run the sim in Studio
  2. Go to the Intro screen
  3. Check the "Limited Food" checkbox
  4. Press the "Add a Mate" button
  5. Press and hold the Fast-Forward button until you get to Generation 16 (must be >= 16)
  6. Press the Pause button
  7. Press the "Preview Sim" button.
  8. Verify that there are no console errors for Studio or the Preview.
Nancy-Salpepi commented 3 years ago

Using the steps provided, there were no console errors for Studio or the Preview. (Mac 11 w/M1 chip + chrome)