Closed Bug 1390039 Opened 7 years ago Closed 7 years ago

stylo: Implement compute_distance for mismatched transform lists

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1390039 - Add tests for mismatched transform lists and some other corner cases.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Servo PR, #18234
41 bytes, text/x-github-pull-request
Details | Review 
Bug 1362896 implemented the compute_distance for matched transform lists. However, we haven't implemented mismatched transform lists because this might need the layout info (i.e. transform box) to resolve the calc value of translate.

In Gecko, we use GeckStyleContext to get the layout info to resolve the translate function; however, if possible, we should avoid using layout info to compute the distance. I'm not sure which way is better, but if we need layout info, we probably need to find a way to retrieve ServoStyleContext/ComputedValue on Servo side during computing the distance.
Boris, does this block shipping?
Flags: needinfo?(boris.chiou)
I'm not sure about this because right only DevTool needs this. What do you think about the priority of this bug, Brian?
Flags: needinfo?(boris.chiou) → needinfo?(bbirtles)
No. It can make the graphs in the Animation Inspector DevTools be inaccurate/unhelpful for a somewhat common case ('transform' is the 2nd most popular property to animate, but mismatched transform lists are less common) so it would be nice to fix, but we can ship without it.
Flags: needinfo?(bbirtles)
Assignee: nobody → boris.chiou
Comment on attachment 8900700 [details]
Bug 1390039 - Part 1: Implement to_transform_3d_matrix.

https://reviewboard.mozilla.org/r/172022/#review177394

I'm not sure where should I put TransformList::to_transform_3d_matrix(), so I will r?nox in the PR later.
Comment on attachment 8900700 [details]
Bug 1390039 - Part 1: Implement to_transform_3d_matrix.

https://reviewboard.mozilla.org/r/172022/#review177694

::: servo/components/layout/fragment.rs:2880
(Diff revision 1)
> -        let operations = match self.style.get_box().transform.0 {
> -            None => return None,
> -            Some(ref operations) => operations,
> +        let transform = match self.style
> +                                  .get_box()
> +                                  .transform
> +                                  .to_transform_3d_matrix(Some(stacking_relative_border_box)) {

Nit: I'm not sure if the . here are supposed to line up vertically, but they don't.

::: servo/components/layout/fragment.rs:2884
(Diff revision 1)
> +            Some(transform) => transform,
> +            None => return None

(Nit: Do we do trailing comma style these days? i.e. comma after None?)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2395
(Diff revision 1)
> +        for it in list1.iter().zip_longest(list2) {
> +            squared_dist = squared_dist + match it {
>                  EitherOrBoth::Both(this, other) => {
> -                    this.compute_squared_distance(other)
> +                    match this.compute_squared_distance(other) {
> +                        Ok(dist) => dist,
> +                        // If one of the transform function pairs is failed (i.e. mismatched
> +                        // transform functions or something wrong), we have to convert the
> +                        // two lists into transform matrices and try to compute their distance.
> +                        // FIXME: If possible, I think it's better to distinguish mismatched
> +                        // functions from other errors.
> +                        _ => {
> +                            all_matched = false;
> +                            break;
> +                        }
> +                    }
>                  },
>                  EitherOrBoth::Left(list) | EitherOrBoth::Right(list) => {
> -                    list.to_animated_zero()?.compute_squared_distance(list)
> +                    list.to_animated_zero()?.compute_squared_distance(list)?
>                  },
>              }
> -        }).sum()
> -    }
> +        }

I suspect you could have done this a little more neatly using collect()? and sum() and so on but this is probably fine too.

::: servo/components/style/values/computed/transform.rs:68
(Diff revision 1)
> +    /// Return the normalized direction vector and its angle.
> +    // A direction vector that cannot be normalized, such as [0,0,0], will cause the
> +    // rotation to not be applied. i.e. Use an identity matrix or rotate3d(0, 0, 1, 0).

I know you're just moving code here, but I guess we could use /// for all lines here and make this note part of the documentation?

::: servo/components/style/values/computed/transform.rs:132
(Diff revision 1)
> +                    // A direction vector that cannot be normalized, such as [0, 0, 0], will cause
> +                    // the rotation to not be applied, so we use identity matrix in this case.
> +                    let theta = Angle::from_radians(2.0f32 * f32::consts::PI - theta.radians());
> +                    let (ax, ay, az, theta) =
> +                        DirectionVector::get_normalized_vector_and_angle(ax, ay, az, theta);
> +                    Transform3D::create_rotation(ax, ay, az, Radians::new(theta.radians()))

(It feels like Angle should be convertible into radians? Should we add a `From<Angle> for Radians` at some point?)

::: servo/components/style/values/computed/transform.rs:135
(Diff revision 1)
> +                    let d = d.to_f32_px();
> +                    if d > 0. {
> +                        Transform3D::create_perspective(d)
> +                    } else {
> +                        Transform3D::identity()
> +                    }

