Screenshot in sim menu not capturing graphs

[catherinecarter]

In Calculus Grapher, the screenshot tool from the PhET menu in the nav bar isn't capturing the graphs. See screenshots below. The first screenshot is the one taken using the sim menu. The second screenshot I took with my computer and reveals what was actually there.
Screenshot with sim tool shows no graph lines:

These are the graph lines that it should be showing:

[pixelzoom]

@catherinecarter We need more info about your computer. Please choose PhET Menu > Report A Problem in the sim, and copy the full unedited info into this issue.

[pixelzoom]

I have no idea what could be causing this, and I have no familiarity with the Screenshot feature implementation. This sim is using bamboo throughout. Interesting that the curves for the radio buttons are showing up, because they are also bamboo. The only place we're explicitly setting renderer is for the curve shown above the slider, where we're using renderer: 'canvas'.

[catherinecarter]

Test device
Mac Desktop Apple M1 Pro
Operating System
Ventura 13.2.1
Browser
Chrome Version 110.0.5481.177
Problem description
Screenshot not capturing graphs
Steps to reproduce
From sim menu, I chose Screenshot
Visuals
See above

Troubleshooting information:
!!!!! DO NOT EDIT !!!!!
Name: ‪Calculus Grapher‬
URL: https://bayes.colorado.edu/dev/phettest/calculus-grapher/calculus-grapher_en.html?ea&brand=phet
Version: 1.0.0-dev.21 (unbuilt)
Features missing: applicationcache, applicationcache, touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/110.0.0.0 Safari/537.36
Language: en-US
Window: 1890x985
Pixel Ratio: 1.600000023841858/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: 31 uniform: 1024
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 16384x16384
OES_texture_float: true
Dependencies JSON: {}

[pixelzoom]

I reproduced on macOS 13.2.1 with all browsers: Chrome Version 110.0.5481.177, Safari 16.3, and Firefox 110.0.1. So it's not browser specific.

[kathy-phet]

@pixelzoom - This has happened in other sims ... e.g. Greenhouse photons not appearing:

This is what Sam said there: phetsims/greenhouse-effect#255 (comment)
We just need to pass in the canvasBounds like so:

Details
Subject: [PATCH] Ignore invisible nodes in move forward/backward, see phetsims/sun#814

Index: js/common/PhotonSprites.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8

diff --git a/js/common/PhotonSprites.ts b/js/common/PhotonSprites.ts
--- a/js/common/PhotonSprites.ts (revision c0fd52d298c3a6c9503247c2b107e1ee6490a740)
+++ b/js/common/PhotonSprites.ts (date 1673477063918)
@@ -25,6 +25,7 @@
import Range from '../../../dot/js/Range.js';
import LayersModel from './model/LayersModel.js';
import greenhouseEffect from '../greenhouseEffect.js';
+import Bounds2 from '../../../dot/js/Bounds2.js';

// constants
const TARGET_PHOTON_IMAGE_WIDTH = 17; // empirically determined to match the design
@@ -62,7 +63,8 @@
sprites: [ visiblePhotonSprite, infraredPhotonSprite ],
spriteInstances: spriteInstances,
renderer: 'webgl',

•  pickable: false
  

•  pickable: false,
  

•  canvasBounds: new Bounds2( 0, 0, 500, 500 )
  

  } );

  // Calculate the scale that will be used to render the photon images.

[pixelzoom]

This sim is not using canvas.

[pixelzoom]

fourier-making-waves also uses bamboo, and does not exhibit this problem. It uses canvas - CanvasLinePlot, ChartCanvasNode, etc.

[kathy-phet]

And @zepumph said here: phetsims/my-solar-system#52 (comment)

[kathy-phet]

If those aren't helpful then consult with @jonathanolson

[pixelzoom]

@kathy-phet said:

And @zepumph said here: phetsims/my-solar-system#52 (comment)

We are indeed using boundsMethod: 'none' for the plots (what @catherinecarter referred to as the "graph lines"). When we consulted with @jonathanolson on performance improvements, his recommendation was to use that option for plots.

[kathy-phet]

Maybe @jonathanolson would recommend just listening for Screenshot and then changing the boundsMethod for that particular context to keep it speedy otherwise. Not sure.

[zepumph]

@pixelzoom, you and I both seemed to have the same, incomplete understanding of boundsMethod: 'none'. Please see my explanation from a conversation with JO over in phetsims/my-solar-system#52 (comment).

