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

Unable to Preview Sim, Preview HTML, or Save if enough generations are made #298

Closed KatieWoe closed 3 years ago

KatieWoe commented 3 years ago

Device MacBook Pro OS MacOS 11.4 Browser Chrome Problem Description For https://github.com/phetsims/QA/issues/662. If enough generations go by in Studio before you try to launch, the launch will fail with a console error that reads "Maximum Call Stack Size Exceeded." It seems to happen around generation 8. Resetting the sim with the orange reset button lets you launch the sim again. Having so many generations seems an unlikely use case, which is likely why we didn't find this earlier, so I'm not sure how serious this, or if there would be easier ways to run into this. Steps to Reproduce

  1. Add a mate in studio
  2. Limit Food
  3. Wait until the generation clock reaches generation 8.
  4. Try to launch the sim

Visuals

Screen Shot 2021-07-02 at 1 26 04 PM
pixelzoom commented 3 years ago

@KatieWoe could you please check to see whether this was a problem that was overlooked in the 1.3 release? That will help determine whether it might be sim-specific, or a PhET-iO regression. Thanks.

KatieWoe commented 3 years ago

Just tested 1.3.1 and the sim still launches at Generation 11 with no console error.

pixelzoom commented 3 years ago

Thanks @KatieWoe.

Since there have been no sim-specific changes between 1.3 and 1.4, I suspect a regression that is specific to PhET-iO. If @samreid or @zepumph wants to have a look, please do so. Otherwise I'll investigate next Tuesday 7/6.

samreid commented 3 years ago

I saw the same at generation 5 on the RC. Trying to reproduce it locally unbuilt, it launched fine at gen 6 and 7, but gen 8 gave Aw Snap:

image

The line that is failing appears to be:

const compressedArray = Array.apply( [], compressedUint8Array );
samreid commented 3 years ago

Based on https://stackoverflow.com/questions/29676635/convert-uint8array-to-array-in-javascript/29676964, I tried:

    const a = Date.now();
    const compressedArray = Array.from( compressedUint8Array );
    const b = Date.now();
    console.log( b - a );

    const c = Date.now();
    const x = Array.prototype.slice.call( compressedUint8Array );
    const d = Date.now();
    console.log( d - c );

The times were 2ms and 3ms respectively, so let's go with Array.from.

samreid commented 3 years ago

I committed a proposed fix, @KatieWoe can you please test in master? If it seems good, let's cherry-pick it to the release branches.

zepumph commented 3 years ago

Thanks @samreid, over to @KatieWoe then

KatieWoe commented 3 years ago

Build off of master launched at Generation 11.

pixelzoom commented 3 years ago

Sounds like the fix is working, so back to @samreid to cherry-pick.

samreid commented 3 years ago

@pixelzoom can you please cherry pick this one? I'm struggling with the problems due to https://github.com/phetsims/chipper/issues/1056. When I followed the process, I get this error:

~/apache-document-root/main/studio$ git cherry-pick df19ef28db71a07dcf44d6752f2e35167654d27e
[natural-selection-1.4 2c4abae] Use Array.from to convert from Uint8Array, see https://github.com/phetsims/natural-selection/issues/298
 Date: Sun Jul 4 07:38:38 2021 -0600
 1 file changed, 2 insertions(+), 5 deletions(-)
~/apache-document-root/main/studio$ cd ../natural-selection/
~/apache-document-root/main/natural-selection$ grunt --brands=phet-io
Running "lint-all" task

Running "report-media" task

Running "clean" task

Running "build" task
Building runnable repository (natural-selection, brands: phet-io)
Building brand: phet-io
>> Webpack build complete: 2992ms
Fatal error: Perennial task failed:
AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: repo package.jso missing from package.json's phetLibs for package.json
  at /Users/samreid/apache-document-root/main/chipper/js/grunt/buildRunnable.js:130:5
  at Array.forEach (<anonymous>:null:null)
  at module.exports (/Users/samreid/apache-document-root/main/chipper/js/grunt/buildRunnable.js:128:29)
  at /Users/samreid/apache-document-root/main/chipper/js/grunt/Gruntfile.js:255:11
  at wrap (/Users/samreid/apache-document-root/main/chipper/js/grunt/Gruntfile.js:74:7)

Full Error details:
AssertionError [ERR_ASSERTION]: repo package.jso missing from package.json's phetLibs for package.json

There is one commit to cherry-pick, and there is already a "natural-selection-1.4" branch in the repo. The commit to cherry-pick is https://github.com/phetsims/studio/commit/df19ef28db71a07dcf44d6752f2e35167654d27e

pixelzoom commented 3 years ago

I'm testing the cherry-pick https://github.com/phetsims/studio/commit/df19ef28db71a07dcf44d6752f2e35167654d27e before patching:

% cd perennial
% grunt checkout-target --repo=natural-selection --target=1.4
% cd studio
% git cherry-pick df19ef28db71a07dcf44d6752f2e35167654d27e
% cd natural-selection
% grunt --brands=phet-io

