perform tilemap upgrade on xml instead of workspace #10368
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Nice |
srietkerk
reviewed
Feb 5, 2025
|
|
||
| const upgradedTileMapping: pxt.Map<pxt.Tile> = {}; | ||
|
|
||
| for (const element of dom.children) { |
There was a problem hiding this comment.
Just curious if iterating over everything in the dom is the most efficient way to do this.. Would this or would filtering for elements with the variable tag and then iterating through those entries be faster?
There was a problem hiding this comment.
i don't think it would make a huge difference either way. this is only looping over the top-level elements in the xml file so it should only be the variables and any top-level events like on-start
srietkerk
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes microsoft/pxt-arcade#6602
fixes microsoft/pxt-arcade#6635
fixes microsoft/pxt-arcade#6601
our old method of upgrading tilemaps doesn't seem to be working with the new blockly; when blockly sets the values of the created fields the field validation code is causing their values to be overwritten before we can perform our upgrades.
to bypass that issue, i converted the upgrade code to be performed directly on the blocks xml instead of the workspace.
also, since people might not be familiar with this ancient code, this is the upgrade logic that i put into place ages ago when we started having a concept of "assets" in projects. previously we stored the tileset in the variables of the blockly workspace and that's what the tileset field drew its options from. tilemap blocks also just inlined all of the tilemap data instead of compiling down to an asset reference