Should canvasBounds be required?

[samreid]

In https://github.com/phetsims/greenhouse-effect/issues/255#issuecomment-1382591012, we found that photons did not render in the screenshots because canvasBounds was not being passed through. There are 3 places where canvasBounds is in the options, but it is optional each time. Should it be required?

[jonathanolson]

It definitely seems like an option where things shouldn't take up "no" bounds... "infinite" bounds doesn't really work too well either. To me it makes sense to be required.

[samreid]

Here's a patch that attempts to make it required:

```diff Subject: [PATCH] Reuse model values for rewind property, see https://github.com/phetsims/gravity-and-orbits/issues/459 --- Index: main/scenery/js/nodes/WebGLNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/scenery/js/nodes/WebGLNode.ts b/main/scenery/js/nodes/WebGLNode.ts --- a/main/scenery/js/nodes/WebGLNode.ts (revision 6e34d8cdeacbcc82f64c1a7bbc4d537bdd94bc14) +++ b/main/scenery/js/nodes/WebGLNode.ts (date 1676237974992) @@ -22,7 +22,7 @@ ]; type SelfOptions = { - canvasBounds?: Bounds2; + canvasBounds: Bounds2; }; export type WebGLNodeOptions = SelfOptions & NodeOptions; Index: main/scenery/js/nodes/CanvasNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/scenery/js/nodes/CanvasNode.ts b/main/scenery/js/nodes/CanvasNode.ts --- a/main/scenery/js/nodes/CanvasNode.ts (revision 6e34d8cdeacbcc82f64c1a7bbc4d537bdd94bc14) +++ b/main/scenery/js/nodes/CanvasNode.ts (date 1676237974995) @@ -21,7 +21,7 @@ ]; type SelfOptions = { - canvasBounds?: Bounds2; + canvasBounds: Bounds2; }; export type CanvasNodeOptions = SelfOptions & NodeOptions; Index: main/bending-light/js/intro/view/WaveCanvasNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/bending-light/js/intro/view/WaveCanvasNode.ts b/main/bending-light/js/intro/view/WaveCanvasNode.ts --- a/main/bending-light/js/intro/view/WaveCanvasNode.ts (revision dff052fa6b7885eae761e6dda060c107a023a584) +++ b/main/bending-light/js/intro/view/WaveCanvasNode.ts (date 1676238147864) @@ -10,7 +10,7 @@ */ import ModelViewTransform2 from '../../../../phetcommon/js/view/ModelViewTransform2.js'; -import { CanvasNode, NodeOptions } from '../../../../scenery/js/imports.js'; +import { CanvasNode, CanvasNodeOptions } from '../../../../scenery/js/imports.js'; import bendingLight from '../../bendingLight.js'; import LightRay from '../../common/model/LightRay.js'; @@ -23,7 +23,7 @@ * @param modelViewTransform - Transform between model and view coordinate frames * @param [providedOptions] - options that can be passed on to the underlying node */ - public constructor( lightRays: LightRay[], modelViewTransform: ModelViewTransform2, providedOptions?: NodeOptions ) { + public constructor( lightRays: LightRay[], modelViewTransform: ModelViewTransform2, providedOptions?: CanvasNodeOptions ) { super( providedOptions ); this.lightRays = lightRays; Index: main/scenery/js/nodes/Sprites.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/scenery/js/nodes/Sprites.ts b/main/scenery/js/nodes/Sprites.ts --- a/main/scenery/js/nodes/Sprites.ts (revision 6e34d8cdeacbcc82f64c1a7bbc4d537bdd94bc14) +++ b/main/scenery/js/nodes/Sprites.ts (date 1676237974989) @@ -29,7 +29,7 @@ // testing, etc.). If false, the canvasBounds will be used for hit testing. hitTestSprites?: boolean; - canvasBounds?: Bounds2; + canvasBounds: Bounds2; }; // We don't specify a default for canvasBounds on purpose, so we'll omit this from the optionize type parameter. ```

There are 15 or so type errors though, with several sim-specific details to work through. So this could take 2 hours or so, and require assistance for sim devs.

[samreid]

I wrote on Slack #dev-public:

Currently canvasBounds is typed as optional, but failing to provide it means we end up catching bugs in QA instead of during type checking. I opened this issue to investigate https://github.com/phetsims/scenery/issues/1521, but my subteam’s choice board is filling up and I don’t feel we have time to work on it in this iteration. But I also don’t want the issue to get forgotten, so I thought I would post it here and see if anyone else wants it.

Self-unassigning.

[samreid]

You can search for canvasBounds? in the code to see the places it is optional. The 3 places are: CanvasNode.ts, Sprites.ts and WebGLNode.ts.

[jonathanolson]

By optional, I mean "it should be required for canvasBounds to work", sorry. But optional as an option, since you can set it with a link, etc., since we don't have the ability to set canvasBoundsProperty with a DerivedProperty right now. We might also want to update it and NOT set it initially.

Presumably we should add an assertion that it needs canvasBounds whenever it's rendered.