SpectrumNode can be undersized for non-integer dimensions

[jbphet]

In phetsims/blackbody-spectrum#62 we ran into an issue where the spectrum representation for visible light on the graph ended up being narrower than expected, and that smallness got worse as the node was scaled up (to see a screen capture of how this looked, search the issue for the word "rainbow"). After much investigation by @arnabp and a little by myself, we traced the problem to SpectrumNode, specifically the following code:

    // Draw the spectrum directly to a canvas, to improve performance.
    var canvas = document.createElement( 'canvas' );
    var context = canvas.getContext( '2d' );
    canvas.width = config.size.width;
    canvas.height = config.size.height;

In our particular case, the value of config.size.width was 54.999999.... due to floating point issues, and the canvas was flooring that value to 54, so the last row of pixels was essentially missing. When scaled up, that gap become very apparent. When we changed to code to:

    // Draw the spectrum directly to a canvas, to improve performance.
    var canvas = document.createElement( 'canvas' );
    var context = canvas.getContext( '2d' );
    canvas.width = Util.roundSymmectric( config.size.width );
    canvas.height = Util.roundSymmectric( config.size.height );

...the problem went away. However, we don't know enough about the nature and usages of this node to feel comfortable making changes like this, so we ended up doing the rounding in the blackbody-spectrum code in order to resolve the immediate problem. Our question is whether this should be changed in SpectrumNode. It seems like perhaps Math.ceil() would be even better, since it would never cut things off, but again, I'm not sure what the potential side effects might be with a change of this nature.

Assigning to @pixelzoom to get his input since he is listed as the first author in the comments.

[pixelzoom]

I'd think that you'd want to use Math.ceil rather than Util.roundSymmectric. Otherwise you're going to lop off pixels for values like 54.3. Otherwise this should be fine, as long as you don't draw outside of config.size.

It also would be good to reimplement SpectrumNode as a subclass CanvasNode, which didn't exist when SpectrumNode was created for use in BLL.

[jbphet]

Okay. Assigning to @arnabp to add Math.ceil, test in Blackbody Spectrum and in the scenery-phet demo and, if all looks good, close. The port to CanvasNode sounds like a reasonable idea, but it's more than we want to take on to resolve this.

[pixelzoom]

... The port to CanvasNode sounds like a reasonable idea, but it's more than we want to take on to resolve this.

OK, we'll track that in #501. But in light of #498 (SpectrumSlider vs NumberControl) if may be better to do this sooner rather than later.

[pixelzoom]

@jbphet @arnabp hold off on changes to SpectrumNode. I'm going to take care of #501, port to CanvasNode.

[pixelzoom]

In Slack, @jbphet said:

Let me know when you've done it, and I'll pull and check it out on blackbody, since I'm quite familiar with the problem that we were seeing there, and it will be a good test case.

SpectrumNode work completed in #501. Pull scenery-phet to test in blackbody-spectrum.

The first thing I notice (comparing to blackbody-spectrum 1.0.0-dev.16) is that brightness looks reduced. I'm not sure why that is, because the algorithm to compute the colors is unchanged.

I have not added Math.ceil calls yet, because there’s no indication in CanvasNode docs that integer values are required. Let me know if the original problem ("graph ended up being narrower than expected") is still present.

[arnabp]

The reason for the reduced brightness is that there now appear to be black lines between each of the rectangular fills. I tried increasing the width of the rectangles, but while that did remove the black lines, the individual rectangles are still very visible. So the new port to CanvasNode has done something to how the SpectrumNode used to be smooth.

I've increased the size of the wavelengthSpectrumNode to better illustrate this:
Old SpectrumNode

New SpectrumNode

New SpectrumNode - Increased width of rectangles to 1.1 pixels

[pixelzoom]

Thanks @arnabp, I'll have a look.

Is the "graph ended up being narrower than expected" problem still present?

[pixelzoom]

No idea why the results are different when rendering to CanvasNode vs document.createElement( 'canvas' ). So in the above commit, I reverted to SpectrumNode extends Image, and I added the Math.ceil calls for width and height.

I see that BLACKBODY_SPECTRUM/GraphDrawingNode is already adding a Math.ceil call. I tried removing it to test, but I'm not familiar with the original problem. So @jbphet or @arnabp please have a look.

I'm also not sure how to proceed with this issue. I've currently documented options.size like this:

// {Dimension2} desired size of the Node. Actual size will be set to integer values via Math.ceil
size: DEFAULT_SIZE,

But it seems problematic to ask for a specific size, then get something else. Perhaps we should do something like render the spectrum to a canvas that is sized a bit larger, then scale the Image node that renders the canvas.toDataURL so that the final size is accurate.

[pixelzoom]

I said:

... Perhaps we should do something like render the spectrum to a canvas that is sized a bit larger, then scale the Image node that renders the canvas.toDataURL so that the final size is accurate.

That's exactly what I did in the above commit. I think this is the way to go. But @jbphet @arnabp let me know what you think.

A general concern with this issue is that some sim may be unknowingly relying on SpectrumNode being smaller than the options.size value that was provided. I poked around a bit to see if I could identify usages, but it was difficult -- SpectrumNode isn't used directly by sims, it's a subcomponent of a few other UI components (WavelengthSpectrum, SpectrumSlider, WavelengthSlider,...)

[arnabp]

Testing in blackbody shows no issues, the spectrum is back to being smooth and the SpectrumNode handles the original graph width problem just fine, so everything looks good on this end.

[arnabp]

Assigning this back to you @pixelzoom, not sure if you want to keep it alive to try to port to CanvasNode again in the future and try to resolve the strange rendering issue, or leave it as is.

[pixelzoom]

I think I'll just punt on using CanvasNode here. Closing.

[pixelzoom]

Reopening because this is affecting phetsims/wave-interference#402.

[pixelzoom]

Confirmed that phetsims/wave-interference#402 is not a problem in master, closing again.