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

Added examples in Matrix4.js for #651 #724

Merged
merged 8 commits into from
May 2, 2013
Merged

Conversation

nobelium
Copy link
Contributor

@nobelium nobelium commented May 1, 2013

Added examples for
Matrix4.toArray
Matrix4.getColumn
Matrix4.setColumn
Matrix4.getRow
Matrix4.setRow
Matrix4.negate
Matrix4.transpose

@@ -796,6 +796,17 @@ define([
* @return {Array} The modified Array parameter or a new Array instance if one was not provided.
*
* @exception {DeveloperError} matrix is required.
*
* @example
* //converts a matrix of order 4 to an array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These examples look good. Thanks @nobelium.

Can we just do a minor wording tweak to make them a bit more friendly to non-math folks by changing "matrix of order 4" to "Matrix4" or "4x4 matrix?"

@nobelium
Copy link
Contributor Author

nobelium commented May 2, 2013

@pjcozzi I have rephrased those lines and added some more examples.

* var a = Matrix4.toArray(m);
*
* // m remains the same
* //creates a = [10.0, 11.0, 12.0, 13.0, 14.0, 15.0, 16.0, 17.0, 18.0, 19.0, 20.0, 21.0, 22.0, 23.0, 24.0, 25.0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this in the debugger? This shows a row-major ordering, but I believe it should be column-major.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 2, 2013

Thanks @nobelium. Just had those few comments. Matrix4 looks a lot better now.

@nobelium
Copy link
Contributor Author

nobelium commented May 2, 2013

@pjcozzi Sorry, my bad. I have corrected the mistakes..

@pjcozzi
Copy link
Contributor

pjcozzi commented May 2, 2013

Thanks @nobelium

pjcozzi added a commit that referenced this pull request May 2, 2013
Added examples in Matrix4.js for #651
@pjcozzi pjcozzi merged commit 22de8d3 into CesiumGS:master May 2, 2013
@mramato
Copy link
Contributor

mramato commented May 2, 2013

Just an FYI this change introduced a bunch of line-ending and trailing whitespace issues reported by jsHint. (fixed in 2e9cbe7) @nobelium It's always a good idea to run ant jsHint on your code to make sure it reports no errors. There's also a chance this is the result of an incorrect autocrlf setting. Please verify this setting is correct by following the directions in the Contributor's Guide

Thanks again for the changes. I only mention this so that your future pull requests go smoothly.

@nobelium
Copy link
Contributor Author

nobelium commented May 3, 2013

Thanks, I did not know that trailing whitespace would create an issue.

@nobelium nobelium deleted the examples branch May 3, 2013 02:15
@mramato
Copy link
Contributor

mramato commented May 3, 2013

There's only a few cases where it can lead to a genuine problem, such as multiline strings. For the most part it's about keeping the code clean and consistent. Extra space and differing line breaks make diffing and maintencence a lot harder. Thanks again for the changes.

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

Successfully merging this pull request may close these issues.

3 participants