Fuzz testing error: pegShadowImage undefined

[jessegreenberg]

Found while working on #25. When running a fuzz test, it is possible to call on the image width before it has been loaded. The following line is meant to catch this:

      // Slight chance the image used isn't loaded. In that case, return & try again on next frame
      if ( self.pegImage === null || self.pegShadowImage === null ) {return;}

but the images are undefined at this point, not null. Would something like

      if ( !self.pegImage || !self.pegShadowImage ) {return;}

be a sufficient fix?

[veillette]

Try the suggestion of @jessegreenberg. Make sure to run a fuzz test with enabled assertions to check it. Also we use this approach in the BallsLayerNode, so this would need to be changed as well.

[Denz1994]

We were able to test your approach @jessegreenberg and changing this line works. The self.pegImage and self.pegShadowImage were in fact "undefined" and not "null" as expected. However, because we still may have a case where the pegs were not loaded when they were called on we, made sure that they were loaded by adding a delay of 10 milliseconds, when we are painting the canvas. This is not noticeable for the user, but gives the sim enough time to load the image and paint the pegs.

Assigning to @jessegreenberg for review.

[jessegreenberg]

Thanks @Denz1994, looks good. But I am not sure that the timeout work around is robust enough. For instance, if an image fails to load or a maintainer incorrectly renames an image variable name, it will produce an infinite loop. Also, if invalidatePaint is called twice in the same animation frame, scenery will fire an assertion since it is indicative of an infinite loop. In 10ms there is no guarantee that we will be on the next animation frame.

Rather than a timeout, could we use the step function to repaint the canvas once the images are ready? One possibility would be to

• create a 'painted' flag in GaltonBoardCanvasNode, initially false
• in paintCanvas, set 'painted' flag to true once images are defined
• in the screen view step functions, you could have something like

      // if the galton board hasn't been painted yet, its images weren't loaded - try again this frame
      if ( !this.galtonBoardCanvasNode.painted ) {
        this.galtonBoardCanvasNode.invalidatePaint();
      }

to try and paint on the next animation frame once the images have been loaded.

Assigning back to @Denz1994 to try the above or for further discussion.

[Denz1994]

This approach is definitely more stable. The timer delay solution was a bit hackish.

We were able to create this "isInitiallyPainted" flag and check for the images' existence. The step function in the view now checks for the flag and invalidates the paint based on the its boolean value. This value is updated during the initial creation of the images and from there on out we don't have to worry about the initial flag because the logic will always pass that it was initially painted.

Assigning back to @jessegreenberg for review.

[jessegreenberg]

Thanks @Denz1994, your commit looks great! In the commit above, I removed the null check for BallsLayerNode for the same reason as the initial issue comment. Your fix in https://github.com/phetsims/plinko-probability/commit/7afbca15e9cbae4731723089c7b929204c5f0aea isn't necessary there because we don't hit that image on startup.