Basically just supply a localBounds override to your node to tell the canvas rasterization that you have valid bounds and need to be rendered.

[pixelzoom]

In phetsims/my-solar-system#52 (comment), @zepumph said:

@jonathanolson and I talked about this just now, and he taught me about how boundsMethod: 'none' is buggy unless it is specified with a custom override of the localBounds. Otherwise you are just telling scenery that you don't take any valid bounds, and scenery is allowed (and potentially expected) to not display your content. Just because the screenshot is the only spot where this occurs doesn't mean that it will be in the future.

@jonathanolson is there any chance that this bug can be fixed, instead of having to add this localBounds workaround? We have many places in calculus-grapher where this workaround would need to be applied.

[zepumph]

The conversation I had with @jonathanolson made me realize that in fact the boundsMethod: 'none' is actually the "work around" type code. What you are saying there is "my node takes up no bounds", and you forgo the responsibility of scenery needing treat your Node as if iit has any content. Really what that option is for is for opting out of a default bounds calculation algorithm, instead saying "I will set it myself". Thus, it is buggy to have a content-ful Node with boundsMethod: 'none' in which you don't then take the responsibility of setting the bounds manually.

We spoke briefly in the conversation about hard coding a closer connection to those two steps, perhaps with assertions, but it is kinda challenging to know, so instead we made sure the doc was up to date and loud about boundsMethod: https://github.com/phetsims/scenery/blob/e528fceb850b87bf29c53ce979efdcad9b0915ac/js/nodes/Path.ts#L61-L63

I hope that is helpful, and I'm happy to talk next steps with you in person if that is best.

[pixelzoom]

It sure would be nice to have some input from @jonathanolson here.

[pixelzoom]

.... Thus, it is buggy to have a content-ful Node with boundsMethod: 'none' in which you don't then take the responsibility of setting the bounds manually.

OK. And it's not possible to let the programmer know that they've done that? Or document it somewhere? So that we don't find out weeks later when Screenshot is broken?

[zepumph]

Or document it somewhere?

We moved the documentation to where boundsMethod is defined this morning:

We spoke briefly in the conversation about hard coding a closer connection to those two steps, perhaps with assertions, but it is kinda challenging to know, so instead we made sure the doc was up to date and loud about boundsMethod: https://github.com/phetsims/scenery/blob/e528fceb850b87bf29c53ce979efdcad9b0915ac/js/nodes/Path.ts#L61-L63

[jonathanolson]

@zepumph noted that it seems important to have assertions to catch cases like this. I agree (we don't want things cut off, whether they are CanvasNode/WebGLNode/boundsMethod:none, etc.)

This is on me, when I recommended boundsMethod:none, (a) I didn't consider the screenshot handling or Canvas render path, (b) I didn't have assertions on to protect against this, and (c) I didn't have documentation recommending this.

Especially with WebGL rendering more in the future, I think it's important to have the bounds constraint be satisfied in general in our code.

Any thoughts/objections on that path (adding assertions to detect bad bounds cases)?

[pixelzoom]

Any thoughts/objections on that path (adding assertions to detect bad bounds cases)?

Sounds like an improvement.

[jonathanolson]

Issue in phetsims/scenery#1546 for the assertions

[pixelzoom]

I'm having no luck using the localBounds workaround. I've starting by trying to get this.predictCurveNode in OriginalGraphNodes.ts to show up in a Screenshot. this.predictCurveNode will be clipped to the bounds of this.chartRectangle. I've tried all kinds of values for localBounds, including Bounds2.EVERYTHING, and this.predictCurveNode is not rendered when I take a ScreenShot. What am I doing wrong?...

[jonathanolson]

Not sure, I'll look into it tomorrow. My understanding is that (a) it should be something finite, and (b) it should be on the Path itself with boundsMethod:none.

[jonathanolson]

Implemented above. I'm not quite sure what was going wrong. Can you see how this implementation looks? (Also the clip area for the curveLayer in GraphNode is using chartRectangle.getShape(), which ignores the stroke, so my method of getting the candidate bounds for plotBounds also ignores the stroke with chartRectangle.getShape().bounds, is that desired?)

[pixelzoom]

Thanks @jonathanolson, changes look great. And I confirmed that everything is present in Screenshots. We may ultimately switch to bamboo Canvas implementation for performance. But thi issue can be consider "done" and closed.