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 upBump euclid dependency to 0.14.1. #1
Conversation
|
Now that euclid 0.13 is out, can this be merged ? I'm updating servo to euclid 0.13 and this is needed ^^ Edit: this PR is wrong, you took euclid master and not euclid 0.13 to test it (I think) |
Actually I meant to say 0.14 but got mixed up. This PR updates plane-split after the massive point/vector refactoring in euclid (which isn't published on crates.io yet). |
|
Ok then is it ok if I open a PR for the actual 0.13 ? :p |
nical
changed the title
|
Yep, no problem! |
nical
changed the title
|
r? @kvark |
| @@ -10,6 +10,6 @@ documentation = "https://docs.rs/plane-split" | |||
|
|
|||
| [dependencies] | |||
| binary-space-partition = "0.1.2" | |||
| euclid = "0.13" | |||
| euclid = "~0.14.1" | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nical
Jun 1, 2017
Author
Contributor
my understanding is that ~0.14.1 means "between 0.14.1 and 0.15 not included" whereas 0.14.1 means "specifically version 0.14.1".
This comment has been minimized.
This comment has been minimized.
Eijebong
Jun 1, 2017
Member
0.14.1 is the same thing as ^0.14.1
http://doc.crates.io/specifying-dependencies.html#specifying-dependencies-from-cratesio
This comment has been minimized.
This comment has been minimized.
| @@ -160,7 +160,8 @@ impl< | |||
| let comp_y = intersect_across(a, b, axis_y); | |||
| // line that tries to intersect both | |||
| let line = Line { | |||
| origin: comp_x + comp_y, | |||
| // TODO(review): I don't understand the meaning of origin here | |||
This comment has been minimized.
This comment has been minimized.
kvark
Jun 1, 2017
Member
so, comp_x is a point on the axis_x axis that is supposed to be between the polygons, and comp_y is the same for axis_y. Adding them is like adding X and Y projections of a vector.
What the code needs is a line that intersects both polygons, simple as that. Taking origin = comp_x + comp_y is not entirely correct here, it's a rough approximation (hence, the TODO at the top).
|
Woops, I'll convert the tests tomorrow. |
|
Thanks! |
nical commentedMay 28, 2017
•
edited
This is a very hipster type of pull request because euclid 0.14 isn't published yet on crates.io. But getting all of servo's use of euclid converted to 0.14 is going to be acrobatic, so I am getting started.
Since euclid 0.14 isn't published yet, you need to add
to Cargo.toml until 0.14 finds its way to crates.io.
No need to land this before that happens but I need to at least have this fixed up locally to start fixing webrender so I had to get started asap.
Since conversion to 0.14 is a lot of work (and a lot of crates), I only converted point to vectors where it seemed to make sense. There's probably some other more stylistic that could make sense (like using
vec3and other helpers), but I propose that we postpone that to followups.Note that I added a comment somewhere in a place where we add two points to compute the origin of a line and I am not sure what this means, so this bit should be reviewed with special care.
Edit: got mixed up between 0.13 and 0.14