Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upOmit nested clip regions if internal one is their subregion #299
+40
−6
Conversation
src/batch.rs
Outdated
| @@ -453,3 +460,30 @@ impl RasterBatch { | |||
| false | |||
| } | |||
| } | |||
|
|
|||
| trait InsideTest<T> { | |||
| fn contains_may_be_false_negative(&self, clip: &T) -> bool; | |||
This comment has been minimized.
This comment has been minimized.
pcwalton
Jun 29, 2016
Collaborator
I would just call this might_contain and clarify in the comments that this may be a false negative.
src/batch.rs
Outdated
|
|
||
| impl InsideTest<ComplexClipRegion> for ComplexClipRegion { | ||
| fn contains_may_be_false_negative(&self, clip: &ComplexClipRegion) -> bool { | ||
| print!("contains_may_be_false_negative: {:?} {:?}\n", self, clip); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/batch.rs
Outdated
| print!("contains_may_be_false_negative: {:?} {:?}\n", self, clip); | ||
| let delta_left = clip.rect.origin.x - self.rect.origin.x; | ||
| let delta_top = clip.rect.origin.y - self.rect.origin.y; | ||
| let delta_right = self.rect.origin.x + self.rect.size.width - (clip.rect.origin.x + clip.rect.size.width); |
This comment has been minimized.
This comment has been minimized.
pcwalton
Jun 29, 2016
Collaborator
Could use self.rect.max_x() and clip.rect.max_x() here for brevity
src/batch.rs
Outdated
| let delta_left = clip.rect.origin.x - self.rect.origin.x; | ||
| let delta_top = clip.rect.origin.y - self.rect.origin.y; | ||
| let delta_right = self.rect.origin.x + self.rect.size.width - (clip.rect.origin.x + clip.rect.size.width); | ||
| let delta_bottom = self.rect.origin.y + self.rect.size.height - (clip.rect.origin.y + clip.rect.size.height); |
This comment has been minimized.
This comment has been minimized.
pcwalton
Jun 29, 2016
Collaborator
Could use self.rect.max_y() and clip.rect.max_y() here for brevity
src/batch.rs
Outdated
| 0 => None, | ||
| n if n > 1 => { | ||
| let internal_clip = clip.last().unwrap(); | ||
| if clip[..clip.len()].iter().all(|current_clip| current_clip.contains_may_be_false_negative(internal_clip)) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/batch.rs
Outdated
| Some(clip[0]) | ||
| } | ||
| }, | ||
| _ => Some(clip[0]), |
This comment has been minimized.
This comment has been minimized.
pcwalton
Jun 29, 2016
Collaborator
Could put this one before the above match and say 1 => Some(clip[0]), then remove the guard
src/batch.rs
Outdated
| self.complex_clip = None; | ||
| self.complex_clip = match clip.len() { | ||
| 0 => None, | ||
| n if n > 1 => { |
This comment has been minimized.
This comment has been minimized.
glennw
Jun 29, 2016
Member
Probably clearer to add a condition for 1 => above this and remove the _ => condition.
src/batch.rs
Outdated
| @@ -453,3 +460,30 @@ impl RasterBatch { | |||
| false | |||
| } | |||
| } | |||
|
|
|||
| trait InsideTest<T> { | |||
This comment has been minimized.
This comment has been minimized.
splav
force-pushed the
splav:border-radius-clip#11857
branch
2 times, most recently
from
6cd0641
to
23b985b
|
|
|
I think this is still relevant but would need to be ported to the wr2 code - thoughts @splav ? |
|
@glennw, sure, I'll port it. |
|
@bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Aug 8, 2016
Omit nested clip regions if internal one is their subregion In case of nested clipping regions it is common that external ones do contain the most internal clipping region and essentially only that internal one should be used. I'm not sure that this is the correct path, but it's rather common ([servo/issues/11562](servo/servo#11562), [servo/issues/11857](servo/servo#11857)) and at least can be used as a fast path if the supposed full of nested clipping regions is rather heavy. This PR implements this fast path. It relies on the fact that int nested regions list the last is the innermost. While not sure that it's a good idea at all, looks like a hack, but still... If there are any thought hot to implement full nested regions support I'll try to implement those (Or may be it will be done in WR2 and there is no need to do this right now).
|
Thanks! |
|
@glennw not at all! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
splav commentedJun 29, 2016
In case of nested clipping regions it is common that external ones do contain the most internal clipping region and essentially only that internal one should be used.
I'm not sure that this is the correct path, but it's rather common (servo/issues/11562, servo/issues/11857) and at least can be used as a fast path if the supposed full of nested clipping regions is rather heavy.
This PR implements this fast path. It relies on the fact that int nested regions list the last is the innermost.
While not sure that it's a good idea at all, looks like a hack, but still...
If there are any thought hot to implement full nested regions support I'll try to implement those (Or may be it will be done in WR2 and there is no need to do this right now).