Skip to content

Commit f24235b

Browse files
riknollCopilot
andauthored
[Stable11.3] Cherry pick asset fixes (#10522)
* try to validate assets before committing them (#10480) * fix temporary asset ids when converting to named assets (#10507) * fix temporary asset ids when they're named * Update pxteditor/monaco-fields/field_tilemap.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix copilot typing error --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 937e4c7 commit f24235b

File tree

10 files changed

+140
-32
lines changed

10 files changed

+140
-32
lines changed

pxtblocks/fields/field_animation.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ export class FieldAnimationEditor extends FieldAssetEditor<FieldAnimationOptions
8080
const frames = parseImageArrayString(text, this.params.taggedTemplate);
8181

8282
if (frames && frames.length) {
83-
const id = this.sourceBlock_.id;
83+
const id = this.temporaryAssetId();
8484

8585
const newAnimation: pxt.Animation = {
8686
internalID: -1,
@@ -97,7 +97,7 @@ export class FieldAnimationEditor extends FieldAssetEditor<FieldAnimationOptions
9797
if (asset) return asset;
9898
}
9999

100-
const id = this.sourceBlock_.id;
100+
const id = this.temporaryAssetId();
101101
const bitmap = new pxt.sprite.Bitmap(this.params.initWidth, this.params.initHeight).data()
102102

103103
const newAnimation: pxt.Animation = {

pxtblocks/fields/field_asset.ts

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ export abstract class FieldAssetEditor<U extends FieldAssetEditorOptions, V exte
264264
}
265265

266266
protected onFieldEditorHide(fv: pxt.react.FieldEditorView<pxt.Asset>) {
267-
const result = fv.getResult();
267+
let result = fv.getResult();
268268
const project = pxt.react.getTilemapProject();
269269

270270
if (this.asset.type === pxt.AssetType.Song) {
@@ -275,15 +275,10 @@ export abstract class FieldAssetEditor<U extends FieldAssetEditorOptions, V exte
275275
const old = this.getValue();
276276
if (pxt.assetEquals(this.asset, result)) return;
277277

278-
const oldId = isTemporaryAsset(this.asset) ? null : this.asset.id;
279-
let newId = isTemporaryAsset(result) ? null : result.id;
278+
result = pxt.patchTemporaryAsset(this.asset, result, project);
280279

281-
if (!oldId && newId === this.sourceBlock_.id) {
282-
// The temporary assets we create just use the block id as the id; give it something
283-
// a little nicer
284-
result.id = project.generateNewID(result.type);
285-
newId = result.id;
286-
}
280+
const oldId = isTemporaryAsset(this.asset) ? null : this.asset.id;
281+
const newId = isTemporaryAsset(result) ? null : result.id;
287282

288283
this.pendingEdit = true;
289284

@@ -552,6 +547,10 @@ export abstract class FieldAssetEditor<U extends FieldAssetEditorOptions, V exte
552547
protected isFullscreen() {
553548
return true;
554549
}
550+
551+
protected temporaryAssetId() {
552+
return this.sourceBlock_.id + "_" + this.name;
553+
}
555554
}
556555

557556
function isTemporaryAsset(asset: pxt.Asset) {

pxtblocks/fields/field_musiceditor.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ export class FieldMusicEditor extends FieldAssetEditor<FieldMusicEditorOptions,
6161

6262
const newAsset: pxt.Song = {
6363
internalID: -1,
64-
id: this.sourceBlock_.id,
64+
id: this.temporaryAssetId(),
6565
type: pxt.AssetType.Song,
6666
meta: {
6767
},

pxtblocks/fields/field_sprite.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ export class FieldSpriteEditor extends FieldAssetEditor<FieldSpriteEditorOptions
7070

7171
const newAsset: pxt.ProjectImage = {
7272
internalID: -1,
73-
id: this.sourceBlock_.id,
73+
id: this.temporaryAssetId(),
7474
type: pxt.AssetType.Image,
7575
jresData: pxt.sprite.base64EncodeBitmap(data),
7676
meta: {

pxteditor/monaco-fields/field_musiceditor.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,19 +7,21 @@ export class MonacoSongEditor extends MonacoReactFieldEditor<pxt.Song> {
77
protected isPython: boolean;
88
protected isAsset: boolean;
99
protected text: string;
10+
protected editing: pxt.Asset;
1011

1112
protected textToValue(text: string): pxt.Song {
1213
this.isPython = text.indexOf("`") === -1
1314
this.text = text;
1415

1516
const match = pxt.parseAssetTSReference(text);
1617
if (match) {
17-
const { type, name: matchedName } = match;
18+
const { name: matchedName } = match;
1819
const name = matchedName.trim();
1920
const project = pxt.react.getTilemapProject();
2021
this.isAsset = true;
2122
const asset = project.lookupAssetByName(pxt.AssetType.Song, name);
2223
if (asset) {
24+
this.editing = asset;
2325
return asset;
2426
}
2527
else {
@@ -29,6 +31,8 @@ export class MonacoSongEditor extends MonacoReactFieldEditor<pxt.Song> {
2931
newAsset.meta.displayName = name;
3032
}
3133

34+
this.editing = newAsset;
35+
3236
return newAsset;
3337
}
3438
}
@@ -39,18 +43,24 @@ export class MonacoSongEditor extends MonacoReactFieldEditor<pxt.Song> {
3943
const contents = hexLiteralMatch[1].trim();
4044

4145
if (contents) {
42-
return createFakeAsset(pxt.assets.music.decodeSongFromHex(contents));
46+
this.editing = createFakeAsset(pxt.assets.music.decodeSongFromHex(contents));
47+
}
48+
else {
49+
this.editing = createFakeAsset(pxt.assets.music.getEmptySong(2));
4350
}
4451

45-
return createFakeAsset(pxt.assets.music.getEmptySong(2));
52+
return this.editing;
4653
}
4754

4855
return undefined; // never
4956
}
5057

5158
protected resultToText(result: pxt.Song): string {
59+
const project = pxt.react.getTilemapProject();
60+
project.pushUndo();
61+
62+
result = pxt.patchTemporaryAsset(this.editing, result, project) as pxt.Song;
5263
if (result.meta?.displayName) {
53-
const project = pxt.react.getTilemapProject();
5464
if (this.isAsset || project.lookupAsset(result.type, result.id)) {
5565
result = project.updateAsset(result)
5666
} else {

pxteditor/monaco-fields/field_sprite.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,19 +7,21 @@ export class MonacoSpriteEditor extends MonacoReactFieldEditor<pxt.ProjectImage>
77
protected isPython: boolean;
88
protected isAsset: boolean;
99
protected template: string;
10+
protected editing: pxt.Asset;
1011

1112
protected textToValue(text: string): pxt.ProjectImage {
1213
this.isPython = text.indexOf("`") === -1
1314
this.template = text.startsWith("bmp") ? "bmp" : "img"
1415

1516
const match = pxt.parseAssetTSReference(text);
1617
if (match) {
17-
const { type, name: matchedName } = match;
18+
const { name: matchedName } = match;
1819
const name = matchedName.trim();
1920
const project = pxt.react.getTilemapProject();
2021
this.isAsset = true;
2122
const asset = project.lookupAssetByName(pxt.AssetType.Image, name);
2223
if (asset) {
24+
this.editing = asset;
2325
return asset;
2426
}
2527
else {
@@ -29,16 +31,23 @@ export class MonacoSpriteEditor extends MonacoReactFieldEditor<pxt.ProjectImage>
2931
newAsset.meta.displayName = name;
3032
}
3133

34+
this.editing = newAsset;
35+
3236
return newAsset;
3337
}
3438
}
3539

36-
return createFakeAsset(pxt.sprite.imageLiteralToBitmap(text, this.template));
40+
this.editing = createFakeAsset(pxt.sprite.imageLiteralToBitmap(text, this.template));
41+
42+
return this.editing;
3743
}
3844

3945
protected resultToText(result: pxt.ProjectImage): string {
46+
const project = pxt.react.getTilemapProject();
47+
project.pushUndo();
48+
result = pxt.patchTemporaryAsset(this.editing, result, project) as pxt.ProjectImage;
49+
4050
if (result.meta?.displayName) {
41-
const project = pxt.react.getTilemapProject();
4251
if (this.isAsset || project.lookupAsset(result.type, result.id)) {
4352
result = project.updateAsset(result)
4453
} else {

pxteditor/monaco-fields/field_tilemap.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@ const fieldEditorId = "tilemap-editor";
66
export class MonacoTilemapEditor extends MonacoReactFieldEditor<pxt.ProjectTilemap> {
77
protected isTilemapLiteral: boolean;
88
protected tilemapLiteral: string;
9+
protected editing: pxt.Asset;
910

1011
protected textToValue(text: string): pxt.ProjectTilemap {
1112
const tm = this.readTilemap(text);
1213

1314
const project = pxt.react.getTilemapProject();
1415
pxt.sprite.addMissingTilemapTilesAndReferences(project, tm);
1516

17+
this.editing = tm;
1618
return tm;
1719
}
1820

@@ -31,7 +33,7 @@ export class MonacoTilemapEditor extends MonacoReactFieldEditor<pxt.ProjectTilem
3133
// If the user is still typing, they might try to open the editor on an incomplete tilemap
3234
}
3335
return null;
34-
}
36+
}
3537
}
3638

3739
this.isTilemapLiteral = true;
@@ -71,7 +73,7 @@ export class MonacoTilemapEditor extends MonacoReactFieldEditor<pxt.ProjectTilem
7173
protected resultToText(asset: pxt.ProjectTilemap): string {
7274
const project = pxt.react.getTilemapProject();
7375
project.pushUndo();
74-
76+
asset = pxt.patchTemporaryAsset(this.editing, asset, project) as pxt.ProjectTilemap;
7577
pxt.sprite.updateTilemapReferencesFromResult(project, asset);
7678

7779
if (this.isTilemapLiteral) {

pxtlib/spriteutils.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ namespace pxt.sprite {
145145
}
146146
}
147147

148-
protected dataLength() {
148+
dataLength() {
149149
return Math.ceil(this.width * this.height / 2);
150150
}
151151
}
@@ -179,7 +179,7 @@ namespace pxt.sprite {
179179
this.buf[index] = value;
180180
}
181181

182-
protected dataLength() {
182+
dataLength() {
183183
return this.width * this.height;
184184
}
185185
}

pxtlib/tilemap.ts

Lines changed: 90 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,12 @@ namespace pxt {
154154
const existing = this.lookupByID(id);
155155

156156
if (!assetEquals(existing, newValue)) {
157+
if (!validateAsset(newValue) && validateAsset(existing)) {
158+
pxt.warn("Refusing to overwrite asset with invalid version");
159+
pxt.tickEvent("assets.invalidAssetOverwrite", { assetType: newValue.type });
160+
return existing;
161+
}
162+
157163
this.removeByID(id);
158164
asset = this.add(newValue);
159165
this.notifyListener(newValue.internalID);
@@ -1709,6 +1715,10 @@ namespace pxt {
17091715

17101716
export function cloneAsset<U extends Asset>(asset: U): U {
17111717
asset.meta = Object.assign({}, asset.meta);
1718+
if (asset.meta.temporaryInfo) {
1719+
asset.meta.temporaryInfo = Object.assign({}, asset.meta.temporaryInfo);
1720+
}
1721+
17121722
switch (asset.type) {
17131723
case AssetType.Tile:
17141724
case AssetType.Image:
@@ -1948,6 +1958,69 @@ namespace pxt {
19481958
return getShortIDCore(asset.type, asset.id);
19491959
}
19501960

1961+
export function validateAsset(asset: pxt.Asset) {
1962+
if (!asset) return false;
1963+
1964+
switch (asset.type) {
1965+
case AssetType.Image:
1966+
case AssetType.Tile:
1967+
return validateImageAsset(asset as ProjectImage | Tile);
1968+
case AssetType.Tilemap:
1969+
return validateTilemap(asset as ProjectTilemap);
1970+
case AssetType.Animation:
1971+
return validateAnimation(asset as Animation)
1972+
case AssetType.Song:
1973+
return validateSong(asset as Song);
1974+
}
1975+
}
1976+
1977+
function validateImageAsset(asset: ProjectImage | Tile) {
1978+
if (!asset.bitmap) return false;
1979+
1980+
return validateBitmap(sprite.Bitmap.fromData(asset.bitmap));
1981+
}
1982+
1983+
function validateTilemap(tilemap: ProjectTilemap) {
1984+
if (
1985+
!tilemap.data ||
1986+
!tilemap.data.tilemap ||
1987+
!tilemap.data.tileset ||
1988+
!tilemap.data.tileset.tileWidth ||
1989+
!tilemap.data.tileset.tiles?.length ||
1990+
!tilemap.data.layers
1991+
) {
1992+
return false;
1993+
}
1994+
1995+
return validateBitmap(sprite.Bitmap.fromData(tilemap.data.layers)) &&
1996+
validateBitmap(tilemap.data.tilemap);
1997+
}
1998+
1999+
function validateAnimation(animation: Animation) {
2000+
if (!animation.frames?.length || animation.interval <= 0) {
2001+
return false;
2002+
}
2003+
2004+
return !animation.frames.some(frame => !validateBitmap(sprite.Bitmap.fromData(frame)));
2005+
}
2006+
2007+
function validateBitmap(bitmap: sprite.Bitmap) {
2008+
return bitmap.width > 0 &&
2009+
bitmap.height > 0 &&
2010+
!Number.isNaN(bitmap.x0) &&
2011+
!Number.isNaN(bitmap.y0) &&
2012+
bitmap.data().data.length === bitmap.dataLength();
2013+
}
2014+
2015+
function validateSong(song: Song) {
2016+
return song.song &&
2017+
song.song.ticksPerBeat > 0 &&
2018+
song.song.beatsPerMeasure > 0 &&
2019+
song.song.measures > 0 &&
2020+
song.song.beatsPerMinute > 0 &&
2021+
!!song.song.tracks;
2022+
}
2023+
19512024
function getShortIDCore(assetType: pxt.AssetType, id: string, allowNoPrefix = false) {
19522025
let prefix: string;
19532026
switch (assetType) {
@@ -2062,12 +2135,6 @@ namespace pxt {
20622135
}
20632136
}
20642137

2065-
2066-
function set16Bit(buf: Uint8ClampedArray, offset: number, value: number) {
2067-
buf[offset] = value & 0xff;
2068-
buf[offset + 1] = (value >> 8) & 0xff;
2069-
}
2070-
20712138
function read16Bit(buf: Uint8ClampedArray, offset: number) {
20722139
return buf[offset] | (buf[offset + 1] << 8)
20732140
}
@@ -2087,4 +2154,21 @@ namespace pxt {
20872154
case AssetType.Song: return snapshot.songs;
20882155
}
20892156
}
2157+
2158+
export function patchTemporaryAsset(oldValue: pxt.Asset, newValue: pxt.Asset, project: TilemapProject) {
2159+
if (!oldValue || assetEquals(oldValue, newValue)) return newValue;
2160+
2161+
newValue = cloneAsset(newValue);
2162+
const wasTemporary = !oldValue.meta.displayName;
2163+
const isTemporary = !newValue.meta.displayName;
2164+
2165+
// if we went from being temporary to no longer being temporary,
2166+
// make sure we replace the junk id with a new value
2167+
if (wasTemporary && !isTemporary) {
2168+
newValue.id = project.generateNewID(newValue.type);
2169+
newValue.internalID = project.getNewInternalId();
2170+
}
2171+
2172+
return newValue;
2173+
}
20902174
}

webapp/src/components/assetEditor/assetSidebar.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,17 @@ class AssetSidebarImpl extends React.Component<AssetSidebarProps, AssetSidebarSt
9595

9696
const project = pxt.react.getTilemapProject();
9797
project.pushUndo();
98+
result = pxt.patchTemporaryAsset(this.props.asset, result, project);
99+
100+
if (result.meta.displayName) {
101+
result = project.updateAsset(result);
102+
}
103+
98104
if (!this.props.asset.meta?.displayName && result.meta.temporaryInfo) {
99105
getBlocksEditor().updateTemporaryAsset(result);
100106
pkg.mainEditorPkg().lookupFile(`this/${pxt.MAIN_BLOCKS}`).setContentAsync(getBlocksEditor().getCurrentSource());
101107
}
102108

103-
if (result.meta.displayName) project.updateAsset(result);
104-
105109
this.props.dispatchChangeGalleryView(GalleryView.User);
106110
this.updateAssets().then(() => simulator.setDirty());
107111
}

0 commit comments

Comments
 (0)