I ran the sim in Studio, and let it get to > 15 generations. "Preview Sim" and "Preview HTML" seem to work. But I'm not sure what's supposed to happen when the "Save" button is pressed. I see this message in the console of the Studio browser window:

state could not be computed as initialState + changedState, storing. customizeWrapperTemplateForAction.js

... and a new (empty) browser tab opens, titled "Untitled".

@samreid What is the expected behavior for "Save" button?

samreid commented 3 years ago

Thanks, I opened https://github.com/phetsims/studio/issues/223 to discuss the "state could not be computed as initialState + changedState" problem.

When pressing the "Save" button, it should trigger a download of a file called natural-selection-custom-wrapper.html. For me, this happens in the new, blank "Untitled" tab. Does it download for you?

Once you get it to download, you can test the saved file by pressing "Load" in studio.

pixelzoom commented 3 years ago

When pressing the "Save" button, it should trigger a download of a file called natural-selection-custom-wrapper.html. For me, this happens in the new, blank "Untitled" tab. Does it download for you?

Once you get it to download, you can test the saved file by pressing "Load" in studio.

It's not at all obvious what Save does, but now that you've given me some hints, I see that it saves an html file to the default downloads directory for the browser. Load is really slow, but seems to be working.

This is (hopefully) ready for testing in the next 1.4 RC.

pixelzoom commented 3 years ago

Oh wait... This is not ready for QA yet. In https://github.com/phetsims/natural-selection/issues/298#issuecomment-875047308, I tested the cherry-pick. But I didn't push it to studio's 'natural-selection-1.4' branch.

pixelzoom commented 3 years ago

OK, now this is cherry-picked and patched. Ready for the next 1.4 RC.

KatieWoe commented 3 years ago

There still seems to be something wrong here. I had several generations on both screens. And while the sim would launch, it did not go to the correct state and an error occurred. The sim was unusable. I also noticed that, even when this did not occur, launching could be slower than before. bad

Screenshot 2021-07-16 163409
pixelzoom commented 3 years ago

@KatieWoe please describe the exact steps to reproduce the problem you're reporting in https://github.com/phetsims/natural-selection/issues/298#issuecomment-881755341.

KatieWoe commented 3 years ago
  1. In Studio on the first screen: Set food to limited and let the clock run until the generation clock is in the midteens
  2. Pause this screen and go to the second screen
  3. Repeat on second screen
  4. Try to Preview sim

This is the extent of what I did. If this does not reproduce the problem, let me know and I can try to see if anything else works.

pixelzoom commented 3 years ago

I cannot reproduce this problem. Here's what I did, on macOS 11.3.1 with Chrome 91.0.4472.114, Dev Tools open, showing console.

  1. Run https://phet-dev.colorado.edu/html/natural-selection/1.4.0-rc.2/phet-io/wrappers/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 15
  6. Press the Pause button
  7. Go to the Lab screen
  8. Repeat steps 3-7
  9. Press the "Preview Sim" button.

The preview launches fine. There are no errors in the console for the preview or Studio.

pixelzoom commented 3 years ago

To verify that I didn't make an error in patching this... In https://github.com/phetsims/natural-selection/issues/298#issuecomment-875003109, @samreid identified one sha that needed to be cherry-picked for NS 1.4, and it was https://github.com/phetsims/studio/commit/df19ef28db71a07dcf44d6752f2e35167654d27e. I did:

% cd perennial/
% grunt checkout-target --repo=natural-selection --target=1.4

... and manually inspected customizeWrapperTemplateForAction.js, to verify that the changes in that sha do indeed appear in studio's 'natural-selection-1.4' branch.

KatieWoe commented 3 years ago

Ok, I went back and tried to recreate the issue and I discovered what I think is the trigger. Specifically, the issue is in the step that say the generation should be in the midteens. The issue seems to occur in generation 16 or above specifically. It can be either screen. I also sometimes noticed an odd shift occur in studio when the generation clock reached generation 16, but it was hard to be sure of. sometimesshift

zepumph commented 3 years ago

Can you please reproduce with the debug version to see if we get a more upstream error (add ?phetioDebug to the studio URL)

pixelzoom commented 3 years ago

Reproduced in 1.4.0-rc.2 and master with the following 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.

The error message is different in 1.4.0-rc.2 vs master, possibly because the latter is being run unbuilt.

In 1.4.0-rc.2, the preview console shows this error message:

(index):121 Uncaught TypeError: Cannot read property 'phetioID' of undefined
    at g.fromStateObject (VM139 natural-selection_all_phet-io.html:849)
    at g.fromStateObject (VM139 natural-selection_all_phet-io.html:849)
    at Vf.applyState (VM139 natural-selection_all_phet-io.html:849)
    at Object.i.applyState (VM139 natural-selection_all_phet-io.html:849)
    at g.applyState (VM139 natural-selection_all_phet-io.html:849)
    at R.setStateForPhetioObject (VM139 natural-selection_all_phet-io.html:849)
    at VM139 natural-selection_all_phet-io.html:849
    at Array.forEach (<anonymous>)
    at R.iterate (VM139 natural-selection_all_phet-io.html:849)
    at R.setState (VM139 natural-selection_all_phet-io.html:849)

