phetsims / vegas

Reusable game components for PhET simulations.
MIT License
1 stars 4 forks source link

RewardNode out-of-phase paint bug #55

Closed phet-steele closed 7 years ago

phet-steele commented 7 years ago

This title is tentative pending investigation by @jonathanolson 😄

I'm inclined to say I only see this problem in FF, not Chrome or Safari. Upon initializing the RewardNode, resizing the browser window will cause most of the sim's display to freeze. The sim is still somewhat usable, however. This also does not seem to be a problem in built versions, as I can only repro on phettest with requirejs (again just with FF). Seen on both Make a Ten and Balancing Chemical Equations (and probably many more!):

rewardnode

The associated error:

Error: Assertion failed: This should not be run in the call tree of updateDisplay().
If you see this, it is likely that either the last updateDisplay() had a thrown error 
and it is trying to be run again (in which case, investigate that error), OR code 
was run/triggered from inside an updateDisplay() that has the potential to 
cause an infinite loop, e.g. CanvasNode paintCanvas() call manipulating 
another Node, or a bounds listener that Scenery missed.

Seen during and is expressed in the requirejs version of phetsims/tasks/issues/762. (Make A Ten)

jonathanolson commented 7 years ago

Some crazy execution order things happening, might be related to https://bugs.jquery.com/ticket/13376

jonathanolson commented 7 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=840412 is the Firefox bug that is presumably triggering this.

jonathanolson commented 7 years ago

Call stack before workaround: firefox-resize

Added deferred handling of resizes to work around this.

jonathanolson commented 7 years ago

@samreid, do you mind reviewing my patch above (as I believe you're the most familiar with the changed code)?

samreid commented 7 years ago

I would like more time to understand the root of the problem, but I noticed that resizePending is initialized to true and there is a resize during startup here:


      // Fit to the window and render the initial scene
      // Can't synchronously do this in Firefox, see https://github.com/phetsims/vegas/issues/55 and
      // https://bugzilla.mozilla.org/show_bug.cgi?id=840412.
      $( window ).resize( function() { self.resizePending = true; } );
      this.resizeToWindow();

the effect is that there are 2 resize calls during startup. @jonathanolson which one do you recommend we keep?

jonathanolson commented 7 years ago

Bah, I was originally thinking of making the resizePending = false inside resizeToWindow(). Thoughts on that?

samreid commented 7 years ago

Yes, that sounds good.

jonathanolson commented 7 years ago

Fixed.

samreid commented 7 years ago

I discussed this problem with @jonathanolson and he said scenery cannot accommodate a change in display size during its update phase. When the fill on the canvas is set, Firefox dispatches a synchronous resizing event which is causing the problem. The solution proposed by @jonathanolson in Joist seems appropriate and I cannot think of any problems it will cause. Closing.

samreid commented 7 years ago

That is to say, the code seems like an appropriate solution, and we should await cross-platform testing during the first RC cycle to determine whether there are any unexpected/unintended practical ramifications from this workaround.