Skip to content

Block serialization compression #1031

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

Merged

Conversation

kchadha
Copy link
Contributor

@kchadha kchadha commented Apr 6, 2018

Resolves

#194

Proposed Changes

This is the last of the save/load work. This pull request covers the following changes:

  • New list limit of 200k items per list.
  • Make sb3 format more compact. This includes:
    • serializing the following block opcodes into a more compact form ([number representing opcode, info about the opcode, (in one or more elements)]:
      • math_number
      • math_positive_number
      • math_whole_number
      • math_angle
      • math_integer
      • colour_picker
      • text
      • event_broadcast_menu (this is the shadow block used in broadcast menus)
      • data_variable
      • data_listcontents
    • reducing duplication in how our data is structured, for example removing serialization of the id-uid pair in the blocks, which were previously stored as:
      blocks: {
          'this is my long uid for my block' : {
               opcode: 'a cool block'
               id: 'this is my long uid for my block'
               ...  more stuff here ...
      }
      
  • Added serialization and deserialization for custom state such as tempo, volume, and video related state

Note: Monitor serialization/deserialization is not covered by this work.

Reason for Changes

Making serialized 3.0 format smaller.

Test Coverage

Existing save/load tests pass. New test cases for the 3.0 format to be added in the near future.

Related PRs

scratchfoundation/scratch-parser#32
scratchfoundation/scratch-gui#1738

Copy link
Contributor

@thisandagain thisandagain left a comment

Choose a reason for hiding this comment

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

This looks really good. Thanks for documenting this so thoroughly. 👍

@towerofnix
Copy link
Contributor

Out of curiosity's sake, what's the purpose of limiting a list to 200k items? Sure, that's an absurdly huge amount that I doubt will ever cause issues! But I'm curious anyways. Were gigantic lists causing problems?

@thisandagain thisandagain added this to the April 2018 milestone Apr 9, 2018
@kchadha
Copy link
Contributor Author

kchadha commented Apr 9, 2018

@towerofnix That's a good question! In 3.0, we are changing the way we store projects on the server to allow bigger projects. There are currently some projects in 2.0 that have very long lists (some for games, but others just to test the limits of a project's size). We want to continue to support these projects, but given that we'll be able to save and load much bigger projects in the future, we don't want to also allow even bigger lists because this will increase browser memory usage while attempting to run one of these projects (or display a monitor for these lists, or scroll through the blocks flyout, etc.). Ultimately, this would cause more browser crashes, which we want to avoid. We're hoping that a 200k limit is a good balance where most projects that currently use large lists (games, etc.) still run well, but we're not actively giving way to crashing more user's browsers.

Copy link
Contributor

@rschamp rschamp left a comment

Choose a reason for hiding this comment

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

Looks great overall, thanks for all the explanatory comments!

obj.opcode = block.opcode;
// NOTE: this is extremely important to serialize even if null;
// not serializing `next: null` results in strange behavior

This comment was marked as abuse.

obj.topLevel = block.topLevel ? block.topLevel : false;
obj.shadow = block.shadow;
obj.shadow = block.shadow; // I think we don't need this either..

This comment was marked as abuse.

This comment was marked as abuse.

}
};
primitiveObj.topLevel = false;
// what should we do about shadows

This comment was marked as abuse.

This comment was marked as abuse.

'ON-FLIPPED': 'on-flipped'
OFF: 'off',
ON: 'on',
ON_FLIPPED: 'on_flipped'

This comment was marked as abuse.

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.

Looks good!

assetType = storage.AssetType.ImageVector;
} else if (costumeFormat === 'png') {
dataFormat = storage.DataFormat.PNG;
} else if (['png', 'bmp', 'jpeg', 'jpg', 'gif'].indexOf(costumeFormat) >= 0) {

This comment was marked as abuse.

@@ -1,13 +1,15 @@
/**
* @fileoverview
* Partial implementation of a SB3 serializer and deserializer. Parses provided
* An SB3 serializer and deserializer. Parses provided

This comment was marked as abuse.

This comment was marked as abuse.

obj.inputs = block.inputs;
obj.fields = block.fields;
obj.inputs = serializeInputs(block.inputs);
obj.fields = serializeFields(block.fields);
obj.topLevel = block.topLevel ? block.topLevel : false;
obj.shadow = block.shadow;

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@@ -69,17 +306,22 @@ const serializeCostume = function (costume) {
// but that change should be made carefully since it is very
// pervasive
obj.md5ext = costume.md5;
obj.dataFormat = costume.dataFormat;
obj.dataFormat = costume.dataFormat.toLowerCase();

This comment was marked as abuse.

…udio engine present. This is analagous to costumes getting loaded even if there is no renderer present.
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.

👍 to the sound change, too!

@kchadha kchadha merged commit 8739a52 into scratchfoundation:develop Apr 11, 2018
@towerofnix towerofnix mentioned this pull request May 26, 2018
@Honyant
Copy link

Honyant commented Mar 22, 2019

@towerofnix That's a good question! In 3.0, we are changing the way we store projects on the server to allow bigger projects. There are currently some projects in 2.0 that have very long lists (some for games, but others just to test the limits of a project's size). We want to continue to support these projects, but given that we'll be able to save and load much bigger projects in the future, we don't want to also allow even bigger lists because this will increase browser memory usage while attempting to run one of these projects (or display a monitor for these lists, or scroll through the blocks flyout, etc.). Ultimately, this would cause more browser crashes, which we want to avoid. We're hoping that a 200k limit is a good balance where most projects that currently use large lists (games, etc.) still run well, but we're not actively giving way to crashing more user's browsers.

My problem with this is that my only way past this limit, which I need for like a video playback thing is to use a method that combines multiple lists, which is harder to manage and expand. So either way, it will crash the browser, so I think lists should still be given infinite length or at least be given an option to do so.

@kchadha
Copy link
Contributor Author

kchadha commented Mar 22, 2019

@Honyant, this pull request was merged almost a year ago, so any new conversation on here will not be easy to track. Please feel free to create an issue for your request.

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.

None yet

6 participants