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:51:45Z

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:51:45Z

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:51:45Z

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:51:45Z

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:51:45Z

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:37:50Z

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.


        Add strongly typed units to the public API.


        Use strongly typed units in wrench.

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


        Auto merge of #602 - nical:more-units-api, r=glennw

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 requested changes Nov 28, 2016

View changes

glennw reviewed Nov 28, 2016

View changes

nical force-pushed the nical:more-units-api branch from ddd39f9 to 3c613a9 Nov 29, 2016

nical force-pushed the nical:more-units-api branch from 3c613a9 to b5bd8ff Nov 30, 2016

nical added 2 commits Nov 22, 2016

Add strongly typed units to the public API.

c87b2d5

Use strongly typed units in wrench.

Loading status checks…

28bc050

nical force-pushed the nical:more-units-api branch from 70caab6 to 28bc050 Dec 1, 2016

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test exempted
Details

nical deleted the nical:more-units-api branch Dec 5, 2016

servo / webrender

Use strongly typed geometry - Part 2 #602

Use strongly typed geometry - Part 2 #602

nical commented Nov 28, 2016 •

edited by larsbergstrom

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 28, 2016

This comment has been minimized.

bors-servo commented Nov 29, 2016

glennw commented Nov 30, 2016

glennw commented Nov 30, 2016

nical commented Nov 30, 2016

glennw commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

glennw commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

servo / webrender

Join GitHub today

Use strongly typed geometry - Part 2 #602

Use strongly typed geometry - Part 2 #602

Conversation

nical commented Nov 28, 2016 • edited by larsbergstrom

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 28, 2016

This comment has been minimized.

bors-servo commented Nov 29, 2016

glennw commented Nov 30, 2016

glennw commented Nov 30, 2016

nical commented Nov 30, 2016

glennw commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

glennw commented Dec 1, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

nical commented Nov 28, 2016 •

edited by larsbergstrom