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

Use bitmapAdapter for costume upload #2575

Merged
merged 8 commits into from
Jul 12, 2018

Conversation

fsih
Copy link
Contributor

@fsih fsih commented Jul 10, 2018

Resolves

Work toward scratchfoundation/scratch-svg-renderer#36
Depends on scratchfoundation/scratch-svg-renderer#43
Depends on scratchfoundation/scratch-vm#1315

Also fixes some places where we use info instead of info/2 for the rotation center. Info[0] appears to be costume width and info[1] appears to be costume height.

Proposed Changes

Attach bitmapAdapter
Use bitmapAdapter for file upload. The api is slightly different from the old importBitmap

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

  • Chrome
  • Firefox
  • Safari

Windows

  • Chrome
  • Firefox
  • Edge

Chromebook

  • Chrome

iPad

  • Safari

Android Tablet

  • Chrome

@fsih fsih added this to the July 2018 milestone Jul 10, 2018
@fsih fsih requested a review from kchadha July 10, 2018 14:47
@fsih
Copy link
Contributor Author

fsih commented Jul 10, 2018

The branch is up! This branch includes the new loading logic as well as all the bitmap assets, so you can actually load them.
After:
https://llk.github.io/scratch-gui/bitmapLibraryWithBitmapLoad/index.html

For comparison, here is the branch that just adds the bitmap assets.
Before:
https://llk.github.io/scratch-gui/bitmap/

Things to test:

  • Anna, Calvert, Cassy, Dan, Football, Lamp are sprites / costumes with bitmap resolution 1, so they are double the size with the new loader
  • Test that uploads of all file types and sizes and from camera get resized the same way in Before and After, and don't become blurry (jimp replacement was successful)

Question marks:

  • Jodi and Sam have file type gif so they don't load in the paint editor on either version currently
  • What does info[] mean?
    • In Before, when uploading individual costumes (not sprites), the rotation center is set to the bottom right corner instead of the center. info[0] is height and info[1] is width, not rotation center (try adding AZ pop down). This has been corrected in After
    • I think for other costumes, info[0] does indeed mean rotationCenterX and info[1] is rotationCenterY, because now in After, some costumes are not centered correctly (e.g. unicorn)

/cc @thisandagain

@paulkaplan paulkaplan mentioned this pull request Jul 12, 2018
9 tasks
Copy link
Contributor

@kchadha kchadha left a comment

Choose a reason for hiding this comment

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

As discussed offline, the rotation center should be updated to divide the info values by 2 for bitmap costumes (and all backdrops?) and leave the values as is for vector costumes.

@fsih fsih assigned kchadha and unassigned fsih Jul 12, 2018
@fsih fsih requested a review from kchadha July 12, 2018 19:25
@@ -26,10 +26,13 @@ class CostumeLibrary extends React.PureComponent {
]);
}
handleItemSelected (item) {
const type = item.md5.split('.')[1];

This comment was marked as abuse.

@fsih
Copy link
Contributor Author

fsih commented Jul 12, 2018

Made a new issue for the orientation issue:
scratchfoundation/scratch-paint#554

fsih added a commit to scratchfoundation/scratch-svg-renderer that referenced this pull request Jul 12, 2018
kchadha
kchadha previously approved these changes Jul 12, 2018
Copy link
Contributor

@kchadha kchadha left a comment

Choose a reason for hiding this comment

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

LGTM pending the package.json updates needed to make the build pass.

@kchadha kchadha removed their assignment Jul 12, 2018
fsih added a commit to scratchfoundation/scratch-vm that referenced this pull request Jul 12, 2018
Don't depend on this until scratchfoundation/scratch-gui#2575 is in

Break out loadVector and loadBitmap from loadCostumeAsset, and handle bitmap resolution 1 assets in loadBitmap
@fsih fsih merged commit 4fc8a0d into scratchfoundation:develop Jul 12, 2018
@fsih fsih deleted the bitmapAdapter branch July 12, 2018 23:14
bcjordan pushed a commit to mitmedialab/prg-raise-playground that referenced this pull request Jan 13, 2020
Don't depend on this until scratchfoundation/scratch-gui#2575 is in

Break out loadVector and loadBitmap from loadCostumeAsset, and handle bitmap resolution 1 assets in loadBitmap
paulkaplan pushed a commit to paulkaplan/scratch-labs-test that referenced this pull request Sep 18, 2020
Don't depend on this until scratchfoundation/scratch-gui#2575 is in

Break out loadVector and loadBitmap from loadCostumeAsset, and handle bitmap resolution 1 assets in loadBitmap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants