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 upAdd tiling for gradients #1078
Add tiling for gradients #1078
Conversation
This commit adds `tile_size` and `tile_spacing` fields to linear and radial gradients. Tiles start at `rect.origin` and are `tile_size` with `tile_spacing` between each tile. Tiling is implemented inside the gradient shaders, not by decomposing the display item into multiple primitives. This has the advantage of being simple to implement, and in the best case where there is no spacing it should be just as fast as transforming the gradient into a repeating gradient. This has the disadvantage that large tile spacing can cause many dropped fragments. There may be good reason to investigate decomposing to multiple primitives in these cases.
|
The test coverage is really impressive, thank you! |
|
|
Add tiling for gradients This adds `tile_size` and `tile_spacing` fields to linear and radial gradient display items to control tiling. A gradient is tiled starting at `bounds.origin`, with tile size of `tile_size` and spacing between tiles of `tile_spacing`. Tiles will fill and be clipped to `bounds`. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1078) <!-- Reviewable:end -->
|
|
|
When testing #1087 on the latest, I tried hooking up to servo and made the following change: - builder.push_gradient(rect, clip, gradient);
+ builder.push_gradient(rect, clip, gradient, rect.size, webrender_traits::LayoutSize::zero());And got 2 WPT failures:
Could this PR be causing them? |
|
The log looks like the failures I was seeing when testing #1074. Can you give it a try with the following diff? It makes gradient start stop relative like the #1074 expects. diff --git a/components/layout/display_list_builder.rs b/components/layout/display_list_builder.rs
index 8c390487f5..091df7e9c8 100644
--- a/components/layout/display_list_builder.rs
+++ b/components/layout/display_list_builder.rs
@@ -952,8 +952,8 @@ impl FragmentDisplayListBuilding for Fragment {
})
}
- let center = Point2D::new(absolute_bounds.origin.x + absolute_bounds.size.width / 2,
- absolute_bounds.origin.y + absolute_bounds.size.height / 2);
+ let center = Point2D::new(absolute_bounds.size.width / 2,
+ absolute_bounds.size.height / 2); |
|
Ah, right, the change makes sense. Testing now. |
|
Hmm, still failing in |
|
I got the same thing with the patch, but looking at the reftest comparison they are a fuzzy match with 16 or so pixels (IIRC). Is that what you're getting too? |
|
@rlhunt indeed, there is about 40 pixels along a horisontal line that show a 1-value difference (33/34) with the reference. Interesting to see why, but not a showstopper. |
|
So anything we land in WR has to result in exact matches for the WPT/CSS tests (which we've managed to do OK so far). |
|
@glennw Okay, I'd be happy to investigate the fuzziness further. I'll take a look at it next chance I get. I'm not sure the best way to debug something like this, so I'm open to suggestions. I might take a look at renderdoc. |
eqrion commentedApr 6, 2017
•
edited by larsbergstrom
This adds
tile_sizeandtile_spacingfields to linear and radial gradientdisplay items to control tiling. A gradient is tiled starting at
bounds.origin,with tile size of
tile_sizeand spacing between tiles oftile_spacing. Tileswill fill and be clipped to
bounds.This change is