Use signed arithmetic to avoid underflow when shrinking window. #243

Merged
merged 1 commit into from Mar 29, 2016

Conversation

Projects
None yet
4 participants
@gmorenz
Contributor

gmorenz commented Mar 20, 2016

fixes #236

The underflow occurs when the y co-ordinate of viewport_size in the last render_api.set_root_stacking_context call is bigger than the y co-ordinate of framebuffer_size in a renderer.render call.

The problem is the calculated layer_origin y is negative, but we calculate it using unsigned ints, and then cast to signed just for the gl calls. The two places where this calculated value is used are gl::scissor and gl::viewport, both of these are perfectly happy with a negative origin point (as far as I know and can tell, but I don't have much experience with opengl), thus I've just changed it so we cast to signed integers early.

The other options would be to require the above doesn't happen, either by using an outdated bigger framebuffer size (which results in the contents of the window moving upwards as the window shrinks), or recalling set_root_stacking_context (which is less efficient).

@gmorenz

This comment has been minimized.

Show comment
Hide comment
@gmorenz

gmorenz Mar 20, 2016

Contributor

Build failure is caused by pinning rust-nightly version but not other crates (aster, quasi), as mentioned in #231

Contributor

gmorenz commented Mar 20, 2016

Build failure is caused by pinning rust-nightly version but not other crates (aster, quasi), as mentioned in #231

@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Mar 28, 2016

Member

This looks good to me once the CI failure is resolved.

Member

glennw commented Mar 28, 2016

This looks good to me once the CI failure is resolved.

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Mar 28, 2016

Member

It looks like it's an upstream dependency failure, so I've restarted the build to see if it passes now.

Member

jdm commented Mar 28, 2016

It looks like it's an upstream dependency failure, so I've restarted the build to see if it passes now.

@gmorenz

This comment has been minimized.

Show comment
Hide comment
@gmorenz

gmorenz Mar 28, 2016

Contributor

@jdm

If I'm reading travis correctly it's still failing on the pinned nightly version, but it's using the old configuration still, e.g. nightly-2016-03-07 where other recent builds use nightly-2016-03-18. The error is still the quasi and aster crates expecting a different nightly version.

Contributor

gmorenz commented Mar 28, 2016

@jdm

If I'm reading travis correctly it's still failing on the pinned nightly version, but it's using the old configuration still, e.g. nightly-2016-03-07 where other recent builds use nightly-2016-03-18. The error is still the quasi and aster crates expecting a different nightly version.

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Mar 29, 2016

Member

@bors-servo: r=glennw

Member

jdm commented Mar 29, 2016

@bors-servo: r=glennw

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Mar 29, 2016

Contributor

📌 Commit 2ee56a8 has been approved by glennw

Contributor

bors-servo commented Mar 29, 2016

📌 Commit 2ee56a8 has been approved by glennw

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Mar 29, 2016

Contributor

⌛️ Testing commit 2ee56a8 with merge 8508c49...

Contributor

bors-servo commented Mar 29, 2016

⌛️ Testing commit 2ee56a8 with merge 8508c49...

bors-servo added a commit that referenced this pull request Mar 29, 2016

Auto merge of #243 - gmorenz:underflow, r=glennw
Use signed arithmetic to avoid underflow when shrinking window.

fixes #236

The underflow occurs when the y co-ordinate of `viewport_size` in the last `render_api.set_root_stacking_context` call is bigger than the y co-ordinate of  `framebuffer_size` in a `renderer.render` call.

The problem is the calculated layer_origin y is negative, but we calculate it using unsigned ints, and then cast to signed just for the gl calls. The two places where this calculated value is used are `gl::scissor` and `gl::viewport`, both of these are perfectly happy with a negative origin point (as far as I know and can tell, but I don't have much experience with opengl), thus I've just changed it so we cast to signed integers early.

The other options would be to require the above doesn't happen, either by using an outdated bigger framebuffer size (which results in the contents of the window moving upwards as the window shrinks), or recalling `set_root_stacking_context` (which is less efficient).
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Mar 29, 2016

Contributor

☀️ Test successful - travis

Contributor

bors-servo commented Mar 29, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit 2ee56a8 into servo:master Mar 29, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment