Use strongly typed geometry - Part 2 #602

nical · 2016-11-28T10:06:38Z

Use strongly typed geometry in the public API. This changeset adds the LayoutPixel unit which is basically an alias to LayerPixel.
I think that it's best to keep the "Layer" terminology internal to webrender, and since the 1-1 correspondence between the api's layout pixels and internal layer pixels is somewhat coincidental, it will help to have separate names if things like async zoom introduce an actual difference between the two coordinate spaces (as it does in Gecko). Using an alias instead of a separate type comes from a mix of laziness (not having to cast from layout t layer pixels all over frame.rs) and the fact that currently layer and layout pixels are the same thing, but I'll add a separate unit if there is a preference for it.

I did not introduce ParentLayoutPixel. I don't know the API well enough yet to be sure whether some geometry is passed in a stacking context's parent coordinate space, but if so we should consider introducing a special unit for it, if only for the sake of proper documentation.

This PR is a lot easier to rebase than part 1 and is a breaking change to the public API, so it's fine to wait a bit if there are cross-crates changes that we want to coordinate before having to adapt servo to this (although it should be easy to do).

This change is

kvark

Looks great in general, got a few things to address

kvark · 2016-11-28T18:20:47Z

replay/src/main.rs

 use gleam::gl;
 use std::io::Read;
 use std::fs::File;
 use std::path::{Path, PathBuf};
 use std::env;
-use webrender_traits::{RenderApi, PipelineId};
+use webrender_traits::{RenderApi, PipelineId, LayerSize, DeviceUintSize};


Having DeviceIntRect, DeviceUintRect, and just DeviceRect now, the first thing that comes to mind is that the latter is inconsistent. Would be more consistent if it was DeviceFloatRect. But really, it would be much simpler to just have DeviceRect<i32>, DeviceRect<u32>, and so forth - we'd still get the safe units but with somewhat clearer semantics.

