Improve toImage*

[jonathanolson]

Should ideally not apply root transform or visibility (like 0.2's internal handling), and we should have an option to apply a certain resolution scale.

[jonathanolson]

Also, we may want to look into initialWidth/initialHeight handling for this, as it could solve many of our problems.

[jonathanolson]

Adding toImageNode as the initial "improved" version. Will improve documentation & testing, and will deprecate other methods so that toImageNode (or whatever its final name will be) is the one true way to create an image node from a node.

[jonathanolson]

The following are now deprecated:

• toImage
• toImageNodeAsynchronous
• toCanvasNodeSynchronous
• toDataURLImageSynchronous
• toDataURLNodeSynchronous

Please use node.rasterized() instead. It supports all of the good options/features for those methods, and additionally supports controlling the resolution (so we don't have to hack together scaling code every time things are used).

Tagging for dev meeting to:

1. Let everyone know it exists.
2. Decide on a priority for moving things over to the new method. It would be nice to remove the 5 methods (and simplify code using this general feature), but I don't know if it's worth it to have a chip-away "issue", or if someone (me?) should go through and convert things.

[pixelzoom]

@jonathanolson Can you say a little about what motivated this issue?

[samreid]

Sounds great, should we schedule a code review and trial port before we go "all in"? I don't know what risks are involved, if any (preceding comment sounds like low risk though).

[pixelzoom]

My sims that are affected by this change:

• acid-base-solutions (toImage in MagnifierNode)
• ph-sale (toImage in RatioNode)
• plinko-probability (toImage in BallsNode and PegsNode)

Comon code affected:

• scenery-phet (toImage in MeasuringTapeNode)
• vegas (toImage in RewardNode)

[jonathanolson]

@jonathanolson Can you say a little about what motivated this issue?

Overall, mainly:

• Having to write ugly scaling up/down code where using many of these methods
• Lots of slightly different ways with no clear recommendation/default
• Asynchronous methods were an absolute pain
• Other synchronous methods that didn't have bounds synchronously were an absolute pain for layout
• Method names were about the implementation, not what they did
• The x/y doing the opposite of what seemed to be natural in retrospect.
• Confusing optional parameters that are almost never needed

Sounds great, should we schedule a code review and trial port before we go "all in"? I don't know what risks are involved, if any (preceding comment sounds like low risk though).

I'm happy for there to be a code review. Up to others.

[pixelzoom]

Thanks for clarifying. +1 for simplifying. Happy to address my sims. Someone else will need to address common code; I don't have time.

[jessegreenberg]

should we schedule a code review

I don't think I have used any of the deprecated functions and don't have the knowledge to review for potential risks.

[jbphet]

My sims that are affected:

• Balancing Act
• States of Matter (and variations)
• Gene Expression Essentials

Common code that I developed:

• Vegas RewardNode

[pixelzoom]

4/5/18 dev meeting notes:
• rasterize is always synchronous.
• Should rasterized get a code review? No, code review is low risk.
• @jonathanolson Will document (in this issue) the mapping between deprecated methods and usage of rasterize.
• What should we do about usages of deprecated methods? @pixelzoom will change his sims. If all goes well, @jonathanolson will change other usages.
• Add 'dev:chip-away' label and revisit at next dev meeting.

[pixelzoom]

@jonathanolson Assign to me after you've documented the mapping between deprecated methods and usages of rasterize. Then I'll convert my sims.

[jonathanolson]

First of all, if any parameters (besides callbacks) are provided to the deprecated methods (i.e. the x/y/width/height parameters), then those should be converted into using the sourceBounds option, e.g. sourceBounds: new Bounds2( -x, -y, width, height) (x/y are reversed, as those are the actual bounds). If only two are provided, contact me to assist.

• toDataURLNodeSynchronous: Use rasterized().
• toDataURLImageSynchronous: Use rasterized( { wrap: false } )
• toCanvasNodeSynchronous: Use rasterized( { useCanvas: true } )
• toImageNodeAsynchronous( callback ): Use rasterized(), but note that it now immediately returns the result (is not asynchronous), so some refactoring may be helpful.
• toImage: Depends on why it was being used. If you need something that can be slotted into a scenery Image, just use toDataURL() (synchronous). If you want a node out, use rasterized() (e.g. MeasuringTapeNode). If you want a Canvas out (e.g. using it like MagnifierNode in ABS, RatioNode in ph-scale, PegsNode in plinko or RewardNode, since it is being drawn to a canvas), toCanvas can be used instead (will be synchronous).

I'll be available to assist in any conversion.

[pixelzoom]

@jonathanolson I addressed my sims in the above commits. All involved replacing toImage with toCanvas. I also renamed image to canvas for variable names. Testing the sims, I saw no problems or visual differences from published versions. Please check my work.

[jonathanolson]

Please check my work.

Scanned the commits, they look good.

[pixelzoom]

Thanks @jonathanolson. Assigned to you to complete replacement of deprecated methods.

[jonathanolson]

I realized (when looking at porting RasterizedTextNode) that use of rasterized() would be better if the Scenery Image produced had its localBounds overridden so that it precisely matched the original node's bounds (so it is a complete drop-in replacement for layout purposes). Since it wouldn't paint outside these bounds, it should be totally valid.

I'll plan to be adding that enhancement to rasterized() unless I hear objections.

[samreid]

Adopting localBounds sounds like the right thing to do.

[jonathanolson]

Added that as a feature (default true) above. I'll continue with porting to use that.

[jonathanolson]

@jbphet, I was looking into porting RasterizedTextNode for this, however it looks like it is actually using the scaled-up bounds (the label that gets rasterized) for the result. For this instance, it would be good to collaborate to determine what's going on.

[jonathanolson]

Discussed briefly with @jbphet, and I think he'll be looking into working on RasterizedTextNode.

[jbphet]

@jonathanolson - I looked at RasterizedTextNode, and I think it was using the correct bounds, i.e. not the scaled-up bounds. I ran several tests, and it looks like localBounds is not affected by the scale, but bounds is. As such, the usage of the localBounds for the actual text node, regardless of the scale setting, appears correct. As a result, basically all I did was change the usage of toDataURLNodeSynchronous() to rasterized(). The behavior looked fine (same as before) in my testing.

[Denz1994]

Adding myself as an assignee to refactor usages of .toImage() M&S.

[jonathanolson]

I didn't do anything this week on this in favor of other high priorities.

[jonathanolson]

I didn't do anything this week on this in favor of other high priorities.

Same this week. Will probably get things fit in over the next week.

[Denz1994]

M&S is no longer using .toImage(). Instead .rasterized() is being used.

[jonathanolson]

After the a11y-tree work I'll plan to have more done on this.

[jonathanolson]

I'll respond to @jbphet's comment above.

[samreid]

Unassigning from dev-meeting until @jonathanolson is ready.