Appearance of "Zoom out to see data." is inconsistent

[phet-steele]

1. Intro screen
2. Zoom in the population graph as far as it can go
3. Add a mate
4. Let the bunnies take over the world, close the dialog

Note the "Zoom out to see data." message is displayed on the population graph.

5. Enable the "Brown Fur" checkbox. (note the message is now removed.)
6. Disable the "Brown Fur" checkbox. (not the message does not reappear.)

From this point, I have not figured out how to ever get this message back without pressing "Start Over". However!! Simply pressing Start Over and repeating the steps above will not return the message. However again!! If you instead follow these steps, you will get the message back:

7. "Start Over"
8. Zoom in the population graph as far as it can go
9. Add a dominant brown fur mutation (this should enable the Brown Fur checkbox, make sure it is on)
10. Add a mate
11. Let the clock run until generation 3, then add the "Limited Food" environmental factor.
12. Let the sim run until at least generation 8, and you should get the message back.
13. Pause sim
14. Zoom out once (you should see some brown fur data). Message will go away correctly.
15. Disable the Brown fur checkbox. The message will not come back!

Seen on Win 10 Chrome and ChromeOS. For phetsims/qa/issues/539.

Troubleshooting Information

URL: https://phet-dev.colorado.edu/html/natural-selection/1.1.0-dev.18/phet/natural-selection_all_phet.html
Version: 1.1.0-dev.18 2020-08-27 20:15:32 UTC
Features missing: touch
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.125 Safari/537.36
Language: en-US
Window: 1155x923
Pixel Ratio: 1/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 30 uniform: 4095
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true

[pixelzoom]

Thanks for the detailed steps @phet-steele.

I reproduced steps 1-6.

I could not reproduce steps 7-15. At step 11, I did not get the message back. I also tried repeating steps 1-6 before moving on to step 7. But no matter... This should give me enough to work with.

[phet-steele]

@pixelzoom I'm going to make an adjustment to the above steps.

In step 9, ensure the brown fur mutation is selected as dominant.

[pixelzoom]

OK. With that change to step 9, I can now reproduce the whole enchilada. Thanks.

[pixelzoom]

Visibility of the message is the responsibility of PopulationGraphNode:

    // If the generation clock has started and there's no data visible, display 'Zoom out to see data.'
    // unlink is not necessary.
    assert && assert( plotsNode.clipArea, 'plotsNode.clipArea is required' );
    const clipAreaBounds = plotsNode.clipArea.getBounds();
    plotsNode.localBoundsProperty.link( localBounds => {
      const clockHasStarted = ( populationModel.timeInGenerationsProperty.value !== 0 );
      const dataIsVisible = localBounds.intersectsBounds( clipAreaBounds );
      zoomOutToSeeDataText.visible = clockHasStarted && !dataIsVisible;
    } );

The problem is that this listener is firing when "Brown Fur" is checked (step 5), but not firing when "Brown Fur" is unchecked (step 6).

[pixelzoom]

A similar case:

1. Run sim, go to Intro screen.
2. Select Brown Fur dominant in "Add Mutations"
3. Zoom all the way in on Population graph
4. Press "Add a Mate" button, wait for bunnies to take over, close dialog.
5. Turn off "White Fur" checkbox in Population graph.
6. Turn off "Brown Fur" checkbox in Population graph. You should see the message since there is "Total" data above the graph, but no message is shown.
7. Zoom out to see Total data, zoom back in so that all Total data is above yMax and no data is shown. Still no message.

[pixelzoom]

To optimize performance, PopulationPlotNode (which draws one plot) is only updated when it's visible. So when it's made invisible, it then has stale points and line segments, which continue to contribute to the bounds. The solution is to clear the plot when it becomes invisible. That is the subject of the patch below. I'm not going to apply this patch until we see what other issues come up for phetsims/qa/issues/539, since this is an issue that might be appropriate to defer for the prototype.

patch

Index: js/common/view/population/PopulationPlotNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/population/PopulationPlotNode.js	(revision 77ad068d5e35951eaad8ed008997172f20d39f60)
+++ js/common/view/population/PopulationPlotNode.js	(date 1598643991411)
@@ -107,9 +107,9 @@
       }
     } );
 
