Clean up PenSkin code

[adroitwhiz]

This PR depends on #438 and #475.

Resolves

Resolves #450

Proposed Changes

This PR removes ~270 lines of unnecessary/obsolete code from PenSkin, greatly simplifying it and possibly improving performance:

• Remove _canvas, drawStamp, _setAttributes
  • All pen drawing is now done in GL, so there is no need for a 2D canvas context to draw things on, or functions to draw things to it.
  • Without a canvas context to call createImageData on (to create silhouette data), the ImageData constructor is used instead. Note that MDN lists support for said constructor in Safari as "unknown". I've confirmed that the ImageData constructor works in iOS Safari 13, but I haven't tested on Safari 11. However, this constructor is already used for video in the VM, so this isn't its first use in the codebase.
  • Instead of getting/setting the canvas' size in places that require it, a _size property has been added to PenSkin.
• Remove _bounds
  • This can also be replaced with _size.
• Remove _silhouetteBuffer
  • It turns out this was never actually used.
• Remove _exportTexture and _drawToBuffer
  • Instead of going through the trouble of drawing the framebuffer texture to _exportTexture when getTexture is called, just expose the framebuffer texture as the skin's texture.
  • Note that updateSilhouette now directly sets the framebuffer, instead of going through a draw region that unsets it afterwards. This behavior is already present in clear, so it shouldn't cause new problems.
• Remove _stampShader + _drawRectangle and its draw region
  • These were only used by _enterDrawToBuffer and _drawToBuffer.
• Remove __modelTranslationMatrix, __modelScalingMatrix, __modelMatrix, __modelTranslationVector, __modelScalingVector
  • These were only used by _drawRectangle.
• Remove __projectionMatrix
  • Similarly, only the rectangle-drawing code used this. It was also passed as a uniform to the line-drawing code, but was unused.
• Pass u_stageSize in _enterDrawLineOnBuffer instead of _drawLineOnBuffer
  • This avoids passing the same uniform in each time.
• Inline _createLineGeometry
  • Since it's only called once (in the PenSkin constructor), I believe inlining it makes the code more clear--you don't have to scroll around as much to see what really happens in the constructor.
• Remove DefaultPenAttributes
  • This is in a separate commit because it may be a bit riskier, but the VM should always provide default pen attributes, making DefaultPenAttributes redundant.

Reason for Changes

These changes improve ergonomics and make the code easier to improve in the future. They may also increase performance.

[fsih]

It looks like at https://github.com/LLK/scratch-vm/blob/9329f93e74ffbb924b12f318a41db69e1387db74/src/extensions/scratch3_pen/index.js#L188 we expect penState to have a penAttributes attribute, otherwise it will crash, which isn't guaranteed when pen state is set

It also seems like when pen state is set, we don't guarantee that penAttributes properly has the .diameter and .color4f attributes, which I guess is why DefaultPenAttributes existed. Seems like where customState is set, we should be verifying that the format of the object is correct in order to safely remove this safety check (that's all in VM though)

[adroitwhiz]

I'll just leave out the default pen attributes changes for now until that can be revisited in the VM

[fsih]

We want to make sure we are always using draw regions for consistency/protection, so rather than changing this to be like clear (by moving bindFrameBufferInfo outside of the enterDrawRegion function), we'd rather change clear to call enterDrawRegion.

Can you explain the logic behind removing the this._drawRectangleRegionEnter(this._stampShader, this._bounds); step, which used t be called by enterDrawRegion? Are those always guaranteed to be no-ops? What makes sure that state is set?

[adroitwhiz]

It looks like _drawRectangleRegionEnter sets the viewport and shader.

Calling clear on the PenSkin just calls gl.clear, which requires the framebuffer to be set but does not depend on the current viewport or shader.

Likewise for updateSilhouette which calls gl.readPixels, which also only requires the correct framebuffer to be bound.

The code for stamping lives in RenderWebGL and never called this code so it shouldn't be affected--it uses the same code path as drawing to the stage and sets the viewport + framebuffer itself.

Finally, the code for pen lines uses an existing draw region which sets the viewport, framebuffer, and shader.

[cwillisf]

👍 thanks for the explanations (and your patience...)

[fsih]

LG, thanks!