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 upBackface support #1606
Backface support #1606
Conversation
|
I'll look at this in detail tomorrow (if @kvark doesn't get to it first!) but on a quick read this seems to change a lot more interface functions than I was expecting. |
|
The backface-visibility also effects on both transformable and non-transformable items. For transformable items (a.k.a stacking context), If backface-visibility is false and transform of stacking context is in backface, then we drop entire stacking context. For non-transformable items, if backface-visibility is false, we'll check its belonging stacking context is in backface or not. So that we need add backface visibility for every display item. I know there are quite many interface changes. But I don't find any other suitable way to do this. |
|
We shouldn't need that many interface changes then - we should be operate at the stacking context level and skip the whole stacking context in that case. Looking at the way we detect if a stacking context is not visible and skipping it is probably a good place to use as a reference, I think. |
|
I'll have a bit more of a think about how to best represent the per-element visibility flag. Perhaps we can store it in the primitive run, since it's so rarely used... |
|
|
|
I think the API changes are reasonable, given that any element can have the flag, not just SC. We just need to introduce a helper structure to pass down everywhere and hide such common elements, e.g. struct PrimitiveInfo {
rect: LayoutRect,
local_clip: Option<LocalClip>,
is_backface_visible: bool,
} |
| @@ -540,16 +548,21 @@ impl DisplayListBuilder { | |||
| debug_assert_eq!(len, count); | |||
| } | |||
|
|
|||
| pub fn push_rect(&mut self, rect: LayoutRect, local_clip: Option<LocalClip>, color: ColorF) { | |||
| pub fn push_rect(&mut self, | |||
This comment has been minimized.
This comment has been minimized.
|
Would this be better with a push/pop_back_face_visibilty or set_default_back_face_visibility item? That avoids having to encode it in every item. |
|
Morris, do you have a testcase where backface-visibility is applied (and has an effect on) a non-transformable item? The spec seems to say "Applies to: transformable elements", so I'd have thought that specifying it on stacking contexts would be enough. |
|
Spec says the transformable elements are "an element whose layout is governed by the CSS box model". So I think the transformable elements are not only indicate stacking context but also indicate all elements that apply box model. From gecko's implementation perspective, we can query IsBackfaceHidden from all kind of display items. So I think backface visibility apply to almost every elements. |
|
I can only see us call BackfaceIsHidden() from the nsDisplayTransform display item, not from any random display item. I've tried to create a testcase where Can you create a testcase that proves me wrong? I agree with you now that the spec doesn't really call this out explicitly. Maybe @mattwoodrow knows more details. |
|
For items that create their own layers we call nsDisplayItem::BackfaceIsHidden (which calls nsIFrame::BackfaceIsHidden). For items that go into a PaintedLayer we call nsIFrame::In3DContextAndBackfaceIsHidden and use the result to make sure we split PaintedLayers when this changes. I think for a testcase, you can create a preserve-3d scene where a node has multiple untransformed children, and backface-visibility: hidden is set on one of them. The spec is definitely not very clear here (and having backface-visibility: hidden apply to untransformed elements is a pain), but iirc thinker implemented it this way to match WebKit. |
|
Testcase for individual item. https://jsfiddle.net/r1Luxv0y/ |
|
I also dumped display list and layer in gecko for above test case. I also turned on force active layer to prevent div merged to painted layer. As you can see the layer tree, we have 3 colors layer with backface hidden and others with backface visible. |
|
@mephisto41 I think the suggestion from @jrmuizel
might work well. That way we don't have to change many interfaces at all, and we only pay the storage cost when that flag changes value. It could be encoded efficiently within the primitive runs. Does that sound like it would work? |
|
@glennw I think this might be good idea. I'll implement it. Thanks for the suggestion. |
|
There's some further discussion about what the semantics of backface-visiblity in https://bugzilla.mozilla.org/show_bug.cgi?id=1393554 |
|
@kvark Maybe you're right, having thought about it a bit over the weekend. @mephisto41 Sorry for all the back and forth - perhaps having it in the API per-primitive, but as a |
|
Sure, I'll implement new patch as @kvark suggested! Thanks. |
|
@kvark Some questions when adding
|
|
@mephisto41 those are good questions:
Hmm, it comes down to a question - to what extend would we want the statefull-ness of the API to be preserved. I'd say, let's leave the
Unless someone tells us that one of those types is on the way out, I'd prefer us keeping the distinction. The comment says:
In order to include struct PrimitiveInfo<P> {
rect: TypedRect<f32, P>,
...
}
type LayoutPrimitiveInfo = PrimitiveInfo<LayoutPixel>; |
|
r? @kvark |
|
Thank you! It's shaping up nicely :) |
| @@ -1559,6 +1563,7 @@ impl StackingContext { | |||
| isolation, | |||
| is_page_root, | |||
| is_visible: false, | |||
| is_backface_visible: is_backface_visible, | |||
This comment has been minimized.
This comment has been minimized.
kvark
Sep 7, 2017
Member
nit: the greatness of rust allows you to write just is_backface_visible, now, DRY
| } | ||
|
|
||
| pub fn local_clip(&self) -> &LocalClip { | ||
| &self.iter.cur_item.local_clip | ||
| pub fn get_layer_primitive_info(&self, offset: &LayoutVector2D) -> LayerPrimitiveInfo { |
This comment has been minimized.
This comment has been minimized.
| "line" => self.handle_line(dl, item, local_clip), | ||
| "image" => self.handle_image(dl, wrench, item, local_clip), | ||
| "text" | "glyphs" => self.handle_text(dl, wrench, item, local_clip), | ||
| "rect" => self.handle_rect(dl, item, local_clip, backface_visible), |
This comment has been minimized.
This comment has been minimized.
kvark
Sep 7, 2017
Member
we could probably make all of those handlers to receive LayoutPrimitiveInfo with zeroed out rectangle, for simplicity
| } | ||
|
|
||
| pub fn local_clip_with_offset(&self, offset: &LayoutVector2D) -> LocalClip { | ||
| self.iter.cur_item.local_clip.create_with_offset(offset) | ||
| pub fn local_clip(&self) -> Option<LocalClip> { |
This comment has been minimized.
This comment has been minimized.
kvark
Sep 7, 2017
Member
In all but one case this function result is unwrapped right away, and it used to return LocalClip directly. Would be simpler to keep it this way and change less code, I think.
|
|
mephisto41
force-pushed the
mephisto41:backface-support
branch
4 times, most recently
from
f00f11f
to
59bf291
|
|
|
@kvark @mephisto41 Is this one ready to go, or are there still a couple of comments to be addressed? |
|
@mephisto41 I'm happy :) thank you! |
|
|
Backface support This solves #1419 . <!-- 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/1606) <!-- Reviewable:end -->
|
|
mephisto41 commentedAug 24, 2017
•
edited by larsbergstrom
This solves #1419 .
This change is