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

Quaternions package #112

Merged
merged 35 commits into from Sep 15, 2018

Conversation

Projects
None yet
3 participants
@GetLocalPlayer
Copy link
Contributor

GetLocalPlayer commented Aug 31, 2018

Depends on #111

GetLocalPlayer added some commits Aug 29, 2018

@SamuelMoriarty
Copy link
Contributor

SamuelMoriarty left a comment

There are some new natives related to rotating special effects in the new patches.

It would be nice if you could:
a) Establish (through research) the conventions used by these functions, in particular, which argument corresponds to which axis of rotation, because I've seen some contention on Hive about what they actually are.
b) Provide extension functions that directly consume quaternions instead of Euler angles.

Also:
Defining (in the docs) which is the default axis orientation angle for the aforementioned functions is going to be useful as well.

Edit:

This also needs unit tests.


/** Extracts Euler-angles (Tait-Bryan) from a quaternion. Order of result angles is X-Y-Z.
There's no unic way for quaternions thus we have to use quat-matrix conversion first. */
public function quat.toEuler() returns vec3

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

Using vec3 for representing a euler angle is a bit misleading here. While technically the representation is the same, semantically these are very different mathematical concepts, and most operations applied to vectors do not make sense in the context of euler angles.

Unless what you mean to imply here by Euler angle is actually a directional unit-vector... Which afaik is not an Euler angle at all.

A separate tuple for euler angles (and maybe even a separate package with functions to operate on them) would be ideal IMO.

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Aug 31, 2018

Contributor

I just realized you mentioned Tait-Bryan angles in there.
Is there a particular reason for using those instead of classical Euler angles? As far as I know, the typical usage of Euler angles in games is the classical Euler angle, i.e. Pitch, Yaw, Roll, and not Yaw, Pitch, Roll

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Aug 31, 2018

  • Use let if locals aren't modified.
  • Same thing about quat(
  • Add unit tests

@Frotty Frotty reopened this Sep 2, 2018

GetLocalPlayer added some commits Sep 5, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 6, 2018

QuaternionsTests package was added.

  • Use let if locals aren't modified.
  • Same thing about quat(
  • Add unit tests

Done.

GetLocalPlayer added some commits Sep 6, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 6, 2018

.assertEquals function for quaternions was moved into Quaternions package.
.assertEquals functions for vec3 and mat3 were deleted according to dependence on #111
Added functions angle.toQuat and mat3.toQuat. Added test functions according to the new ones.

GetLocalPlayer added some commits Sep 7, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 8, 2018

Requirements have been met. I believe I won't add anything new.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 10, 2018

Good stuff so far, a few points:

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 10, 2018

toEuler

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 10, 2018

Which is one function and it returns vec3 but ok I'll add information in hotdoc.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 10, 2018

yes, and the x,y,z values of that vector are raw angles

GetLocalPlayer added some commits Sep 12, 2018

@SamuelMoriarty

This comment has been minimized.

Copy link
Contributor

SamuelMoriarty commented Sep 12, 2018

This is quality. I like it, only wish there were more things to do with quats in WC3.

Still, you have my kudos, @GetLocalPlayer

I don't think I have any more comments to add.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 12, 2018

Yea, good stuff. I would probably name the File just Quaternion since it doesn't hold multiple datastructures like vecturs/matrices.

GetLocalPlayer added some commits Sep 14, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 14, 2018

Done.
Took thought about adding .exp .log .pow. functions separately but there's no log function in Wc3 (isn't there?) so that was in vain.

GetLocalPlayer added some commits Sep 14, 2018

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 14, 2018

The package has been renamed to Quaternion.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 14, 2018

Took thought about adding .exp

Quaternion.exp(real alpha) returns Quaternion

Doesn't require logarithm, just pow which exists. it just calculates quat^alpha, where alpha is a real.

As for the others, there of course are logarithm libraries in Jass, but you're right, nothing native, so I think exp should suffice. When a PR for log is added, it can always be added later.

@GetLocalPlayer

This comment has been minimized.

Copy link
Contributor Author

GetLocalPlayer commented Sep 14, 2018

there of course are logarithm libraries in Jass

Which may be not the best solution just because Jass is slow. May be it would be better to ask Blizzard to add log native.
Anyway, I don't think there's a case in Wc3 that would require the whole range of quaternion functions.

Doesn't require logarithm, just pow which exists. it just calculates quat^alpha, where alpha is a real.

Yep, that's done. So, is there any requirement left?

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 14, 2018

May be it would be better to ask Blizzard to add log native.

Good luck with that.

Yep, that's done. So, is there any requirement left?

Implementation could be improved I think, but other than that nope.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 14, 2018

Can you push once so that travis will build the branch again?
And resolve conflict pls

GetLocalPlayer added some commits Sep 15, 2018

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 15, 2018

Very nice 🍬

@Frotty Frotty merged commit 1a8fd3c into wurstscript:master Sep 15, 2018

1 check passed

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

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

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Sep 16, 2018

Ok one problem I just noticed now is that you yourself use deprecated functions, causing warnings for every one using stdlib. need to adress that.

@GetLocalPlayer GetLocalPlayer referenced this pull request Sep 16, 2018

Merged

Quaternion warnings #119

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