Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move SVG renderer logic back into SVGSkin #745

Merged

Conversation

adroitwhiz
Copy link
Contributor

Depends on scratchfoundation/scratch-svg-renderer#213

Resolves

A step towards #550

Proposed Changes

This PR moves the logic for actually rendering SVGs back into SVGSkin from SvgRenderer, and removes the dependency on the SvgRenderer class.

Reason for Changes

This will allow for future modifications to the way SVGs are drawn, which are necessary to fix #550.

Test Coverage

Tested manually

@adroitwhiz adroitwhiz force-pushed the remove-svgrenderer-dependency branch from 1a6233b to a9d70a5 Compare January 8, 2021 02:40
src/SVGSkin.js Outdated Show resolved Hide resolved
@adroitwhiz adroitwhiz force-pushed the remove-svgrenderer-dependency branch from c9c2e9e to 40ad5d3 Compare March 4, 2021 15:25
src/SVGSkin.js Outdated Show resolved Hide resolved
This will allow for fancier stuff to be done with the SVG viewbox in the
future to avoid subpixel jitter.
@adroitwhiz adroitwhiz force-pushed the remove-svgrenderer-dependency branch from 40ad5d3 to 801b0da Compare March 4, 2021 16:50
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this, and sorry it took us so long to review it!
There's one small change I've requested just to make sure it's safe but overall it looks pretty good

src/SVGSkin.js Outdated Show resolved Hide resolved
@adroitwhiz adroitwhiz requested a review from cwillisf June 1, 2021 22:04
Instead of creating a new image every time `setSVG` is called and
checking whether that image is complete with `loaded`, reuse the same
image, override its `src`, & use `onload` instead of `addEventListener`
to replace the previous event listener. This is necessary to avoid race
conditions with the `_svgImageLoaded` flag.
@adroitwhiz adroitwhiz force-pushed the remove-svgrenderer-dependency branch from e5df8b5 to 7af5b3e Compare June 10, 2021 12:56
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subpixel SVG costume handling: a writeup
2 participants