-    // unmultilink is not necessary
+    // Update when visibility or range changes. unmultilink is not necessary.
     Property.multilink( [ this.visibleProperty, this.xRangeProperty, this.yRangeProperty ],
-      visible => visible && this.updatePlot()
+      visible => visible ? this.updatePlot() : this.clearPlot()
     );
   }
 
@@ -218,6 +218,19 @@
     }
   }
 
+  /**
+   * Clears the plot. While a plot is invisible, it is not updated.  So when a plot is made invisible, it must
+   * also be cleared. This is important because bounds are used to decide if the graph is displaying data, and
+   * whether the "Zoom out to show data" message should be displayed.  Bounds computation includes invisible Nodes,
+   * and if the plot still contains geometry, then it will be incorrectly contributing to bounds.
+   * See https://github.com/phetsims/natural-selection/issues/209
+   * @private
+   */
+  clearPlot() {
+    this.pointsPath.setShape( null );
+    this.stepPath.setShape( null );
+  }
+
   /**
    * Model-view transform for x axis.
    * @param {number} xModel - x model value, in generations

[pixelzoom]

This fix is low risk. I recommend that we roll this fix into the prototype RC, and @ariel-phet approves.

[pixelzoom]

Fixed in the above commit and ready for verification.

To verify in 1.1.0-rc.1:

1. Run the sim
2. Go to ether screen
3. Zoom the Population graph in fully, so the y-axis range is [0,5]
4. Press the "Add a Mate" button.
5. Wait for bunnies to take over the world, close the dialog
6. Verify that "Zoom in to see data" appears in the graph
7. Check the "Brown Fur" checkbox to the left of the Population graph. Verify that "Zoom in to see data" disappears.
8. Uncheck the "Brown Fur" checkbox. Verify that "Zoom in to see data" appears.

[amanda-phet]

The message seems to be working just fine. I did some other sequences of showing/hiding the message and do find one thing that's a little strange/buggy.

If I have nothing checked at all, I see the message "Zoom out to see the data." So I zoom out, but nothing changes, because I have nothing checked. I can't take a screenshot because I am on the iPad, but here were my steps:

1. Add a mutation (probably not necessary, but it implies that the checkboxes have data)
2. Add a mate
3. Check limited food
4. zoom in fully so the y-axis range is [0,5] and wait to see the message.
5. Uncheck all population panel checkboxes (total, white fur, brown fur) and still see the message
6. Zoom out fully so the y-axis range is [0,2500] and still see the message.

[amanda-phet]

Well that's weird.. I clicked reset-all to do more testing and "Zoom out to see data" is showing on the graph, but there is one bunny hopping around. I'll try to keep investigating.

[pixelzoom]

@amanda-phet In #209 (comment), it sounds like you are describing this known issue: #213.

[amanda-phet]

Indeed.

Still wondering about the reset-all thing.. I'll try to reproduce it.

[pixelzoom]

Re "the reset-all thing"... Steps to reproduce:

1. Go to ether screen
2. Press the "Add a Mate" button.
3. Wait for bunnies to take over the world, close the dialog
4. Uncheck "Total" checkbox
5. Press "Reset All"

Not sure what the issue is (I haven't investigated), but it's reproducible. Now the decision is whether to fix it for the prototype RC.

[amanda-phet]

Yup I got there too. I was thinking of putting these steps in #213 since it seems more related to that issue. Maybe when that one is fixed this won't happen?

[pixelzoom]

This is apparently a new problem, introduced by 23f61b8. It's not reproducible in 1.1.0-dev.18.

[pixelzoom]

It's possible that #213 might make this go away. But "Zoom..." is not supposed to be displayed when the sim has not started running yet, and whether checkboxes are checked is irrelevant.

[pixelzoom]

Simpler steps to reproduce:

1. Go to either screen
2. Add a Mate
3. Immediate pause
4. Uncheck "Total"
5. Reset All

[pixelzoom]

PopulationGraphNode.js is responsible for showing the "Zoom..." message. I added a console.log:

    plotsNode.localBoundsProperty.link( localBounds => {
      const clockHasStarted = ( populationModel.timeInGenerationsProperty.value !== 0 );
      const dataIsVisible = localBounds.intersectsBounds( clipAreaBounds );
+     console.log( `clockHasStarted=${clockHasStarted} dataIsVisible=${dataIsVisible}`);
      zoomOutToSeeDataText.visible = clockHasStarted && !dataIsVisible;
    } );

When the Reset All button is pressed, it reports:

clockHasStarted=true dataIsVisible=false

So what's going on here is that (on Reset All) the graphs are getting cleared before the clock is getting reset. So this code think the clock is still running and "Zoom..." should be visible.

So 23f61b8 didn't introduce a new problem, it exposed an additional problem.

[pixelzoom]

A couple of other notes, and then I'm going to try to ignore this for the weekend :)

• The order that things are reset was change in cebb401 for Usability of "Add a Mate", "Play", and "Start Over" button group #166.

• The code in Appearance of "Zoom out to see data." is inconsistent #209 (comment) should probably be determining clockHasStarted by looking at generationClock.isRunningProperty instead of populationModel.timeInGenerationsProperty.value.

EDIT: No, if should not look at generationClock.isRunningProperty, because the message would disappear when sim is paused, and when the student is review results after the sim has ended.

[pixelzoom]

Here's a patch that should fix the latest problem (that manifests with Reset All), as well as #213 and #214. I recommend that this fix first be tested in a master dev version. Then it can be applied to the prototype release branch, which will require spot checking in another RC.

patch

Index: js/common/view/population/PopulationGraphNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/population/PopulationGraphNode.js	(revision 1c22312b90c1c2c7562848ae4c7f4e9c9f7247d9)
+++ js/common/view/population/PopulationGraphNode.js	(date 1598845920173)
@@ -7,6 +7,7 @@
  * @author Chris Malley (PixelZoom, Inc.)
  */
 
