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

(demo) Remove bitmap resolution from asset library json #5475

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

ericrosenbaum
Copy link
Contributor

NOT TO BE MERGED

this is just an illustrative PR for @fsih

This is a variation of #5460, but with the added change of removing the bitmapResolution field from all assets. This change should theoretically be safe (because the information in that field is redundant or unnecessary), but I noticed that it caused a bug:

  • Add an SVG sprite from the library containing 2 or more costumes (e.g. "ball")
  • click the "next costume" block
  • note that the position of the sprite changes

My best guess is that this is caused by a line in the VM: https://github.com/LLK/scratch-vm/blob/develop/src/sprites/rendered-target.js#L712

basically, if the bitmapResolution is missing when you add a costume, it sets it to 2. A couple lines later, it divides rotation center by bitmapResolution. So for SVGs with a missing bitmapResolution, the rotation center gets incorrectly changed

@adroitwhiz
Copy link
Contributor

I think it may be worth mentioning that the VM doesn't actually need to ever set the rotation centers after loading them initially, so the buggy VM code you mentioned can be completely removed.

@fsih
Copy link
Contributor

fsih commented Feb 28, 2020

Blocked on scratchfoundation/scratch-vm#2314

@fsih
Copy link
Contributor

fsih commented Apr 2, 2020

@ericrosenbaum I tested this out and I think it can be merged now!

@ericrosenbaum
Copy link
Contributor Author

@fsih cool! this is actually on top of #5460, which is still in review, though, so it will have to come after that one

@chrisgarrity
Copy link
Contributor

@ericrosenbaum should this just get closed now?

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.

4 participants