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

Introduce box/boxs types instead of passing an array of two vec3(s) around #359

Open
v1993 opened this issue Dec 1, 2023 · 3 comments
Open

Comments

@v1993
Copy link
Contributor

v1993 commented Dec 1, 2023

While a typedef vec3[2] box would already be nice to have in array API since it's more explicit about meaning of a value, struct API would benefit from this change the most. Currently, due to lack of a dedicated structure, functions that return a box utilize an out parameter instead of actually returning the struct, e.g.

CGLM_INLINE
void
glms_aabb_(merge)(vec3s box1[2], vec3s box2[2], vec3s dest[2]) {

This largely runs contrary to the whole idea of struct API, which tries to avoid out parameters and instead return results.

By introducing a boxs struct this could be changed to far more natural

CGLM_INLINE
boxs
glms_aabb_(merge)(boxs box1, boxs box2)

However, unlike typedef in array API, this would be a breaking change; thus I'm unsure what's the best course of action here.

P.S.: I suppose types could be called aabb/aabbs instead to avoid clashing with more generic box term and match type function prefixes as well.

@recp
Copy link
Owner

recp commented Dec 2, 2023

Hi @v1993,

You are right, struct api could return a type but I'm not sure for bringing new type since engines may have an AABB type internally, introducing a new one could lead to conflicts with existing typenames

Still, we can consider adding a new type, such as 'aabb' or 'glm_aabb_t', maybe just for struct api. Let's get more feedback to see if there are any concerns or suggestions.

@v1993
Copy link
Contributor Author

v1993 commented Dec 2, 2023

I think that it would be good to have it in both APIs, at very least for consistency. Not quite sure about type name conflicts - cglm already defines its types without any prefixes, so I'd probably go for consistent, if somewhat conflict-inducing, naming scheme.

If you're worried about possible conflicts, then it might be a good idea to "scope" all types, e.g. name them glm_<name>_t with an (most likely enabled by default) option to typedef them to unscoped names. That way, users who prefer a little extra verbosity over possibilty of a conflict can disable shorthand names and use full ones.

That said, I'm just voicing my thoughs here - I'm actually working on Vala bindings for cglm (struct API turned out to be incredible for this use case), so C names don't really matter for me - only function signatures do.

@duarm
Copy link
Contributor

duarm commented Dec 3, 2023

One of the things I like about cglm is that it doesn't prefix types, it's too much verbosity for the sake of avoiding a possible conflict, and everyone pays the price. It should be optional if it's a concern.

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

No branches or pull requests

3 participants