Assertion in debug build only

[jessegreenberg]

While looking into phetsims/john-travoltage#317, we noticed that this assertion is hit in the the _debug.html version. This isn't happening in RequireJS which is very strange. Here is the assertion.

Assertion failed: homeScreenIcon is too small: 0

I am not sure what could be causing this error only in the debug version. This sim is currently in dev testing, so @mbarlow12 so he is aware.

[mbarlow12]

@jessegreenberg this is really confusing. When inspecting the error, the icon appears to have the appropriate width and height attributes when passed to validateIconSize. Perhaps we could pair on this at some point soon?

[mbarlow12]

Here's a screenshot:

[jessegreenberg]

I am also confused and don't know why this is happening. Since this issue is related to phetsims/qa#204, lets add to developer meeting agenda to see if anyone more familiar with the debug build or image plugin has thoughts about why this is happening.

[jessegreenberg]

I verified that this is not an issue in other sims, including forces-and-motion-basics which also uses scenery/Image for screen icons.

[samreid]

I attempted to reproduce this problem. I pulled all repos and am up to date in master. Then I ran build john-travoltage and launched http://localhost/john-travoltage/build/phet/john-travoltage_all_phet_debug.html in Mac Chrome with the Dev Tools open. There is no assertion error. I also tried http://localhost/john-travoltage/build/phet/john-travoltage_all_phet_debug.html?ea and likewise saw no issues. Same behavior in firefox. Can you please elaborate how you are creating the problem?

[jessegreenberg]

Thanks for checking @samreid, this issue so far seems specific to coulombs-law. I just pulled all repos and ran grunt build in coulombs-law. Navigating to http://localhost:8080/coulombs-law/build/phet/coulombs-law_all_phet_debug.html, I see this assertion from Screen.js

Assertion failed: homeScreenIcon is too small: 0

We are not hitting this assertion in RequireJS.

EDIT: Stack trace shows this coming from CoulombsLawMacroScreen, but might happen for both screens in this sim.

[samreid]

The problem is that the Screen constructor is called before launch is called and that is where the icon Image is created. Launch awaits loading for all the images and works great for Screen.createModel and Screen.createView. For unknown reasons Coulomb's Law icon isn't loaded until launch completes and hence we have a problem here. @chrisklus and I tried delaying the validation until the next frame like so:

    // Validate icon sizes, after launch occurs, because that makes sure the images have the desired size
    setTimeout( function() {
      validateIconSize( options.homeScreenIcon, MINIMUM_HOME_SCREEN_ICON_SIZE, HOME_SCREEN_ICON_ASPECT_RATIO, 'homeScreenIcon' );
      validateIconSize( options.navigationBarIcon, MINIMUM_NAVBAR_ICON_SIZE, NAVBAR_ICON_ASPECT_RATIO, 'navigationBarIcon' );
    }, 0 );

and this worked properly. However, this is a risky solution because on some platforms, the next frame may be too soon. It seems we need (a) a way to load the images before or (b) a way to validate them after load. @jonathanolson was involved with image loading and may know more.

[samreid]

Other possibilities:

(1) Add a listener that validates after launch completes
(2) Check the Image and if loaded, validate right away. If not loaded, then add a listener that validates after load.

[samreid]

After further investigation, @chrisklus and I discovered the problem unique to Coulomb's Law is that it is creating the Screens before images are loaded. We revised that in the above commit and now it's launching well. Removing from developer meeting and leaving assigned to @jessegreenberg and @mbarlow12 to verify. Do we need a coding style note that checks Screens are created in launch, or is the image validation a good enough way of catching that?

[mbarlow12]

I think there needs to be some documentation. We may also want to update or add some comments to the grunt create-sim task as it's not entirely clear there either that the screens must be declared within the launch callback.

[jessegreenberg]

Thanks for the help with this @samreid and @chrisklus!

There is some documentation already in SimLauncher.js

Launch the Sim by preloading the images and calling the callback.

And in Image.js

A normal HTML <img>. If it hasn't been fully loaded yet, Scenery will take care of adding a listener that will update Scenery with its width/height (and load its data) when the image is fully loaded. NOTE that if you just created the <img>, it probably isn't loaded yet, particularly in Safari. If the Image node is constructed with an <img> that hasn't fully loaded, it will have a width and height of 0, which may cause issues if you are using bounds for layout. Please see initialWidth/initialHeight notes below.

[jessegreenberg]

We could be to add a line of documentation in simula-rasa-main.js to warn that Images shouldn't be create outside of SimLauncher.launch if it is important for their dimensions to be defined.

[jessegreenberg]

I added a comment about this. @mbarlow12 @samreid is this helpful or noise? Otherwise, I think this issue can be closed.

EDIT: I also tested that 9888e4e fixed this issue.

[mbarlow12]

Removing my assignment. @samreid I think we can close as well.

[samreid]

Looks good, thanks @jessegreenberg, closing.