On the other hand this is the naming convention used for all geometry in Gecko which is a different kind of consistency but an appreciable one if gecko and servo are to share more code in the future.
The rationale is that we tend to manipulate float-based geometry more often so they get the short name. Typically there is no need for, say, a LayoutIntPoint (and we probably don't really need DeviceUintPoint as the choice between DeviceIntPoint and DeviceUintPoint is a bit inconsistent right now. In gecko we just used signed integers and floats for geometries).
I'm not found of typing "<>" brackets all over the place but I admit that's hardly an argument.

Let's open a separate issue about this and change it if enough people feel strongly one way or another.

kvark · 2016-11-28T18:42:39Z

webrender/src/frame.rs

@@ -331,8 +331,8 @@ impl Frame {

    /// Returns true if any layers actually changed position or false otherwise.
    pub fn scroll(&mut self,
-                  mut delta: Point2D<f32>,
-                  cursor: Point2D<f32>,
+                  mut delta: LayerPoint,


I believe this should really be LayerSize

I have proposed the addition of vector classes in euclid (separate from points which are currently used as vectors in a lot of places when size types are not used instead). The pull request is open and hopefully we can bikeshed and maybe merge it by the end of the workweek.
Let's wait a little bit before fixing the insistent point-vs-size-for-vectors situation.

kvark · 2016-11-28T18:42:52Z

webrender/src/frame.rs

@@ -472,9 +472,9 @@ impl Frame {
        // Insert global position: fixed elements layer
        debug_assert!(self.layers.is_empty());
        let root_fixed_layer_id = ScrollLayerId::create_fixed(root_pipeline_id);
-        let root_viewport = LayerRect::new(LayerPoint::zero(), LayerSize::from_untyped(&root_pipeline.viewport_size));
+        let root_viewport = LayerRect::new(LayerPoint::zero(), root_pipeline.viewport_size);


kvark · 2016-11-28T18:44:17Z

webrender/src/internal_types.rs

            }
        }
    }

-    pub fn resize(&mut self, size: &Size2D<i32>) -> Result<(), &'static str> {
+    pub fn resize(&mut self, size: &DeviceIntSize) -> Result<(), &'static str> {


it rubs me the wrong way to see Int there, but I can see that it was this way before you

kvark · 2016-11-28T18:46:24Z

webrender_traits/src/api.rs

@@ -207,8 +207,8 @@ impl RenderApi {
    }

    /// Translates a point from viewport coordinates to layer space
-    pub fn translate_point_to_layer_space(&self, point: &Point2D<f32>)
-                                          -> (Point2D<f32>, PipelineId) {
+    pub fn translate_point_to_layer_space(&self, point: &LayoutPoint)


hmm, this one makes me worried. The function is supposed to translate a point from one space to another, yet our type system does not reflect that here, using LayoutPoint on both ends

Good catch, I think that it should take a WorldPoint and return a LayoutPoint. I made the change locally, that said the implementation is panic!("unused api - remove from webrender_traits"); so I guess it won't stick around for long.

bors-servo · 2016-11-28T21:37:31Z

☔ The latest upstream changes (presumably #595) made this pull request unmergeable. Please resolve the merge conflicts.

glennw · 2016-11-28T23:35:47Z

webrender/src/frame.rs

@@ -236,7 +236,7 @@ impl Frame {
        old_layer_scrolling_states
    }

-    pub fn get_scroll_layer(&self, cursor: &Point2D<f32>, scroll_layer_id: ScrollLayerId)
+    pub fn get_scroll_layer(&self, cursor: &LayerPoint, scroll_layer_id: ScrollLayerId)


cursor is in world space.

bors-servo · 2016-11-29T19:44:59Z

☔ The latest upstream changes (presumably #585) made this pull request unmergeable. Please resolve the merge conflicts.

glennw · 2016-11-30T06:47:20Z

Although there's a couple of rough edges in places, this seems like a net improvement - let's get it rebased and then I'll merge it.

glennw · 2016-11-30T20:18:14Z

@nical CI is failing - I guess wrench / sample / replay may need to be updated.

nical · 2016-11-30T21:49:46Z

Ah, wrench landed \o/, I'm rebasing it now.

glennw · 2016-12-01T00:18:59Z

@bors-servo r+

bors-servo · 2016-12-01T00:19:00Z

📌 Commit 70caab6 has been approved by glennw

bors-servo · 2016-12-01T03:08:44Z

🔒 Merge conflict

bors-servo · 2016-12-01T03:09:07Z

☔ The latest upstream changes (presumably #611) made this pull request unmergeable. Please resolve the merge conflicts.

glennw · 2016-12-01T20:44:18Z

@bors-servo r+

bors-servo · 2016-12-01T20:44:19Z

📌 Commit 28bc050 has been approved by glennw

bors-servo · 2016-12-01T20:44:21Z

⚡ Test exempted - status

Use strongly typed geometry - Part 2 Use strongly typed geometry in the public API. This changeset adds the LayoutPixel unit which is basically an alias to LayerPixel. I think that it's best to keep the "Layer" terminology internal to webrender, and since the 1-1 correspondence between the api's layout pixels and internal layer pixels is somewhat coincidental, it will help to have separate names if things like async zoom introduce an actual difference between the two coordinate spaces (as it does in Gecko). Using an alias instead of a separate type comes from a mix of laziness (not having to cast from layout t layer pixels all over frame.rs) and the fact that currently layer and layout pixels are the same thing, but I'll add a separate unit if there is a preference for it. I did not introduce ParentLayoutPixel. I don't know the API well enough yet to be sure whether some geometry is passed in a stacking context's parent coordinate space, but if so we should consider introducing a special unit for it, if only for the sake of proper documentation. This PR is a lot easier to rebase than part 1 and is a breaking change to the public API, so it's fine to wait a bit if there are cross-crates changes that we want to coordinate before having to adapt servo to this (although it should be easy to do).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/602)

kvark suggested changes Nov 28, 2016

View reviewed changes

glennw reviewed Nov 28, 2016

View reviewed changes

nical force-pushed the more-units-api branch from ddd39f9 to 3c613a9 Compare November 29, 2016 14:52

nical force-pushed the more-units-api branch from 3c613a9 to b5bd8ff Compare November 30, 2016 19:39

nical and others added 2 commits December 1, 2016 10:17

Add strongly typed units to the public API.

c87b2d5

Use strongly typed units in wrench.

28bc050

nical force-pushed the more-units-api branch from 70caab6 to 28bc050 Compare December 1, 2016 09:26

bors-servo merged commit 28bc050 into servo:master Dec 1, 2016

nical deleted the more-units-api branch December 5, 2016 04:37

Use strongly typed geometry - Part 2 #602

Use strongly typed geometry - Part 2 #602

Uh oh!

Conversation

nical commented Nov 28, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Nov 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Nov 29, 2016

Uh oh!

glennw commented Nov 30, 2016

Uh oh!

glennw commented Nov 30, 2016

Uh oh!

nical commented Nov 30, 2016

Uh oh!

glennw commented Dec 1, 2016

Uh oh!

bors-servo commented Dec 1, 2016

Uh oh!

bors-servo commented Dec 1, 2016

Uh oh!

bors-servo commented Dec 1, 2016

Uh oh!

glennw commented Dec 1, 2016

Uh oh!

bors-servo commented Dec 1, 2016

Uh oh!

bors-servo commented Dec 1, 2016

Uh oh!

Uh oh!

nical commented Nov 28, 2016 •

edited by larsbergstrom

Loading