-
Notifications
You must be signed in to change notification settings - Fork 736
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
Adds rotate vector function #6145
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must conform to coding guidelines (spaces instead of tabs, empty newline at the end).
This would make more sense in CBA. Also name of the function does not imply it is rotating around another vector, there is already |
_normalVector params ["_ux", "_uy", "_uz"]; | ||
|
||
private _rotationMatrix = [ | ||
[cos(_theta) + ((_ux^2) * (1 - cos(_theta))), (_ux * _uy * (1-cos(_theta))) - (_uz * sin(_theta)), (_ux * _uz * (1 - cos(_theta))) + (_uy * sin (_theta))], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are doing cos(_theta)
and sin(_theta)
a lot in here. Saving it into a variable and reusing it would be alot more efficient.
private _vyp = (_vx * ((_rotationMatrix select 1) select 0)) + (_vy * ((_rotationMatrix select 1) select 1)) + (_vz * ((_rotationMatrix select 1) select 2)); | ||
private _vzp = (_vz * ((_rotationMatrix select 2) select 0)) + (_vy * ((_rotationMatrix select 2) select 1)) + (_vz * ((_rotationMatrix select 2) select 2)); | ||
|
||
private _returnVector = [_vxp, _vyp, _vzp]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This serves no purpose.
Cleaning up the code doesn't change the fact that this should go into CBA instead of ACE. |
|
||
private _vxp = (_vx * ((_rotationMatrix select 0) select 0)) + (_vy * ((_rotationMatrix select 0) select 1)) + (_vz * ((_rotationMatrix select 0) select 2)); | ||
private _vyp = (_vx * ((_rotationMatrix select 1) select 0)) + (_vy * ((_rotationMatrix select 1) select 1)) + (_vz * ((_rotationMatrix select 1) select 2)); | ||
private _vzp = (_vz * ((_rotationMatrix select 2) select 0)) + (_vy * ((_rotationMatrix select 2) select 1)) + (_vz * ((_rotationMatrix select 2) select 2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be written as:
private _vxp = _vector1 vectorDotProduct (_rotationMatrix select 0);
private _vyp = _vector1 vectorDotProduct (_rotationMatrix select 1);
private _vzp = _vector1 vectorDotProduct (_rotationMatrix select 2);
I think we should use the Rodrigues Formula, when we translate this into CBA:
|
Remind me if it is mathematically legal to change the order between product and scalar multiplication: (_k vectorMultiply (_k vectorDotProduct _v) vectorMultiply (1 - cos(_theta))) -> (_k vectorMultiply ((_k vectorDotProduct _v) * (1 - cos _theta))) Because that could be slightly faster. |
Here is what I mean:
6 multiplications
4 multiplications |
Yes, you can do it like that. |
Review ready. Whether it belongs in CBA or not is irrelevant. I'd rather know that we have it and where it is. |
When merged this pull request will:
Add a common function to rotate a vector by a given angle using a second vector as an axis of rotation.