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

More undefined reference errors #490

Merged
merged 10 commits into from
Mar 15, 2016

Conversation

kankaristo
Copy link

Continuing from #488 and #489...

I found more undefined references with different operator functions. I thought I got them all with the last PR, but noticed that I didn't have operator/ working yet.

This is still work-in-progress and I'd like some feedback:

  • I removed the friend operator/ member functions from mat2x2, mat3x3 and mat4x4.
    • I think these are left over from some older version and I can't find them in any other classes. In fact, they seem to be the only friends in the entire code base.
    • They clash with similar functions in the glm namespace (undefined reference errors if you use them).
    • Are they still used, or is it OK to remove them?
  • Parameter naming for operator functions with tvec1 parameters.
    • In some places they're named v1 or v2 (because it's a vector type).
    • Other places call them scalar (one-dimensional vector, so it's a single number).
    • My previous PR called them v1 and v2, because that's what tvec2 called them, but I noticed this a bit inconsistent.
    • Which naming convention should I use (or does it matter)? Would you like for me to go over all operator functions and make them consistent?

I'd also like to take a closer look (probably tomorrow) to see if I can find any more of these. There are plenty of operators I'm not (yet) wrapping with SWIG, but I might as well get them all fixed in one go (if I find any more).

These don't seem to have matching definitions and they conflict with
similar functions in the glm namespace (in the same header files).
@Groovounet
Copy link
Member

I agree that "friend operator/" looks like (very) old left over and actually U can't remember what was the use of this.

I think the conversion of tvec1 should be v1 - v2 style like other vector types. This said, I don't believe any of this is really consistent.

@kankaristo
Copy link
Author

Alright, I think I've got them all (ready for review).

I removed const & from some operator functions where the parameter was a POD type, in order to match other functions for same type (tvec4). I added const & to other POD parameters, so that the functions would match other functions for that type (tmat2x4). I wanted to keep changes minimal, so I didn't start doing this across all types.

Both tquat and tdualquat were missing declarations for boolean operators (== and !=), so I added those.

Some operator declarations for tdualquat used tquat instead of tdualquat, so I fixed those. The unary operator- seemed to be doing what operator+ should do, and the definition for the unary operator+ was missing.

I have to admit that I don't really even know what a dual quaternion is used for (I have a limited understanding of normal quaternions), so these should really be checked by someone who knows more about them.

Groovounet added a commit that referenced this pull request Mar 15, 2016
@Groovounet Groovounet merged commit 93d09e0 into g-truc:master Mar 15, 2016
@Groovounet Groovounet added the bug label Mar 15, 2016
@Groovounet Groovounet added this to the GLM 0.9.8 milestone Mar 15, 2016
@Groovounet Groovounet self-assigned this Mar 15, 2016
@Groovounet
Copy link
Member

This is a really good work!

Thanks for contributing!
Christophe

@kankaristo kankaristo deleted the more-undefined-reference-errors branch March 15, 2016 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants