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

ADL support #239

Closed
wants to merge 2 commits into from
Closed

ADL support #239

wants to merge 2 commits into from

Conversation

Jaxan
Copy link

@Jaxan Jaxan commented Sep 19, 2014

We moved from our own math library to glm and noticed some difference. For example we had something like this in our codebase:

using vec3 = ourlib::vec3;

vec3 a(1,0,0);
auto b = normalize(a);

Note that this works as long as normalize is defined in the same namespace as the vectortype (this is how ADL works). I was hoping that we could change to glm by just changing the typedefs. However in this case normalize could not be found, as normalize is not in the namespace glm::detail.

In order to let this work I made this header (as an extension of course), which injects all glsl functions in the glm::detail namespace.

Pros:

  • Switching between math libraries is easier.
  • Still better than using namespace glm;.

Cons:

  • Different functions might be called depending on which headers are included.

Fun fact: there is a one-line trick to inject the detail namespace into the glm namespace: http://ideone.com/ZqUiHX (the hack is on line 20). But this is just weird ;).

Any thoughts?

@NickNick
Copy link

For what it's worth, I'd rather see this in the specific headers that are included now in this one header, so that ADL is always functioning after including trigonometric.hpp, exponential.hpp, etc. instead of also having to include this one header.

@Groovounet
Copy link
Member

I resolved this issue a little differently by moving tvec* and tmat* types to glm namespace.

I think this should resolve your issue.

Thanks,
Christophe

@Groovounet Groovounet closed this Oct 5, 2014
@Groovounet Groovounet added this to the GLM 0.9.6 milestone Oct 5, 2014
@Groovounet Groovounet self-assigned this Oct 5, 2014
@Jaxan
Copy link
Author

Jaxan commented Oct 6, 2014

That should also resolve the issue indeed. The reason I implemented it like this was that I didn't want to change glm, but just add behaviour in our codebase (so that we could stick to glm 0.9.5.4 in brew).

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants