Skip to content

Add component-wise ceiling division of 2d and 3d vectors #482

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

Closed
wants to merge 1 commit into from

Conversation

alexkirsz
Copy link

Rest of the fix for #480 (#481 is the first part).

I implemented this in its own PR in case there are issues with this particular API/implementation, while I think #481 is simple enough not to be controversial.

@bors-servo
Copy link
Contributor

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

@nical
Copy link
Contributor

nical commented Apr 12, 2021

I'm a tad scheptical of adding ceiling_division to euclid. What is your use case for it? It's a bit unclear what the semantics should be with negative numbers and more generally what it means in the context of linear algebra.

@alexkirsz
Copy link
Author

One use case where I needed this was for filling a grid of size MxN pixels with tiles of size OxP, where you'll need ceiling_div(M, O)xceiling_div(N, P) tiles.

@alexkirsz alexkirsz force-pushed the component_ceiling_div branch from b3671c5 to 0b7adbb Compare April 13, 2021 09:44
@nical
Copy link
Contributor

nical commented Apr 13, 2021

IMHO it's a bit too niche for inclusion in euclid and easy enough to implement outside of this crate.

@alexkirsz
Copy link
Author

I can totally see that. In fact, now that we have component_div, it can more easily be implemented with:

(a + b - Vector2::one()).component_div(b)

I'll close this PR and the associated issue. Thanks for taking the time to review and discuss this! 😄

@alexkirsz alexkirsz closed this Apr 14, 2021
@nical
Copy link
Contributor

nical commented Apr 14, 2021

Thanks for your understanding and contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants