Use get_relative_transform for clip <-> prim space conversions. #2977

gw3583 · 2018-08-20T09:48:32Z

This expands the functionality in get_relative_transform to work
for both primitive and clip space conversions.

This code path is not super efficient (it could be improved), but
on the majority of sites it never runs, and even on cases where it
does run, the coordinate system tree is typically very shallow. We
can optimize this function if it ever shows up in a profile.

This change is

gw3583 · 2018-08-20T09:49:37Z

r? @kvark

Try run here (looks good I think, just a few unrelated intermittents):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffd9a4eb3c27b46449058123fec949ff4d3cdea1

I changed the expected result for one of the wrench tests - it is an incorrect test that came from Servo, and the item should actually be clipped in this case.

kvark

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gw3583)

webrender/src/clip_scroll_tree.rs, line 146 at r1 (raw file):

        for node in nodes {
            let coord_system = &self.coord_systems[node.0 as usize];

I wonder if we can do it the other way, so that we don't have to collect/reverse nodes and everything is a simple pass? I.e. currently we collect nodes by ascending from child to parent, then reverse them, then accumulate the transforms on the way from parent to child. Can we instead accumulate it from child to parent in the first place?

webrender/src/clip_scroll_tree.rs, line 495 at r1 (raw file):

    let m = cst.get_relative_transform(from, to).unwrap();
    let pt = m.transform_point2d(&p).unwrap();
    assert!((pt.x - expected_x).abs() < 0.0001 &&

nit: can we use the euclid::approx_eq::ApproxEq::approx_epsilon here?

webrender/src/clip_scroll_tree.rs, line 595 at r1 (raw file):

#[test]
fn test_cst_scale_translation() {

wow nice set of tests! 👏

webrender/src/clip_scroll_tree.rs, line 625 at r1 (raw file):

    test_pt(100.0, 100.0, &cst, child1, root, 200.0, 150.0);
    test_pt(100.0, 100.0, &cst, child2, root, 300.0, 450.0);
    // test_pt(100.0, 100.0, &cst, root, child1, 0.0, 50.0);

what about those commented out ones?

wrench/reftests/transforms/reftest.list, line 13 at r1 (raw file):

== segments-bug.yaml segments-bug-ref.yaml
platform(linux,mac) == content-offset.yaml content-offset.png
platform(linux,mac) != coord-system.yaml coord-system.png

oh, shouldn't we just update the png instead to make it match? Equality test is more useful than the non-equality one


        Use get_relative_transform for clip <-> prim space conversions.

This expands the functionality in get_relative_transform to work for both primitive and clip space conversions. This code path is not super efficient (it could be improved), but on the majority of sites it never runs, and even on cases where it does run, the coordinate system tree is typically very shallow. We can optimize this function if it ever shows up in a profile.

gw3583

Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @kvark and @gw3583)

webrender/src/clip_scroll_tree.rs, line 146 at r1 (raw file):