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

Rename HomogeneousVector into Vector4D #356

Closed
wants to merge 1 commit into from
Closed

Conversation

@nical
Copy link
Collaborator

nical commented Jul 3, 2019

Up for debate of course, but I think Vector4D is nicer and looks more like what people are used to.


This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented Jul 3, 2019

r? @kvark

@kvark
Copy link
Member

kvark commented Jul 3, 2019

(before reading the code)
I don't think this change fits into euclid approach today. It's a matter of choosing the level of abstraction to cover. I.e. is this a generic math library, or is it more geometry-specific thing? "Homogeneous vector" has an important semantics (that it was made for and used for, exclusively) on top of a generic 4D vector that you are trying to turn it into.
Existing precedent is the removal of post_mul for matrices in favor of post_transform: these are the same thing on a different level: "transform" aspect is more practical, while "mul" is more abstract, theoretical.

@nical
Copy link
Collaborator Author

nical commented Jul 3, 2019

I don't feel strongly about this one, and I don't disagree with what you said, so let's just drop this PR.

@nical nical closed this Jul 3, 2019
bors-servo added a commit that referenced this pull request Jul 4, 2019
Rename default::Vector4D into default::HomogeneousVector

Since we didn't end up taking #356.

<!-- 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/359)
<!-- 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

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