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

Call fixupSvgString on all SVGs #219

Open
fsih opened this issue Feb 5, 2021 · 5 comments
Open

Call fixupSvgString on all SVGs #219

fsih opened this issue Feb 5, 2021 · 5 comments

Comments

@fsih
Copy link
Contributor

fsih commented Feb 5, 2021

Expected Behavior

fixupSvgString fixes common issues that make SVGs invalid. Probably any SVG uploaded to Scratch should have these fixes applied. This way, Scratch would support more SVGs.
https://github.com/LLK/scratch-svg-renderer/blob/9ebf57588aa596c4fa3bb64209e10ade395aee90/src/fixup-svg-string.js#L51

Actual Behavior

fixupSvgString is currently only called if optVersion == 2 here (i.e. when loading a sb2 or sprite2)
https://github.com/LLK/scratch-vm/blob/e63993257210ce9f3a66ac764f7f4b1941f63fa5/src/import/load-costume.js#L10

Describe what actually happens

Steps to Reproduce

Explain what someone needs to do in order to see what's described in Actual behavior above

Operating System and Browser

e.g. Mac OS 10.11.6 Safari 10.0

@kchadha
Copy link
Contributor

kchadha commented Feb 18, 2021

From our discussion offline -- would this cause us to resave all of our vector assets?

@fsih
Copy link
Contributor Author

fsih commented Feb 19, 2021

We should check to not resave if the asset hasn't been changed by fixup SVG string. Then as long as the SVG doesn't have any forbidden elements, it won't be resaved. There shouldn't be too many SVGs like that. See https://github.com/LLK/scratch-svg-renderer/blob/develop/src/fixup-svg-string.js

@apple502j
Copy link

Currently all v2 SVGs are marked as dirty. We can fix this.
0001-Always-call-v2SvgAdapter.patch.txt

@apple502j
Copy link

@fsih It seems like there are still some "question mark" problems lying around (according to BaG) - I hope this is why?

See above for solutions to bandwidth problem. (am I the only one who thinks the 5 million dollar from Google should be spent on general maintenance as well, not just the webinar?)

@fsih
Copy link
Contributor Author

fsih commented Apr 20, 2021

Reverted by scratchfoundation/scratch-vm#3026

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

No branches or pull requests

4 participants