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

Make dup_x etc. methods public #14

Closed
bitshifter opened this issue Jul 22, 2019 · 8 comments · Fixed by #86
Closed

Make dup_x etc. methods public #14

bitshifter opened this issue Jul 22, 2019 · 8 comments · Fixed by #86
Labels
enhancement New feature or request

Comments

@bitshifter
Copy link
Owner

These are currently pub(crate) but they look like they'd be useful externally based on cessen/psychopath@88e7365#diff-7e162a40d8f4de03dc9ab929005466cfR65

@bitshifter bitshifter added enhancement New feature or request good first issue Good for newcomers labels Jul 22, 2019
@cessen
Copy link
Contributor

cessen commented Jul 23, 2019

Do you mind if I go ahead and expose these now, so I can start using them in Psychopath?

@bitshifter
Copy link
Owner Author

Sure, go for it.

@cessen
Copy link
Contributor

cessen commented Jul 23, 2019

Looking at Vec3 and Vec2, I'm not actually sure what semantics we want there.

For example, in my use-case, even if I were using Vec3 I would still want the dup_* methods to return a Vec4, since I'm using it to arrange data for full SIMD utilization. But I don't know if that's what the semantics should actually be more generally. Right now the Vec3::dup_* methods return another Vec3.

Do you have any thoughts on that?

@bitshifter
Copy link
Owner Author

bitshifter commented Jul 23, 2019

That is a good point. It's not really an easy thing to handle nicely at the moment.

The cases I have internally I've kept it in the same type as that's what my usage has been (e.g. Vec3::dup_x() -> Vec3).

Long term, what I was thinking with a swizzle macro would solve that, for example vec3_swizzle!(v, 0, 0, 0, 0) would produce a Vec4 based on the number of lane parameters. That was the idea anyway, I'm hoping it's feasible.

You could use the existing Vec4::dup_x if you could cast your Vec3 to a Vec4 without going through a redundant extend call, but that wouldn't be a very safe API.

It could be solved by adding Vec3::dup_x_vec4 or something but I think I'd rather not do that as it would lead to an explosion of methods.

Perhaps I should leave these as is for now.

If you want to use intrinsics directly you can convert Vec3 and Vec4 to and from __m128 and you could cast a Vec3 to a Vec4 by doing that, a bit ugly though and this only works if SSE2 is available.

@cessen
Copy link
Contributor

cessen commented Jul 23, 2019

Yeah, that all makes sense.

I think maybe what we should do for now, then, is just expose the Vec4::dup_* methods since we have a clear concrete use-case for them, and leave the other ones private for now. When we have a better idea of what they might be useful for, we can expose them with the right semantics then. Does that sound reasonable to you?

@bitshifter
Copy link
Owner Author

Yeah, it probably makes sense.

@cessen
Copy link
Contributor

cessen commented Jul 23, 2019

The more I'm thinking about this, the more I actually kind of don't want to expose this. It seems so special-cased, and I think waiting for a shuffle macro would make more sense. My code in Psychopath still works just fine, even if it isn't maximally efficient, and I can change it once we have shuffling.

@bitshifter
Copy link
Owner Author

Alright, I'll leave this open in case anyone else appears who has a need for it.

@bitshifter bitshifter removed the good first issue Good for newcomers label Aug 13, 2019
bitshifter added a commit that referenced this issue Oct 27, 2020
Add vector swizzle functions.

Fixes #14 and #15.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants