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

Matrices package #111

Merged
merged 35 commits into from Sep 14, 2018

Conversation

Projects
None yet
3 participants
@GetLocalPlayer
Copy link
Contributor

GetLocalPlayer commented Aug 31, 2018

No description provided.

GetLocalPlayer added some commits Aug 29, 2018

@SamuelMoriarty
Copy link
Contributor

SamuelMoriarty left a comment

Stdlib needed this for a long time, but there are improvements that can be made.

Also, major point: This really should be unit-tested.

this.rc10*m.rc00 + this.rc11*m.rc10, this.rc10*m.rc01 + this.rc11*m.rc11)

/** Returns matrix' column by id as a vector or zero-vector if doesn't exist. */
public function mat2.col(int id) returns vec2

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Can you test how well this (and .row()) get inlined?
Consider using a temp-var maybe instead of separate return statements.

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 1, 2018

Author Contributor

Didn't think about inlining at all. You're right, I'll rewrite it with a temporary variable.



/* =========== 2X2 MATRIX =========== */
/** Matrxi 2x2

This comment has been minimized.

@SamuelMoriarty

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 1, 2018

Author Contributor

Thanks

this.rc20*scalar, this.rc21*scalar, this.rc22*scalar)

/** Matrix multiplication is not commutative that means matrix*vect is not the same as vec*matrix. */
public function real.op_mult(mat3 m) returns mat3

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

How is this comment relevant to scalar multiplication?

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 1, 2018

Author Contributor

Funny, thanks.

this.rc20*m.rc02 + this.rc21*m.rc12 + this.rc22*m.rc22)

/** Returns matrix' column by id as a vector or zero-vector if doesn't exist. */
public function mat3.col(int id) returns vec3

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Same consideration as with mat2 regarding inlining

this.rc01, this.rc11, this.rc21,
this.rc02, this.rc12, this.rc22)

/** Returns a zero-matrix in case there's no iverse matrix.*/

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Doc comment should describe more than just it's corner case.
Also, consider adding a .hasInverse() function instead of returning a zero-matrix, or returning a result tuple like
inverseresutl(boolean success, mat3 result).

Trying to check if this call succeeded will otherwise entail 9 comparison if you want to compare it against the zero-matrix.

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 1, 2018

Author Contributor

For such kind of test we have to calc determinant and once more in .inverse if it's not 0. More operations that's undesirable. I think tuple-result is the best solution thanks for the hint.

this.rc00*this.rc11 - this.rc01*this.rc10)
return ZERO33

/** Extracts Euler-angles (Tait-Bryan) from rotation matrix. Order of result angles is X-Y-Z.*/

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Why Tait-Bryan and not classical?

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 1, 2018

Author Contributor

The only reason for this function to exist is BlzSetSpecialEffectOrientation that requires angles in X-Y-Z order. In most cases it's better to avoid Euler angles usage. I rather replace this one with effect.setOrientation(mat3/quat)

return vec3(yaw, pitch, roll)

public function mat3.toString() returns string
return "Matrix 3x3\n{0} {1} {2}\n{3} {4} {5}\n{6} {7} {8}".format(

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Don't use .format(), it's very infefficient.

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 1, 2018

Author Contributor

Is performance above readability in such debug functions?

This comment has been minimized.

@Frotty

Frotty Sep 2, 2018

Member

it's fine.



public let ZERO22 = mat2(0, 0, 0, 0)
public let ZERO33 = mat3(0, 0, 0, 0, 0, 0, 0, 0, 0)

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Needs a better constant name.
Also, use public constant vs public let.

public let ZERO22 = mat2(0, 0, 0, 0)
public let ZERO33 = mat3(0, 0, 0, 0, 0, 0, 0, 0, 0)

public let IDENTITY22 = mat2(

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Ditto.

1, 0,
0, 1)

public let IDENTITY33 = mat3(

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Ditto.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Aug 31, 2018

Yup, unit tests are mandatory.


public function mat2.op_mult(real scalar) returns mat2
return mat2(
this.rc00*scalar, this.rc01*scalar,

This comment has been minimized.

@Frotty

Frotty Aug 31, 2018

Member

You leave spaces around - and + but not *. Pls fix for *

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 1, 2018

Author Contributor

As far as I know an operation with high priority doesn't require space around it. In most languages. That means I should leave one space around - and + but shouldn't do that around * and /. In this case you ask me to fix it for * but not for /. Is there any wurst-style doc I need to learn?


/** Returns a zero-matrix in case there's no inverse matrix.*/
public function mat2.inverse() returns mat2
var d = this.determinant()

This comment has been minimized.

@Frotty

Frotty Aug 31, 2018

Member

use let for constant locals.

00 01
10 11
*/
public tuple mat2(real rc00, real rc01, real rc10, real rc11)

This comment has been minimized.

@Frotty

Frotty Aug 31, 2018

Member

Get rid of rc. Either use the mathematical convention ij or m

var z = v.z
var cos = angl.cos()
var sin = angl.sin()
return mat3(

This comment has been minimized.

@Frotty

Frotty Aug 31, 2018

Member

this is a prime example of why I don't like having an empty row mat3( followed by un-tabulated entries.
This to me just looks confusing/out of place.
I would suggest filling the first row and then properly aligning the cells, e.g.:

mat3(	1 + (1 - cos)*(x*x - 1), 	-z*sin+ (1 - cos)*x*y, 		y*sin+ (1 - cos)*x*z,
	z*sin+ (1 - cos)*x*y, 		1 + (1 - cos)*(y*y - 1), 	-x*sin+ (1 - cos)*y*z,
	-y*sin+ (1 - cos)*x*z, 		x*sin+ (1 - cos)*y*z, 		1 + (1 - cos)*(z*z - 1))

This applies to all of these statements above, especially mat2 with short params.

Also these calcs are overly big. You should e.g. save 1 - cos in a var

GetLocalPlayer added some commits Sep 2, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 2, 2018

  • Most matrices tests were added.
  • Cannot produce Mat2Euler tests The compiler was out of date.

You encountered a bug in the interpreter: java.lang.Error: The native Asin has not been implemented for compiletime!

  • rc replaced with m
  • .inverse now returns inverseresult22 and inverseresult33 tuples
  • Such strings as
    return mat2(
                this.m00*scalar, this.m01*scalar,
                this.m10*scalar, this.m11*scalar)

rewritten as

    return mat2(this.m00*scalar,    this.m01*scalar,
                this.m10*scalar,    this.m11*scalar)
  • .col and .row were rewritten to support inlining
@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 2, 2018

You encountered a bug in the interpreter: java.lang.Error: The native Asin has not been implemented for compiletime!

I will add it. It's added.

Regarding spacing around operators, intellij simply spaces every, and I don't see any point in having some unintuitive rule for this. Just space everything consistently. if you want to clarify parts of a term, use brackets or local vars.



@test
function test()

This comment has been minimized.

@Frotty

Frotty Sep 2, 2018

Member

That's not good.
Please see StringTests and tests in Maths/Vectors as examples.
Make smaller, self contained functions, annotate them each individually and most importantly:
get rid of the prints and add asserts.
Assertions are what make tests fail, right now they will never fail and thus are completely pointless.

You can run tests locally in vscode, F1 -> run tests in current file e.g.

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 2, 2018

Author Contributor

What else should I know to not rewrite it again again and again. How many instructions do you have and why I can't see the whole list?

This comment has been minimized.

@Frotty

Frotty Sep 2, 2018

Member

You could check out existing packages before doing something very different.

There is no list yet, but I also thought about it being useful in the future. Should probably start a code conventions and contribution doc.

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 2, 2018

Author Contributor

And I did. For example, Vectros package contains * with space around it and without. Or Angles package. But I have to do this strange thing.

This comment has been minimized.

@Frotty

Frotty Sep 2, 2018

Member

I'm talking about the tests. Obviously many packages are old, made or modified by different people, resulting in different styles. Especially before we had proper PRs and reviews.
New additions however, should follow the convention.
Soon the formatting should be standardized, if @karlek finishes wurstscript/WurstScript#620 and then we can easily format the stdlib consistently.

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 2, 2018

Author Contributor

New additions however, should follow the convention.

Which doesn't exist.
Dunno how many surprises are waiting for me. Maybe later.

This comment has been minimized.

@Frotty

Frotty Sep 2, 2018

Member

I'm sorry to hear you are disappointed by the pull request process, but I don't think it's anything out of the ordinary.
It can easily take weeks before a pull request is accepted, on any project.
Just check other PRs on our projects and other repositories.

I really don't understand why you get so frustrated. We appreciate your contribution, just take a little time to do improvements. This isn't critical and it's fine if you come back in a week or two.

Also, there is no need to close this. Just keep this PR if you ever plan to continue it, or even someone else can make these modifications.

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 2, 2018

Author Contributor

May be because I hardly distinguish speech styles but not, I'm not frustrated.
I'm not interested in WC3 much enough to keep going hint2hint in a dark room so I decided to close this until there's the convention and correct list of requirements. As I said, maybe latter.

@GetLocalPlayer GetLocalPlayer referenced this pull request Sep 2, 2018

Merged

Quaternions package #112

@Frotty Frotty reopened this Sep 2, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 4, 2018

How reliable is this?

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 4, 2018

Still subject to change

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 5, 2018

Can tests be done in separate files for each matrix type? Should I create an individual PR for each of those test files? Can custom mat2.assertEquals function be used with testFails in it instead of real.assertEquals. Or it can be only those ones that are defined in Wurstunit?

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 5, 2018

Can tests be done in separate files for each matrix type?

I think one extra file MatrixTests should suffice, but sure.

Should I create an individual PR for each of those test files?

No, as it's directly bound to this PR. Tests are required for this code to be accepted.

Can custom mat2.assertEquals function be used with testFails

Not sure what you mean by testFails. You can make assert extension functions for mat like the one in Wurstunit.

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 6, 2018

Tests were united. MatricesTests pacgake was added.

GetLocalPlayer added some commits Sep 6, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 7, 2018

Jesus, what a mess I've done. Should I do anything with it? I'm sorry to ask, I'm quite newbie in Git.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 7, 2018

Many commits != mess. I will squash them all into one commit on merging anyway.
You could start using git on your machine, instead of using the web interface, but doesn't really matter for the PR.

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 7, 2018

It is done.

Show resolved Hide resolved wurst/math/Matrices.wurst Outdated
Show resolved Hide resolved wurst/math/Matrices.wurst Outdated
Show resolved Hide resolved wurst/math/Matrices.wurst Outdated
import initlater Printing
import initlater Matrices

This comment has been minimized.

@Frotty

Frotty Sep 10, 2018

Member

?? Is this import even used. In any case, try to not have initlater imports in the stdlib, it's really just a last remorse for users with complex cyclic dependencies.

This comment has been minimized.

@GetLocalPlayer

GetLocalPlayer Sep 10, 2018

Author Contributor

Fixed.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 10, 2018

For all multiplication operations you should specify in hotdoc, whether you are using pre- or post-multiplication.

Also, you offer creation of rotation and scaling matrix, so please add translation matrix as well. Rename "rotation matrix" related funcs accordingly. You should also add a mat3.toRotation(angle val) that takes just an angle.

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 10, 2018

For all multiplication operations you should specify in hotdoc, whether you are using pre- or post-multiplication.

This is not clear for me.

Also, you offer creation of rotation and scaling matrix, so please add translation matrix as well.

Translation matrix for vec2 is 3x3 matrix (with implicit conversion vec2 to vec3). Translation matrix for vec3 is 4x4 matrix (with implicit conversion vec3 to vec4 as well). First can be done easily. Do you require to add mat4 in the package?

You should also add a mat3.toRotation(angle val) that takes just an angle.

What should such function do? Any 3D rotation can be represented as axis-angle but just an angle?

GetLocalPlayer added some commits Sep 10, 2018

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 10, 2018

This is not clear for me.

Write in doc for the non commutative funcs which operation you are doing. But i guess it's fine due to form of extension functions implying it already.

Do you require to add mat4 in the package?

vec2 to mat3 trans should suffice for now unless you're very keen on making a mat4 tuple reality :)

What should such function do? Any 3D rotation can be represented as axis-angle but just an angle?

It set the matrix to a rotation matrix that will rotate any vector in counter-clockwise direction around the z-axis, meaning 0,0,1 for the 3rd row

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 10, 2018

But i guess it's fine due to form of extension functions implying it already.

Well, non commutative functions are op_mult where pre/post-multiplication is described by an expression itself vec * mat ormat * vec. For .toEuler and the others I'll add Each column represents an axis in a rotation matrix in tuples hotdocs.

vec2 to mat3 trans should suffice for now

Will it be confusing if I overload op_mult for vec2 in case mat3 * vec2 = vec2? 2d translation and rotation through one matrix.

unless you're very keen on making a mat4 tuple reality :)

Nah. Wc shows a strong performance fall in work with several primitives (square/box/pyramid) through 3x3 matrices.

It set the matrix to a rotation matrix that will rotate any vector in counter-clockwise direction around the z-axis, meaning 0,0,1 for the 3rd row

Oh, a short alternative to angle.toRotation(vec3 axis) where axis is vec3(0, 0, 1), right?

GetLocalPlayer added some commits Sep 12, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 12, 2018

Done.
Instead of .toRotation(), .toRotationX(), .toRotationY(), .toRotationZ() were added.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 12, 2018

IDENTITY22 and IDENTITY33 are still indented in old style.
Other than that looking good.

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 13, 2018

Oh, yes. How could I miss it.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 14, 2018

@SamuelMoriarty do you approve the changes?
LGTM now 👍

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 14, 2018

🍨 Thanks @GetLocalPlayer !

@Frotty Frotty merged commit 6e2851e into wurstscript:master Sep 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@GetLocalPlayer GetLocalPlayer deleted the GetLocalPlayer:GetLocalPlayer-matrices branch Sep 15, 2018

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