+import Bounds2 from '../../../../../dot/js/Bounds2.js';
 import Vector2 from '../../../../../dot/js/Vector2.js';
 import merge from '../../../../../phet-core/js/merge.js';
 import ZoomButtonGroup from '../../../../../scenery-phet/js/ZoomButtonGroup.js';
@@ -143,14 +144,12 @@
       yAxisLabelNode.centerY = gridNode.y + ( gridHeight / 2 );
     } );
 
-    // If the generation clock has started and there's no data visible, display 'Zoom out to see data.'
+    // If the plot has data that is not visible, display 'Zoom out to see data.'
     // unlink is not necessary.
     assert && assert( plotsNode.clipArea, 'plotsNode.clipArea is required' );
-    const clipAreaBounds = plotsNode.clipArea.getBounds();
     plotsNode.localBoundsProperty.link( localBounds => {
-      const clockHasStarted = ( populationModel.timeInGenerationsProperty.value !== 0 );
-      const dataIsVisible = localBounds.intersectsBounds( clipAreaBounds );
-      zoomOutToSeeDataText.visible = clockHasStarted && !dataIsVisible;
+      zoomOutToSeeDataText.visible = !localBounds.equals( Bounds2.NOTHING ) &&
+                                     !localBounds.intersectsBounds( plotsNode.clipArea.bounds );
     } );
 
     super( options );

[pixelzoom]

I went ahead and applied the patch to master in the above commit.

@amanda-phet please test in 1.2.0-dev.1. Please also confirm that it addresses #213.

[amanda-phet]

The original issue seems to be fixed, and since #213 is fixed it seems even more unlikely that I can reproduce the reset-all bug as well. All is looking good.

@phet-steele should probably look this over too since he originally identified this bug.

[pixelzoom]

@amanda-phet My understanding is that @phet-steele was not available the first part of this week. Do you want to delay the next RC until he's available, or can @ariel-phet serve as the second set of eyes?

[pixelzoom]

4affe22 has been cherry-picked into the 1.1 branch.

[amanda-phet]

My understanding is that @phet-steele was not available the first part of this week. Do you want to delay the next RC until he's available, or can @ariel-phet serve as the second set of eyes?

I forgot about that. Yes, @ariel-phet please feel free to review this too.

[ariel-phet]

Looks good to me in latest dev.

[pixelzoom]

Ready for RC testing in phetsims/qa#542.

[ariel-phet]

Verified fixed. Closing