Any reason we dropped create_perspective_matrix here?
Attachment #8900700 - Flags: review?(bbirtles) → review+
Comment on attachment 8900701 [details]
Bug 1390039 - Part 2: Use DirectionVector as an alias of euclid::Vector3D<f32>.

https://reviewboard.mozilla.org/r/172024/#review177698

::: servo/components/style/values/computed/transform.rs:24
(Diff revision 1)
> +/// A vector to represent the direction vector (rotate axis) for Rotate3D.
> +pub type DirectionVector = Vector3D<CSSFloat>;

Any reason we prefer CSSFloat to f64?

Are we sure this drop in precision is ok?
Attachment #8900701 - Flags: review?(bbirtles) → review+
Comment on attachment 8900702 [details]
Bug 1390039 - Add tests for mismatched transform lists and some other corner cases.

https://reviewboard.mozilla.org/r/172026/#review177700

I haven't checked the math in these added tests but I trust you have and that this test passes on the Gecko style system so that even if something's wrong, it's consistently wrong :)

::: dom/animation/test/mozilla/test_distance_of_transform.html:322
(Diff revision 1)
> +                         'translate(50px) rotate(90deg) scale(5) skew(1rad)');
> +  assert_equals(dist,
> +                Math.sqrt(50 * 50 + Math.PI / 2 * Math.PI / 2 +
> +                          4 * 4 * 2 + 1 * 1),
> +                'distance of transform lists');
> +}, 'Test distance of transform lists but different length');

Test distance of transform lists where one has extra items

::: dom/animation/test/mozilla/test_distance_of_transform.html:329
(Diff revision 1)
> +  assert_equals(dist, Math.sqrt(Math.PI * Math.PI + 1.5 * 1.5 + 0.5 * 0.5),
> +    'distance of transform lists');

(Nit: not sure if you meant to indent the message here like we do in the other tests)
Attachment #8900702 - Flags: review?(bbirtles) → review+
Comment on attachment 8900700 [details]
Bug 1390039 - Part 1: Implement to_transform_3d_matrix.

https://reviewboard.mozilla.org/r/172022/#review177694

> Nit: I'm not sure if the . here are supposed to line up vertically, but they don't.

Actually, I am also confused about how line up this. Maybe it's better to declare another variable to avoid this. Will check the line up way other people used.

> (Nit: Do we do trailing comma style these days? i.e. comma after None?)

Oh. it seems nox alway adds comma, but sometime I saw there is no comma. hmm. ok, let's add a comma there.

> I suspect you could have done this a little more neatly using collect()? and sum() and so on but this is probably fine too.

OK, I will check if I can use collect here. If I don't have any idea, I believe nox will give me a better way if any. Thanks.

> (It feels like Angle should be convertible into radians? Should we add a `From<Angle> for Radians` at some point?)

Oh. I didn't notice that. Thanks.

> Any reason we dropped create_perspective_matrix here?

create_perspective_matrix is in layout module, so I just want to not move so many code and create too many dependenceis between them. (Oh maybe I think too much.) It's fine to move this function from layout/fragment.rs to here.
Comment on attachment 8900701 [details]
Bug 1390039 - Part 2: Use DirectionVector as an alias of euclid::Vector3D<f32>.

https://reviewboard.mozilla.org/r/172024/#review177698

> Any reason we prefer CSSFloat to f64?
> 
> Are we sure this drop in precision is ok?

I seem Gecko use Point3D as the direction vector, and point3D is gfx::Float (i.e. float), so it should be fine here for now. BTW, I checked test cases I added, and it seems ok.
Comment on attachment 8900702 [details]
Bug 1390039 - Add tests for mismatched transform lists and some other corner cases.

https://reviewboard.mozilla.org/r/172026/#review177700

> I haven't checked the math in these added tests but I trust you have and that this test passes
> on the Gecko style system so that even if something's wrong, it's consistently wrong :)

Thanks for the review. I tested this on both Gecko and Servo style systems, and I try my best to add tests which can reflect the consistency on both style systems. (Only some tiny precision differences, so it seems I have to use asseret_approx_equal for some cases, according to the try.)
Attachment #8900700 - Attachment is obsolete: true
Attachment #8900701 - Attachment is obsolete: true
Attached file Servo PR, #18234 — 

    
    

    
      
  
Priority: -- → P2

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c98f36b7578
Add tests for mismatched transform lists and some other corner cases. r=birtles

    
    

    
  
https://hg.mozilla.org/integration/autoland/rev/10bd7a6f14b8ea02ad024e47ed207edb53655553

Note that the VCS sync service was hung for a bit, so the code landed a few revisions after the test, so we expect the test to be orange until then.

    
    

    
  
OH, I thought my servo patch is landed first. Anyway, it looks good now on autoland. Thanks for the reminder.

    
    

    
  
(In reply to Boris Chiou [:boris] from comment #19)
> OH, I thought my servo patch is landed first.

Yeah, it landed on servo, but didn't get synced over to autoland until later when the VCS service got unstuck.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3c98f36b7578
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: