Use point/size in ClipBox instead of rect.#1776
Merged
jneem merged 1 commit intolinebender:masterfrom May 19, 2021
jneem:clipbox-size
Merged
Use point/size in ClipBox instead of rect.#1776jneem merged 1 commit intolinebender:masterfrom jneem:clipbox-size
jneem merged 1 commit intolinebender:masterfrom
jneem:clipbox-size
Conversation
This fixes a layout bug I was having, but I think it's also an indication of a bigger issue. The issue is that in various parts of the layout code, we expand dimensions to the nearest display point. Besides the fact that we should be using pixels intead of display points (IMO), this is numerically unstable: if because of some numerical stuff you have a dimension that's a rounding error larger than an integer number of pixels, the layout "jumps". What does this have to do with ClipBox? Well, ClipBox was storing its layout rect as a `Rect`, which is stored as the coordinates of opposite corners. When scrolling the ClipBox, the size should stay fixed but the origin should move. But moving the origin can *change the size* very slightly because of numerical errors converting to/from the Rect representation. These slight errors got magnified by the pixel expansion in other parts of the layout code. This PR fixes the numerical imprecision in the ClipBox code, but I suspect that the real fix is to eliminate the numerical instability.
maan2003
approved these changes
May 17, 2021
Collaborator
maan2003
left a comment
maan2003
left a comment
There was a problem hiding this comment.
Looks good! The .expand() is tricky to manage
maan2003
added
S-ready
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This fixes a layout bug I was having, but I think it's also an
indication of a bigger issue.
The issue is that in various parts of the layout code, we expand
dimensions to the nearest display point. Besides the fact that we should
be using pixels intead of display points (IMO), this is numerically
unstable: if because of some numerical stuff you have a dimension that's
a rounding error larger than an integer number of pixels, the layout
"jumps".
What does this have to do with ClipBox? Well, ClipBox was storing its
layout rect as a
Rect, which is stored as the coordinates of oppositecorners. When scrolling the ClipBox, the size should stay fixed but the
origin should move. But moving the origin can change the size very
slightly because of numerical errors converting to/from the Rect
representation. These slight errors got magnified by the pixel expansion
in other parts of the layout code.
This PR fixes the numerical imprecision in the ClipBox code, but I
suspect that the real fix is to eliminate the numerical instability.