-
Notifications
You must be signed in to change notification settings - Fork 150
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 edge case check to vectRotate3D #1373
Conversation
Intercepts an error if 0 is entered as a theta value, and just returns the vector unchanged.
[[1,2,3], [1,0,0], 0] call CBA_fnc_vectRotate3D // 0 There is no error here. And I don't see why it would error, as there are no divisions that could be by zero. Also, not to be petty, but the fuck was that change 3 years ago: The author says your name, but I remember quite well translating the algorith to SQF myself. |
Close? |
There's something odd going on that I can't seem to readily reproduce in the abstract, and I'm not sure what exactly is happening when it does happen, figured this would potentially have solved it.
Which wasn't before this (acemod/ACE3#6145) or this (07174a7). You did contribute in the threads to be sure, but a bunch of others did too, I just suggested and wrote the thing as it basically became in the CBA pr. |
Ok, maybe I am getting old. Just post the code that raises the error, so it can be reproduced. |
Just a whole block (without context or some of the variables I assure you are present) that seems to be one example of a repeat offender-
|
Add some debugging |
@commy2 |
Give some numbers to reproduce. |
copied from the RPT file, Format is V1, V2, and V1
|
??? toFixed 20;
private _vec1 = [0.310837,0.944462,-0.106643];
private _vec2 = [0.310628,0.944532,-0.106627];
[_vec1, _vec2, 0] call CBA_fnc_vectRotate3D returns Maybe use toFixed 20; at the top of the script to keep more digits on the floats. |
Apparently it's a super-tiny issue more with their vector dot product stuff, than anything here, I guess.
|
toFixed 20;
private _vec1 = [0.27024456858634949000,0.95774263143539429000,-0.09847404062747955300];
private _vec2 = [0.26996955275535583000,0.95781064033508301000,-0.09856573492288589500];
private _theta = 1.00000011920928960000;
[_vec1, _vec2, _theta] call CBA_fnc_vectRotate3D
Also, now theta is != 0. That means the current PR doesn't fix it? |
That's my guess, since this is coming from an issue with the vanilla vector dot product generating values over 1 from unit vectors, since |
Can you just give me the code to reproduce please? I don't get it to error out with your numbers. |
|
toFixed 20;
_vec1 = [0.27024456858634949000,0.95774263143539429000,-0.09847404062747955300];
_vec2 = [0.26996955275535583000,0.95781064033508301000,-0.09856573492288589500];
_crossVector = (vectorNormalized _vec1) vectorCrossProduct (vectorNormalized _vec2);
_theta = acos(_vec1 vectorDotProduct _vec2);
_theta reports
|
As I said, I think I can blame BI for this one, rather than the script, though the 'if > 0' seems to catch this weirdness anyhow. |
Sure, however
|
toFixed 20;
_vec1 = [0.27024456858634949000,0.95774263143539429000,-0.09847404062747955300];
_vec2 = [0.26996955275535583000,0.95781064033508301000,-0.09856573492288589500];
_crossVector = (vectorNormalized _vec1) vectorCrossProduct (vectorNormalized _vec2);
_theta = acos ((_vec1 vectorDotProduct _vec2) min 1);
[_vec1, _crossVector, _theta] call CBA_fnc_vectRotate3D; |
Also, pretty sure you should use |
If doing something a bit much like
isn't right out, I'll change it to that. Otherwise probably could close the PR.
I mean, that will still throw the same error for these values, but point noted. |
Huh? |
It returns the vector when |
So it's just: if (_theta isEqualType 0 && {_theta == 0}) Basically what you want is for the function to type check. |
Yes, had been unaware of the |
Would have to test that, but pretty sure The thing is, I see no justification why returning the original vector is necessarily correct when _theta is NaN. Why not any other rotated vector? |
If I'm understanding your question, my response is that if you rotate a vector by zero degrees, you're not rotating it at all, and it returns the same vector as you started with, after the rotation operation. Similarly, if you're trying to rotate a vector by anything but a number... you're not rotating it at all, which is the same effect as rotating it zero degrees, which leaves you with the same vector you started with, after having attempted your rotation matrix operation. So the outcome being the same, the case of the returned value should also be the same. |
The only thing I could see is it raising an error via params ["_vector", "_rotationAxis", ["_theta", 0, [0]]]; But it would make the function slightly slower. And inconsistent with the other math functions that trust input. |
You're the code curator. If you don't want having any check like that to be the case, you can close the PR without my objection. |
I'd rather BI fixes |
Intercepts an error if 0 is entered as a theta value, and just returns the vector unchanged.
When merged this pull request will:
theta
is zero.theta
is zero.