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

Reference/resolve variables by ID instead of name #89

Merged
merged 13 commits into from
Feb 26, 2023

Conversation

towerofnix
Copy link
Member

@towerofnix towerofnix commented Feb 17, 2023

This is a major change to the way sb-edit reads and serializes variables.

This PR changes BlockInput.Variable and BlockInput.List so their value is a pair of values (in an object), { id: string, name: string }, instead of one value (just the name). The fromSb3 deserializing code pulls from the IDs in the actual sb3 file, and all serializing code is updated to respect the new format — most notably, this resolves issues in toLeopard surrounding multiple variables existing of different IDs but the same name.

The last commit, af5280b, adds a final pass to fromSb3 deserialization so that variables which are never referenced in blocks nor shown as monitors are totally wiped from the code — this is a bit of a presumptuous decision and means sb3 -> sb3 conversion is explicitly not a one-to-one conversion, but I think that's an okay precedent to set, and it seemed to make more sense to run this pass on input data in fromSb3 rather than preprocessing in toLeopard. (In the future, I'm considering separating this chunk of code out so it's one of a modular set of "processing" prefabs including other automatic code optimizations, but that would be for a separate PR.)

@adroitwhiz I added you as a PR reviewer, but feel free to pass if you aren't interested or don't currently have the time!

@towerofnix
Copy link
Member Author

Will need to regen snapshots for this PR and the follow-up one, rn I am crashing for the night! 🐌

@PullJosh
Copy link
Collaborator

PullJosh commented Feb 20, 2023

Hey! I have been looking at this code (and your other PRs) in my moments of free time... It will take me a bit longer to review everything, but I just wanted to send you a message to say a huge thanks for the contribution. 😄

I really appreciate you continuing to contribute fixes! ❤️

@towerofnix
Copy link
Member Author

No problem at all! I've been enjoying popping back into this codebase. Leopard is really one of my favorite projects surrounding Scratch, so I'm glad to have the opportunity to poke around the code and improve it! ✨

Copy link
Collaborator

@PullJosh PullJosh 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 to me! :) I left two one comment that is worth reading, but nothing that blocks a merge in my opinion.

@towerofnix I'll leave it up to you to actually merge the PR because you have multiple PRs open and I don't know which order will be easiest. (Once all the changes have been merged, I am happy to handle publishing the updates to npm and redeploying the Leopard website.)

src/io/leopard/toLeopard.ts Show resolved Hide resolved
src/io/leopard/toLeopard.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@adroitwhiz adroitwhiz left a comment

Choose a reason for hiding this comment

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

The logic looks good overall. My main concern is that the code is written in a style that's a bit too functional for JS in my opinion. While functional programming languages like Haskell optimize chained map/filter/etc. operations on lists, JS engines will happily create every intermediate array eagerly.

My other nitpick (not addressed in the individual review comments) is that the existing codebase uses T[] to represent arrays, while this PR uses Array<T>; it's probably better to match the existing coding style.

Also, it looks like Prettier hasn't been run over the code yet (npm run format).

src/io/leopard/toLeopard.ts Show resolved Hide resolved
src/io/sb3/fromSb3.ts Outdated Show resolved Hide resolved
src/io/sb3/fromSb3.ts Outdated Show resolved Hide resolved
src/io/sb3/fromSb3.ts Outdated Show resolved Hide resolved
src/io/leopard/toLeopard.ts Outdated Show resolved Hide resolved
@towerofnix
Copy link
Member Author

Thanks, both! I've gone over your code reviews. The most notable change is in the implementation for Target.blocks — a property which I updated for this purpose but forgot to push to this branch! — matching your suggestion of recursively pushing to an "all blocks" collector array instead of spreading/flat-mapping intermediates, @adroitwhiz.

I've also made similar changes and optimizations throughout, see my recent commits. Most notable: caching a set of stage variables instead of iterating over target.variables every time, as well as entirely short-circuiting the set check if the target is the stage.

Copy link
Collaborator

@adroitwhiz adroitwhiz 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 now! I didn't even realize Target.blocks existed.

@towerofnix towerofnix merged commit 9f8f12d into leopard-js:master Feb 26, 2023
@towerofnix towerofnix deleted the var-ids branch February 26, 2023 21:20
@towerofnix
Copy link
Member Author

@adroitwhiz Yep, it wasn't visible when you reviewed because I pushed it to compat (which is based off this branch) and forgot to push it here too. 📦

Thanks both for the reviews!

@PullJosh I've merged this pull request. It's not dependent on any other PRs, as my other pull requests are either independent or support for #90. They should all be reviewed, but here's a summary of their relationships to help guide you:

Since this pull request fixes user-visible bugs (failing to import some projects such as seen in #85 and leopard-js/leopard#162) and does not depend on any other pull requests, it's ready and useful to end users to release a version of sb-edit before other pull requests are reviewed.

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