| Summary: | Quaternion patch | ||
|---|---|---|---|
| Product: | [JogAmp] Jogl | Reporter: | Petr Skramovsky <petr.skramovsky> |
| Component: | opengl | Assignee: | Sven Gothel <sgothel> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | harvey.harrison, xerxes |
| Priority: | --- | ||
| Version: | 2 | ||
| Hardware: | All | ||
| OS: | all | ||
| Type: | --- | SCM Refs: |
jogl f5fe8439f52acaf4bc2afe04531f1aa8ca30390b
jogl eaf15f8020bdd97f7d919721c9cc74eb72e6c493
jogl 0a4defdfe478c3e231f3b1255cba6989aa2c489e
jogl 08bfa5797069cb5757620e74b8befaa079d04ddb
jogl a368548b20321ea5cdace6cc495e632ca9d5c99c
jogl 85b96a891bb81776faf0a6dd1783443a045f74fb
|
| Workaround: | --- | ||
| Attachments: |
patch
demo |
||
Created attachment 470 [details]
demo
.zip containing a very simple quaternion camera class + demo program
Thank you Peter for pushing this Quaternion enhancement patch as 4 individual commits to github: https://github.com/petr-s/jogl/commit/ba8f322de06204afe4c7cb81e51a1d46477bb586 - new method for vector multiplication, new copy constructor https://github.com/petr-s/jogl/commit/1fe8fa0765f13980cd4498cb65ab2e36908e75b1 - ixed isIdentity method, deprecated isEmpty method (quaternion doesn't have such a property + method do same thing as isIdentity), new setIdentity method, default constructor sets this quaternion to identity, new fromAxis method/costructor https://github.com/petr-s/jogl/commit/0f9e158d89d2971d505373409e4ab56699e66eea - removed unnecessary castings, removed unnecessary methods for vector operations changed to VectorUtil instead https://github.com/petr-s/jogl/commit/81c911541690f573b631974212f0e49d4d83daba - reformatted to same style Petr,
Your patches look fine, I had some trivial comments that I made in your github repo, reproduced below.
+ public void fromAxis(float[] vector, float angle) {
+ float sin = FloatUtil.sin(angle *= 0.5f);
Please don't hide the assignment in the argument here, would appreciate a temp
var.
Maybe:
double halfangle = angle * 0.5f;
And then use it below as well.
Also, I'd prefer (but not require) a real name in the committer/author field 'petrs' is not so helpful.
In any event, looks pretty good, will recommend pulling with the above fixed.
Harvey thank you for your review and suggestions, the latest revision fixes the issue and i set my user.name for jogl repo as well. Thanks, looks good to me, I wouldn't mind seeing the fixup folded back into the other commit, but it's not a huge deal. Would you mind terribly fixing up your commit name in the other commits? I'm not sure how Sven feels about it, so don't feel it is a must-do. From a code standpoint, I feel this is pullable. Harvey Thank you - merged. I also took the liberty to add qualifier 'final' to Quaternion and VectorUtil. |
Created attachment 469 [details] patch i would like to submit this patch, because i have tried to use quaternion class for my camera but i wasn't able to do it. so sum of changes: - reformatted - method for init from axis vector and angle - method for transform vector - few minor things what i'm not sure of: - naming - maybe more/better comments