Skip to content

Commit

Permalink
add validation
Browse files Browse the repository at this point in the history
  • Loading branch information
ansis committed Nov 27, 2019
1 parent c93bbbd commit 3e1038c
Showing 1 changed file with 36 additions and 1 deletion.
37 changes: 36 additions & 1 deletion src/render/image_manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import potpack from 'potpack';

import {Event, Evented} from '../util/evented';
import {Event, ErrorEvent, Evented} from '../util/evented';
import {RGBAImage} from '../util/image';
import {ImagePosition} from './image_atlas';
import Texture from './texture';
Expand Down Expand Up @@ -86,10 +86,45 @@ class ImageManager extends Evented {
}

addImage(id: string, image: StyleImage) {
this._validate(id, image);

This comment has been minimized.

Copy link
@arindam1993

arindam1993 Nov 27, 2019

Contributor

if the validation fails, the function will still go onto execute since none of the errors are thrown, do we want that?

This comment has been minimized.

Copy link
@ansis

ansis Nov 27, 2019

Author Contributor

I'm not sure, but I think it might be better to attempt to render rather than show a blank map. What are your thoughts?

This comment has been minimized.

Copy link
@arindam1993

arindam1993 Nov 27, 2019

Contributor

Well, the map should't be blank, it'd just fail at adding the image? and aren't we robust at rendering if an image mentioned in a style doesnt exist.

This comment has been minimized.

Copy link
@ahk

ahk Nov 27, 2019

Contributor

You could return a boolean and then not add the image to the list, while still rendering the rest of the map, while also allowing the Errors to log out each of the failures found. Alternatively you could make an interface that returns a list of errors and check that it's empty or print/fire them.

This comment has been minimized.

Copy link
@ansis

ansis Nov 27, 2019

Author Contributor

yep! image skipping added in 40e86d3

assert(!this.images[id]);
this.images[id] = image;
}

_validate(id: string, image: StyleImage) {

This comment has been minimized.

Copy link
@ahk

ahk Nov 27, 2019

Contributor

Would it be useful to return the failing value also as part of the error message?

This comment has been minimized.

Copy link
@ansis

ansis Nov 27, 2019

Author Contributor

combining this discussion with 3e1038c#r36169156

if (!this._validateStretch(image.stretchX, image.data && image.data.width)) {
this.fire(new ErrorEvent(new Error(`Image "${id}" has invalid "stretchX" value`)));
}
if (!this._validateStretch(image.stretchY, image.data && image.data.height)) {
this.fire(new ErrorEvent(new Error(`Image "${id}" has invalid "stretchY" value`)));
}
if (!this._validateContent(image.content, image)) {
this.fire(new ErrorEvent(new Error(`Image "${id}" has invalid "content" value`)));
}
}

_validateStretch(stretch: ?Array<Array<number>> | void, size: number) {
if (!stretch) return true;
let last = 0;
for (const part of stretch) {
if (part[0] < last || part[1] < part[0] || size < part[1]) return false;
last = part[1];
}
return true;
}

_validateContent(content: ?Array<number> | void, image: StyleImage) {
if (!content) return true;
if (content.length !== 4) return false;
if (content[0] < 0 || image.data.width < content[0]) return false;
if (content[1] < 0 || image.data.height < content[1]) return false;
if (content[2] < 0 || image.data.width < content[2]) return false;
if (content[3] < 0 || image.data.height < content[3]) return false;
if (content[2] < content[0]) return false;
if (content[3] < content[1]) return false;
return true;
}

updateImage(id: string, image: StyleImage) {
const oldImage = this.images[id];
assert(oldImage);
Expand Down

0 comments on commit 3e1038c

Please sign in to comment.