-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
add warning for integer overflow in the SymbolInstancesArray #2966
Conversation
Language looks good to me! Add a couple test cases and |
Apologies for the failed CI build. This happened while we were upgrading our CI infrastructure per #2958. If you rebase on |
9c3acda
to
831c268
Compare
test added @lucaswoj ty for the constant suggestion! 🎈 |
t.equal(bucketC.populateArrays(collision, stacks), undefined); | ||
t.equal(numWarnings, 2, 'integer overflow warning is triggered when glyph and/or symbol quad exceeds MAX_QUADS'); | ||
// Put it back | ||
console.warn = warn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend using sinon.spy
on util.warnOnce
instead of monkey-patching console.warn
. Sinon gives us a nicer API for making assertions about what warning messages have been emitted, gives us easy restoration of the original method, and allows real console.warn
messages to make their way to the console where they belong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this approach from here after searching for other tests that were looking for warnings. I will look into sinon for a refactor.
831c268
to
9534060
Compare
@lucaswoj I implemented your suggestions, but should the warning be actually printing to the console when I run |
Ideally not. You should be able to work around this by replacing |
9d238ce
to
91629a3
Compare
🚢 on ✅ ? |
@@ -594,6 +599,8 @@ SymbolBucket.prototype.addSymbolInstance = function(anchor, line, shapedText, sh | |||
|
|||
var iconBoxStartIndex = iconCollisionFeature ? iconCollisionFeature.boxStartIndex : this.collisionBoxArray.length; | |||
var iconBoxEndIndex = iconCollisionFeature ? iconCollisionFeature.boxEndIndex : this.collisionBoxArray.length; | |||
if (iconQuadEndIndex > this.MAX_QUADS) util.warnOnce("Too many symbols being rendered in a tile. See https://github.com/mapbox/mapbox-gl-js/issues/2907"); | |||
if (glyphQuadEndIndex > this.MAX_QUADS) util.warnOnce("Too many glyphs being rendered in a tile. See https://github.com/mapbox/mapbox-gl-js/issues/2907"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work, because this.MAX_QUADS
refers to the instance variable, while SymbolBucket.MAX_QUADS
is a static property of SymbolBucket
.
add warning for integer overflow in the SymbolInstancesArray
…e sinon.spy for testing the warning
91629a3
to
2b5ba51
Compare
good call @mourner -- should I reset the value of |
@mollymerp yes, well noted! |
2b5ba51
to
dad7195
Compare
Looks good |
first stab at warning language 😁
the condition is based on the assumption that the max valid 16 bit unsigned integer is 65535
Checklist
master
ref #2907
this branch
buffer: 1,005 ms
fps: 60 fps
frame-duration: 7.8 ms, 0% > 16ms
query-point: 0.45 ms
query-box: 36.59 ms
geojson-setdata-small: 4 ms
geojson-setdata-large: 107 ms
master
buffer: 909 ms
fps: 60 fps
frame-duration: 7.7 ms, 0% > 16ms
query-point: 0.41 ms
query-box: 40.46 ms
geojson-setdata-small: 6 ms
geojson-setdata-large: 97 ms