In master (unbuilt), the preview console shows this error message:

PhetioStateEngine.js:339 Uncaught Error: Assertion failed: father in state schema but not in the state object
    at window.assertions.assertFunction (assert.js:25)
    at StateSchema.js:108
    at Array.forEach (<anonymous>)
    at checkLevel (StateSchema.js:102)
    at StateSchema.checkStateObjectValid (StateSchema.js:114)
    at IOType.isStateObjectValid (IOType.js:292)
    at IOType.validateStateObject (IOType.js:334)
    at IOType.applyState (IOType.js:184)
    at PhetioStateEngine.setStateForPhetioObject (PhetioStateEngine.js:428)
    at PhetioStateEngine.js:328

The "father" referred to here is probably the father field in Bunny.js.

pixelzoom commented 3 years ago

Assigning to the PhET-iO team, since I'm unfamiliar with phetioEngine, and unfamiliar with the first fix that was done here. Let me know if I can help.

samreid commented 3 years ago

Testing in master, I can see the state object is the string DELETED:

image

I outputted the state object and it doesn't have any occurrence of DELETED.

Basically, by the 16th generation, bunny_0 is deleted, and therefore when setting the state, we need a way to delete bunny_0. I can call dispose on it, but I suspect we will also need to remove it from lists, possibly BunnyCollection and a PhetioGroup.

samreid commented 3 years ago

I saw that BunnyCollection was listening to the bunnyGroup as the ground truth, and that disposing a group element also disposed the element, so I attempted this patch, which assumes the dynamic element's parent is the container. But it didn't go so well in the browser--hanging/crashing:

```diff Index: js/PhetioStateEngine.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/PhetioStateEngine.js b/js/PhetioStateEngine.js --- a/js/PhetioStateEngine.js (revision 561542d3e189a5933218ff1b673e57dc64ad73ee) +++ b/js/PhetioStateEngine.js (date 1626718368845) @@ -141,6 +141,7 @@ deletedPhetioIDs.forEach( id => { assert && assert( !changedState[ id ] ); + console.log( 'marking as deleted: ' + id ); changedState[ id ] = DELETED_VALUE; } ); return changedState; @@ -425,7 +426,13 @@ assert && assert( !!type.applyState, 'IO Types should have applyState' ); this.onBeforeApplyStateEmitter.emit( phetioObject ); - type.applyState( phetioObject, state[ phetioID ] ); + if ( state[ phetioID ] === 'DELETED' ) { + const group = this.phetioEngine.getPhetioObject( phetioObject.tandem.parentTandem.phetioID ); + group.disposeElement( phetioObject, true ); + } + else { + type.applyState( phetioObject, state[ phetioID ] ); + } } /** ```
pixelzoom commented 3 years ago

@samreid said:

I saw that BunnyCollection was listening to the bunnyGroup as the ground truth, and that disposing a group element also disposed the element, ...

Yes, that's correct. We decided long ago not to use a separate list/array, and use PhetioGroup as the "ground truth" for what elements currently exist. Is there anything wrong with this approach?

+ if ( state[ phetioID ] === 'DELETED' ) {

I have no idea what this is, whether this is potentially a sim problem, or whether it's a PhET-iO problem. So I can't investigate further on my own. Let me know when you're available to collaborate.

samreid commented 3 years ago

Let's track this new problem in https://github.com/phetsims/natural-selection/issues/303

samreid commented 3 years ago

The new problem has been addressed in #303 and is ready for testing and cherry-picking.

pixelzoom commented 3 years ago

Verification failed in https://github.com/phetsims/qa/issues/672.

Even though it sounds like https://github.com/phetsims/natural-selection/issues/303 is a new issue, this issue has technically not been signed off by QA. So I'll keep this issue open for verification in the next RC.

To verify:

  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.

... then verify that there are no console errors for Studio or the Preview.

Nancy-Salpepi commented 3 years ago

I was able to successfully launch the sim after pausing in generation 17. However, the home screen briefly flashes on the screen before taking me to the intro screen with generation 17 paused. The same thing happened when I launched the html.(Mac 11 M1 chip + chrome)

zepumph commented 3 years ago

However, the home screen briefly flashes on the screen before taking me to the intro screen with generation 17 paused. The same thing happened when I launched the html.(Mac 11 M1 chip + chrome)

This is a known constraint of the way that PhET-iO is set up. Short answer: don't worry about it. Long answer: It is because there is an asynchronous clock tick between the time the sim starts and sends a message to the PhET-iO "wrapper" and the time that the wrapper receives it. Then another one when the wrapper sends back the customizations to the sim. Within that time, the sim has enough time to be drawn into its initial configuration because the customizations are applied.

Nancy-Salpepi commented 3 years ago

Thank you Michael. I will close the issue then.