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

Please do not verbatim-copy JOML code #2

Closed
httpdigest opened this issue Sep 13, 2016 · 6 comments
Closed

Please do not verbatim-copy JOML code #2

httpdigest opened this issue Sep 13, 2016 · 6 comments

Comments

@httpdigest
Copy link

httpdigest commented Sep 13, 2016

The Mat4d class is almost a verbatim copy of JOML's Matrix4f, including the JavaDocs, whose references do not match anymore, because glm just renamed the class and method names.

Examples:
JOML 1.7.1: https://github.com/JOML-CI/JOML/blob/5b9b0c0aa7e374dba08cfca4fe47bdd9c8cc6d03/src/org/joml/Matrix4f.java#L7283-L7300

glm:

/**
* Compute a normal matrix from the upper left 3x3 submatrix of
* <code>this</code> and store it into the upper left 3x3 submatrix of
* <code>dest</code>. All other values of <code>dest</code> will be set to
* {@link #identity() identity}.
* <p>
* The normal matrix of <tt>m</tt> is the transpose of the inverse of
* <tt>m</tt>.
* <p>
* Please note that, if <code>this</code> is an orthogonal matrix or a
* matrix whose columns are orthogonal vectors, then this method <i>need
* not</i> be invoked, since in that case <code>this</code> itself is its
* normal matrix. In that case, use {@link #set3x3(Matrix4f)} to set a given
* Matrix4f to only the upper left 3x3 submatrix of this matrix.
*
* @see #set3x3(Matrix4f)
*
* @param dest will hold the result
* @return dest
*/
public Mat4d invTransp3(Mat4d dest) {

If you want to copy code and even JavaDoc verbatim, then you should at least give credit or ask for permission.

@ghost
Copy link

ghost commented Sep 14, 2016

I completely agree with httpdigest, not giving credit is neither professional nor respectful.

@elect86
Copy link
Collaborator

elect86 commented Sep 14, 2016

Sorry sorry sorry, you are right, I should't have done that!

That's the problem when you are overloaded of work and you copy paste lazily a method surely working just to get a bug fixed (and then for the same laziness you finish to do the same for almost the whole class..) and postpone to give credits or find what the bug was. I did that when I opened that issue about the normalization input vector... I don't know if you remember

However I modified quickly the readme to give you asap credits and put a quick fix at this issue

I'll go tomorrow through the code, because there are also a couple of other methods in the Glm from joml

Please again sorry, Kai, I really didn't mean to provoke this. I don't know if this has the same meaning in english, but I completely did that in good faith (remember when I contributed a little to joml), with no bad intentions

@httpdigest
Copy link
Author

Alright, then. No worries. :) I appreciate your response.

@elect86
Copy link
Collaborator

elect86 commented Sep 15, 2016

Oh nice, I am glad you appreciated :)

I removed some unused methods coming from joml and modified again the readme writing explicitly "Credits:" and listing the joml methods I left there... I'd like to ask you if I may keep those remaining methods until I manage to review with calm the whole class

Ps: let me offer you a drink :p

@httpdigest
Copy link
Author

Hey, what's up. Just one thing:

and return the object itself in order to concatenate operations

You don't have to go overboard with credits for API design concepts that no one did invent and should claim credit for. :) Actually, now I feel a bit worried for my first post here.
What I meant with the first post was just about verbatim copying code including the JavaDocs.
I of course certainly do not take credit for simple API design concepts. :)

Cheers,
Kai

@elect86
Copy link
Collaborator

elect86 commented Nov 5, 2016

Hey Kay, sorry for the late reply, yeah you are right

I just felt so guilty that I overreacted :p

Thanks for your message though :)

Have a nice we!

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

No branches or pull requests

2 participants