Scenery layout fails when removing gene option

[Nancy-Salpepi]

Test device MacBook Air (m1 chip)

Operating System macOS 12.4

Browser safari and chrome

Problem description For https://github.com/phetsims/qa/issues/818, when removing a gene from the Add Mutations panel on the Lab screen, an error message pops up in the console. It may also take 30-40 seconds for the checkbox to uncheck. The sim still works.

I didn't see this error when removing the fur gene on the Intro screen.

Steps to reproduce

1. In Studio, go to the Lab Screen
2. Open console
3. Go to any gene, for example: naturalSelection.labScreen.view.genes.furVisibleProperty and uncheck the checkbox

Visuals

[pixelzoom]

Reproduced in 1.5.0-dev.1 and master.

This was not a problem in 1.4.3. It's new in 1.5, and it's caused by the new scenery layout code.

Assigned to @jonathanolson with top priority.

[pixelzoom]

Here's the stack trace from master:

Stack trace

``` Transform3.js? [sm]:545 Uncaught RangeError: Maximum call stack size exceeded at Transform3.inverseDeltaY (Transform3.js? [sm]:545:16) at Row._updateLocalPreferredHeight (HeightSizable.ts:193:65) at TinyEmitter.emit (TinyEmitter.ts:92:9) at Row.onTransformChange (Node.ts:2739:27) at TinyEmitter.emit (TinyEmitter.ts:92:9) at Transform3.invalidate (Transform3.js? [sm]:102:24) at Transform3.prependTranslation (Transform3.js? [sm]:140:10) at Row.prependTranslation (Node.ts:2671:21) at Row.translate (Node.ts:2324:14) at Row.setTop (Node.ts:3053:12) inverseDeltaY @ Transform3.js? [sm]:545 _updateLocalPreferredHeight @ HeightSizable.ts:193 emit @ TinyEmitter.ts:92 onTransformChange @ Node.ts:2739 emit @ TinyEmitter.ts:92 invalidate @ Transform3.js? [sm]:102 prependTranslation @ Transform3.js? [sm]:140 prependTranslation @ Node.ts:2671 translate @ Node.ts:2324 setTop @ Node.ts:3053 set top @ Node.ts:3063 set top @ LayoutProxy.ts:216 setProxyMinSide @ NodeLayoutConstraint.ts:125 positionStart @ MarginLayoutCell.ts:251 (anonymous) @ FlowConstraint.ts:283 (anonymous) @ FlowConstraint.ts:273 layout @ FlowConstraint.ts:232 updateLayout @ LayoutConstraint.ts:188 updateLayoutAutomatically @ LayoutConstraint.ts:202 emit @ TinyEmitter.ts:92 notifyListeners @ TinyProperty.ts:129 set @ TinyProperty.ts:77 set value @ TinyProperty.ts:63 _updateLocalPreferredHeight @ HeightSizable.ts:199 emit @ TinyEmitter.ts:92 notifyListeners @ TinyProperty.ts:129 set @ TinyProperty.ts:77 set value @ TinyProperty.ts:63 set preferredHeight @ HeightSizable.ts:114 layout @ Panel.ts:237 updateLayout @ LayoutConstraint.ts:188 updateLayoutAutomatically @ LayoutConstraint.ts:202 emit @ TinyEmitter.ts:92 notifyListeners @ TinyProperty.ts:129 set @ TinyProperty.ts:77 set value @ TinyProperty.ts:63 _updateLocalPreferredHeight @ HeightSizable.ts:199 emit @ TinyEmitter.ts:92 onTransformChange @ Node.ts:2739 emit @ TinyEmitter.ts:92 invalidate @ Transform3.js? [sm]:102 append @ Transform3.js? [sm]:155 appendMatrix @ Node.ts:2648 scale @ Node.ts:2378 scale @ Node.ts:2366 updateMaxDimension @ Node.ts:2777 validateBounds @ Node.ts:1424 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1386 get @ TinyStaticProperty.ts:31 notifyListeners @ TinyStaticProperty.ts:56 validateBounds @ Node.ts:1433 get @ TinyStaticProperty.ts:31 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 requestAnimationFrame (async) runAnimationLoop @ Sim.ts:945 Show 168 more frames studio-main.js? [sm]:130 Uncaught RangeError: Maximum call stack size exceeded at Transform3.inverseDeltaY (Transform3.js? [sm]:545:16) at Row._updateLocalPreferredHeight (HeightSizable.ts:193:65) at TinyEmitter.emit (TinyEmitter.ts:92:9) at Row.onTransformChange (Node.ts:2739:27) at TinyEmitter.emit (TinyEmitter.ts:92:9) at Transform3.invalidate (Transform3.js? [sm]:102:24) at Transform3.prependTranslation (Transform3.js? [sm]:140:10) at Row.prependTranslation (Node.ts:2671:21) at Row.translate (Node.ts:2324:14) at Row.setTop (Node.ts:3053:12) ```

[Nancy-Salpepi]

I am also seeing that error message when I remove a row from the Alleles panel--For example: naturalSelection.labScreen.view.graphs.pedigreeNode.allelesPanel.earsRow.visibleProperty

[pixelzoom]

More information, isolating the problem ... @jonathanolson let me know if you want to collaborate on this.

naturalSelection.labScreen.view.genes.furVisibleProperty is one of the global Properties found in GenesVisibilityManager. It's used to globally show/hide all UI components that are related to a specific gene (in this case, fur).

In GeneVisibilityManager.js, the general listener for each Property is found in createGeneVisibleProperty on line 59:

      // Set the visibility of UI components related to the gene. unlink is not necessary.
      property.link( visible => {
        addMutationsPanel.setGeneVisible( gene, visible );
        populationNode.setGeneVisible( gene, visible );
        proportionsNode.setGeneVisible( gene, visible );
        pedigreeNode.setGeneVisible( gene, visible );
      } );

By commenting out the first 3 lines of that listener, I isolated the problem to pedigreeNode.setGeneVisible:

      // Set the visibility of UI components related to the gene. unlink is not necessary.
      property.link( visible => {
        // addMutationsPanel.setGeneVisible( gene, visible );
        // populationNode.setGeneVisible( gene, visible );
        // proportionsNode.setGeneVisible( gene, visible );
        pedigreeNode.setGeneVisible( gene, visible );
      } );

Drilling down, pedigreeNode.setGeneVisible sets the visibility of a row in the Alleles panel, shown when the Pedigree radio button is selected.

In AllelesPanel.js:

  setGeneVisible( gene, visible ) {
    assert && assert( gene instanceof Gene, 'invalid gene' );
    assert && assert( typeof visible === 'boolean', 'invalid visible' );

    const row = _.find( this.rows, row => ( row.gene === gene ) );
    assert && assert( row, `row not found for ${gene.name} gene` );
    row.visible = visible;
  }

If I comment out the last line of setGeneVisible, the "stack size exceeded" exception does not occur. row.visible is the visibility of a Row, which is a subclass of HBox defined in AllelesPanel.js.

As @Nancy-Salpepi reported in https://github.com/phetsims/natural-selection/issues/313#issuecomment-1180803621, directly setting visibility of any Row in AllelsPanel.js also causes the exception. In the case of "fur", setting either naturalSelection.labScreen.view.genes.furVisibleProperty or naturalSelection.labScreen.view.graphs.pedigreeNode.allelesPanel.furRow.visibleProperty to false causes the exception. Setting the latter does not require commenting out code like I did above.

So for the purposes of debugging, I recommend using: naturalSelection.labScreen.view.graphs.pedigreeNode.allelesPanel.furRow.visibleProperty

[pixelzoom]

I added this to AllelePanel constructor, to see if furRow.visibleProperty is being toggle repeatedly. It is not -- the listener is called once, then the exception occurs.

    furRow.visibleProperty.lazyLink( visible => {
      console.log( `furRow.visible = ${visible}` );
    } );

[pixelzoom]

I started removing parts of AllelePanel's Row class, to see if a specific UI component was triggering the problem. If I remove the Image used to represent the allele, the problem goes away. In AllelePanel.js, inner class AlleleNode extends HBox:

    const textNode = new Text( abbreviation, {
      font: NaturalSelectionConstants.CHECKBOX_FONT,
      maxWidth: 12 // determined empirically
    } );

    const imageNode = new Image( image, {
      scale: 0.5 // determined empirically
    } );

    assert && assert( !options.children, 'AlleleNode sets children' );
-   options.children = [ textNode, imageNode ];
+   options.children = [ textNode ];

[jonathanolson]

That was quite a tricky interaction. It needed the 3-node-deep layout + a maxWidth/maxHeight. Because maxWidth is handled synchronously, it resulted in the scale being reduced on something that had a preferred width. Because the preferred width was larger than the maxWidth, it synchronously sized the content up slightly (violating the maxWidth). This would normally be fine, but this listener triggers BEFORE the additional layout code that would update the minimumWidth (and thus it would set the correct preferred width).

Long story short, it hit a loop where (a) every iteration, it was one level deeper in recursion due to maxWidth handling, so it crashed that way, and (b) it was shrinking the container (AllelesPanel) and growing the content iteratively in a loop.

By never setting our preferred width (from layout containers) above a maxWidth, this will (a) prevent this infinite loop and (b) allow correct behavior (since if we reduce the preferred size for that purpose, the maxWidth itself would scale up the node correctly later).

Can't reproduce with the commit above, and I've tested with snapshot testing to make sure there's no visible sim differences.

@pixelzoom can you test (and review?)

[pixelzoom]

Wow, that sounds like fun :) Tested and working in master. Thanks for fixing!

@Nancy-Salpepi over to you for verification in master. If it looks OK, please label fixed-awaiting-deploy.

[pixelzoom]

Removing the dev:phet-io label, since this was not techincal a PhET-iO problem. It just manifested in a PhET-iO content.

[Nancy-Salpepi]

Looks good on my end as well!

[pixelzoom]

To verify in https://github.com/phetsims/qa/issues/967, follow the steps in https://github.com/phetsims/natural-selection/issues/313#issue-1299183559.

Please close this issue if it verifies OK.

[Nancy-Salpepi]

Looks good in 1.5.0-dev.5. Closing.