Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 3D cuboid implementation #311

Merged
merged 1 commit into from Mar 4, 2019
Merged

Add 3D cuboid implementation #311

merged 1 commit into from Mar 4, 2019

Conversation

@marcosatti
Copy link
Contributor

marcosatti commented Dec 9, 2018

Extends the library to add support for the 3D primitive Cuboid. Not sure if you intend this library to be mostly 2D only, but there is already some 3D support, so this is just a little extra - cuboids are pretty useful. Couldn't call this Box as it would conflict with rust's Box.

I plan to use this for my own project later on.

Tests are pending - I would like your feedback first (ok PR or not), then go from there.

For the z-axis equivalent of (horizontal, vertical), I have called it applicate. This is what googling around suggested.


This change is Reviewable

@nical
Copy link
Collaborator

nical commented Dec 12, 2018

Thanks for this PR!

We plan to add a TypedBox2D type using endpoints representation (top-left + lower-right) instead of origin + size because the former is more efficient for the majority of operations we use in WebRender.
I think that it makes sense to use the endpoints representation for the 3D version as well (can use a and b as member names) and also rename this new type into TypedBox3D

The rest of the changes look good to me (preferably with the commits squashed once we are good to go).

src/size.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
@marcosatti marcosatti force-pushed the marcosatti:master branch 2 times, most recently from f1f356a to 4b38c06 Jan 13, 2019
@marcosatti
Copy link
Contributor Author

marcosatti commented Jan 13, 2019

I think this is ready to be merged.

Copy link
Collaborator

nical left a comment

Thanks a lot for addressing the box representation. I apologize for requesting yet another round of changes.

I filed #317 to discuss a way to more easily add opinionated names without making euclid incompatible with a lot of projects, not sure how popular the idea will be but I'd rather be conservative about names in the mean time.

src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
src/side_offsets.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

nical left a comment

Sorry I didn't see the new commits arriving in this PR. I started writing a 2D equivalent by pretty much porting this over to 2D and spotted a couple of mistakes in the process.

It'd be great if you could extract the side-offsets-related changes into a separate PR and squash the commits in this one. It looks almost ready to go.

src/box3d.rs Outdated Show resolved Hide resolved
src/box3d.rs Outdated Show resolved Hide resolved
@marcosatti
Copy link
Contributor Author

marcosatti commented Feb 10, 2019

Sorry been a bit busy lately, I'll try getting on to it this week.

@nical
Copy link
Collaborator

nical commented Feb 10, 2019

Sorry been a bit busy lately, I'll try getting on to it this week.

Don't be! Take your time and thanks for putting up with me coming up with requests to change things a gazillion times.

@marcosatti marcosatti force-pushed the marcosatti:master branch from dcab58b to 96a4b5e Mar 4, 2019
@marcosatti marcosatti force-pushed the marcosatti:master branch from 96a4b5e to 29caaee Mar 4, 2019
@marcosatti
Copy link
Contributor Author

marcosatti commented Mar 4, 2019

Guess this is ready... I'll make a new PR for the side offsets.

@nical
Copy link
Collaborator

nical commented Mar 4, 2019

Thanks! @bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Mar 4, 2019

📌 Commit 29caaee has been approved by nical

@bors-servo
Copy link
Contributor

bors-servo commented Mar 4, 2019

Testing commit 29caaee with merge 57a8e96...

bors-servo added a commit that referenced this pull request Mar 4, 2019
Add 3D cuboid implementation

Extends the library to add support for the 3D primitive `Cuboid`. Not sure if you intend this library to be mostly 2D only, but there is already some 3D support, so this is just a little extra - cuboids are pretty useful. Couldn't call this Box as it would conflict with rust's Box.

I plan to use this for my own project later on.

Tests are pending - I would like your feedback first (ok PR or not), then go from there.

For the z-axis equivalent of (horizontal, vertical), I have called it applicate. This is what googling around suggested.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/311)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 4, 2019

☀️ Test successful - checks-travis
Approved by: nical
Pushing 57a8e96 to master...

@bors-servo bors-servo merged commit 29caaee into servo:master Mar 4, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@nical nical mentioned this pull request Mar 5, 2019
bors-servo added a commit that referenced this pull request Mar 6, 2019
Add TypedBox2D.

This is the 2D equivalent of `TypedBox3D` introduced in #311.

`TypedBox2D`'s endpoint representation is better suited for performing inclusion and intersection tests than `TypedRect` among other things.

`TypedRect`'s origin + size representation might still be better for layout-ish things.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/320)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.