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 BoolVector2D/3D. #287

Merged
merged 1 commit into from Apr 13, 2018
Merged

Add BoolVector2D/3D. #287

merged 1 commit into from Apr 13, 2018

Conversation

@nical
Copy link
Collaborator

nical commented Apr 11, 2018

This is something I have been missing for a while, not enough to go ahead and make the PR until now, but I think it is a nice addition.

This adds boolean vectors similar to what you can see in glsl and let you rewrite things like:

if some_vec3.x > another_vec3.x ||
  some_vec3.y > another_vec3.y ||
  some_vec3.z > another_vec3.z) {
  // do the thing!
}

into:

if some_vec3.greater_than(&another_vec3).any() {
  // do the thing!
}

We don't get to use the > operator because rust wants it to return a boolean, but I think it already makes things quite a bit nicer as is.
This PR doesn't implement all of the various candies we can think of for vector boolean ops, nor does it implement BoolVector4D but they can be added as followups if we need/want them.


This change is Reviewable

@nical nical force-pushed the nical:bvec branch from f6389f1 to 4ecfa4c Apr 11, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 11, 2018

r? anyone

@kvark
Copy link
Member

kvark commented Apr 11, 2018

First question, why are those separate types instead of just extra methods for TypedVector2D<bool, U>?

@nical
Copy link
Collaborator Author

nical commented Apr 12, 2018

I don't think the unit makes sense for these types, and the boolean vectors have a completely separate set of methods.

@kvark
Copy link
Member

kvark commented Apr 12, 2018

Second question is: what is the use case for actually needing BoolVector2D/3D as opposed to going straight to any()?

Copy link
Member

kvark left a comment

Needs some tests. Otherwise LGTM

}

#[inline]
pub fn xy(&self) -> BoolVector2D {

This comment has been minimized.

@kvark

kvark Apr 12, 2018

Member

that's just a subset of all possible swizzles. Is there a particular reason to only include those? Are those needed in practice?

This comment has been minimized.

@nical

nical Apr 12, 2018

Author Collaborator

These are the swizzles we currently have in TypedVector2D. I wouldn't mind if we add the other ones but there are so many of them I'd rather just wait until someone asks for them.

I have used them a bit outside of webrender, but the one I use most at the moment is to go from 3d to 2d because foo = myvec.xy() is nicer and more explicit than foo = myvec.to_2d() in my opinion.
Having them doesn't add any real complexity so I don't think it's worth arguing over really.

This comment has been minimized.

@kvark

kvark Apr 12, 2018

Member

yeah, myvec.xy() is nicer, but it's not clear why we shouldn't then be able to do myvec.yx()

This comment has been minimized.

@nical

nical Apr 12, 2018

Author Collaborator

Totally. There is really no particular reason as far as I am concern to not add yx() and friends other than being busy with other things. If you write the PR I'll r+ it.

@kvark
Copy link
Member

kvark commented Apr 13, 2018

@nical alright, so how do you feel about adding some tests?

@nical nical force-pushed the nical:bvec branch from 4ecfa4c to 5a0648a Apr 13, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 13, 2018

Done.

@kvark
kvark approved these changes Apr 13, 2018
Copy link
Member

kvark left a comment

Thanks!
what's up with the first commit?
otherwise r=me

@nical
Copy link
Collaborator Author

nical commented Apr 13, 2018

I'm tempted to say a github bug. It's the tip of the master branch, not sure why it shows up here since it is in master already. I tried to remove it and rebase on top of master but it still shpws up here. Anyway it rebases/merges cleanly so we can land now.

@KiChjang
Copy link
Member

KiChjang commented Apr 13, 2018

Hmm... the first commit hash differs from that of the commit found in the master branch... perhaps during rebase, it asked for you to recommit a commit that was in the master branch?

@nical
Copy link
Collaborator Author

nical commented Apr 13, 2018

I did:

git rebase -i @^^^
# removed the undesired commit
git pull --rebase upstream master

It didn't ask me anything in particular.
Git isn't my strong suit, I'll try to re-build the branch with cherry-pick instead of rebase to see if it works better.

@nical nical force-pushed the nical:bvec branch from 5a0648a to da7865a Apr 13, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 13, 2018

Looks like it worked.

@kvark
Copy link
Member

kvark commented Apr 13, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2018

📌 Commit da7865a has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2018

Testing commit da7865a with merge a54f33a...

bors-servo added a commit that referenced this pull request Apr 13, 2018
Add BoolVector2D/3D.

This is something I have been missing for a while, not enough to go ahead and make the PR until now, but I think it is a nice addition.

This adds boolean vectors similar to what you can see in glsl and let you rewrite things like:

```rust
if some_vec3.x > another_vec3.x ||
  some_vec3.y > another_vec3.y ||
  some_vec3.z > another_vec3.z) {
  // do the thing!
}
```

into:

```rust
if some_vec3.greater_than(&another_vec3).any() {
  // do the thing!
}
```

We don't get to use the `>` operator because rust wants it to return a boolean, but I think it already makes things quite a bit nicer as is.
This PR doesn't implement all of the various candies we can think of for vector boolean ops, nor does it implement BoolVector4D but they can be added as followups if we need/want them.

<!-- 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/287)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2018

☀️ Test successful - status-travis
Approved by: kvark
Pushing a54f33a to master...

@bors-servo bors-servo merged commit da7865a into servo:master Apr 13, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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

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