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

Transform2 type #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Transform2 type #54

wants to merge 2 commits into from

Conversation

wackbyte
Copy link

@wackbyte wackbyte commented May 17, 2020

I added a Transform2 type and moved Transform to Transform3. Fixes #4.

@yoanlcq
Copy link
Owner

yoanlcq commented May 17, 2020

Hi,

Thanks for the PR. I'll happily merge it once you fix the errors reported by AppVeyor CI.

Renaming Transform to Transform3, although a good naming choice for the sake of consistency with Transform2, is a breaking change I am somewhat reluctant to accept. I think it is better to just leave it as it is, especially since 3D is the default, common case.

Users can always use renaming imports if they wish. On our end, however, I do not think pub use Transform3 = Transform; is a good thing to do either, because then users would wonder which they should use and why there is an alias at all.

In any case, this is a welcome addition. Just please rename Transform3 back to Transform, the benefits largely outweight the disadvantages. Thanks in advance!

While I'm at it, would you be opposed to renaming Transform2 to Transform2D? Both names are fine to me, but I prefer the second one.

@wackbyte
Copy link
Author

I prefer Transform2 as it uses the same naming scheme as the other types, like Vec3 or Mat4.

@Koopa1018
Copy link

Koopa1018 commented Aug 13, 2020

I would argue against Transform2 as well, because it doesn't adequately communicate the intent of the the type. Having it named like Vec3 and Mat4 suggests that it is like Vec3 and Mat4--a vector type of some kind--which isn't what Transform is for at all!

If we additionally assume users will look for Transform2d when they need a 2D Transform, suddenly we've got two layers of confusion: "I need a 2D Transform type but there's only this Transform2 thing; is that what I need, or is that a vector, or...????"

@yoanlcq
Copy link
Owner

yoanlcq commented Aug 14, 2020

Just my 2 cents, while I'd be in favor of naming it Transform2D or Transform2d, I don't think naming it Transform2 is that big a deal either. The struct can be quickly accessed and figured out, and in some IDEs or editors, the doc comment is shown when hovering over the type.
I don't think it can be confused with a vector, indeed because there is Vec2 already, so there is not much else a Transform2 can be.

So I don't think the name is that ambiguous, and in rare cases where it would be, it's easy to access the documentation and figure it out quickly.

I get that this is fairly subjective; for this kind of stuff, it's good to see how other well-known libraries do it.

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.

Add Transform2D struct
3 participants