-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
new Quaternion(Up, Down) * Up == Up
, but it should be Down
#80249
Comments
What are you basing what it should be on? The other results flipping the vector? |
Based on the description in the documentation, I assumed when an arc on a sphere goes from one side to the other, it should be a 180° rotation. |
For any case where the two vectors are opposite it gives the quaternion |
Again I don't know much about quaternions, but I'd guess flipping either the x-axis or the z-axis would work fine, as long as it's a 180° rotation. |
Yes but how do you pick that? I'm not sure this particular use of this constructor is reliable, but I'm not fully familiar with the system The method for constructing it breaks down when the vectors are parallel, which requires to just pick some value instead |
As far as I can tell there's infinitely many possibilities for the shortest arc between the top and the bottom, so any arbitrary result is fine as long as it's one of the valid results. |
But again, how to decide when this is the case? I think this should be documented as I suspect it's quite complex to try to add tests for this specific case |
I believe if the exterior product ( |
We check it by seeing if the two vectors are parallel and opposite, tested with the scalar product My question is: Is this case degenerate? I'd suspect it is and it's not well defined, and the question is if it is worth it to ensure it gives one of the "valid" cases or just return a degenerate case (being clarified in the documentation of course) Note that it also breaks down in a number of other cases when the input vectors are not along the axes, so that's much more complicated For example, how would you pick the result for the input vector: |
Asking for a 180° rotation seems fairly common for me, so regardless of whether or not it's technically degenerate (which I can't answer), it would seem desirable to me to produce some result that meets the programmer's expectations. Although I can understand that anything to do with quaternions is very difficult and I don't mind this going unresolved for some time. I've already implemented a work-around for my own project. Just to give a use case, for me it was the following code that triggered this edge case: void update_transform() {
var rotation = new Quaternion(Vector3.Up, _normal);
tf = Rotation(rotation) * Translation(new(0, distance, 0));
} Which worked quite well except when |
180 degree rotation around what axis though? |
I think any axis should do, as long as it's the right kind of 180° rotation. Again there's infinitely many right answers in this situation, and as long as it's any of those, it seems to me that it should be okay. |
But how do you find it? |
It seems to me that any axis that is orthogonal to the two given parallel vectors would be okay, so you could use a cross product to find it. |
That is true, the question is if it is worth the several added operations to find this since it is still undefined, there's no such thing as the "shortest arc" in this case |
I thought you said the engine already checks for the case in which the vectors are parallel, in which case it seems to me this isn't really adding any further overhead except in the edge case where the vectors are indeed parallel — and that's the specific case that (in my opinion) currently isn't being handled particularly well. I don't really see any significant performance downside. |
Well you'll have two, possibly three, cross products, and an normalization, which is quite a lot more In total:
|
But it seems to me the added operations would specifically only be in the code branch of the parallel edge case (which doesn't work all that well), and wouldn't affect the normal cases. I might be misunderstanding, though. |
It won't, but that doesn't remove the performance increase here I think there needs to be some argument in favor of this case being a case that isn't degenerate and is worth actually finding a universal fix for It's really in my opinion a question of finding a less wrong answer |
I think taking a little extra time to produce a good result beats getting a wrong result very quickly. Edit: I'll await further opinions, nevertheless thank you for your time, especially since it's a volunteer position. |
Check if input vectors a, b are antiparallel, which is trivial if you check for parallel already. If true create a vector which is guaranteed not to be parallel: Compute rotation axis: Create 180 deg quaternion: |
For me this is clearly a bug. The If this constructor wouldn't be supposed to handle the opposite vectors then it shouldn't handle them in any way whatsoever. In such case documenting such unsupported behavior and erroring in every such case could be a solution. But as mentioned, this constructor already handles such cases, just most of them incorrectly. Hence I think for the opposite vectors the proper thing to do is to make this constructor return a rotation by 180 around any arbitrarily choosen axis orthogonal to the input vectors. And document it clearly; users can always manually handle such case if some specific behavior is desired. Also looking at the code it seems like it assumes the input vectors are normalized (then dot product is equivalent to the cosine of the angle between the vectors), but it's not ensured/checked anyhow. This should be changed too. Lines 146 to 148 in 16a9356
Footnotes
|
Documentation does not say anything about the case of opposite vectors returning unexpected results, and since a valid 180-degree rotation is still possible around an arbitrarily chosen axis, why don't just return that? With a note, that slerp-ing such rotation might yield unexpected results, since the axis is chosen at random. |
That was more or less the consensus so far (minus the note kleonc made about preferring rotation around the Y axis). It's really just that no one has implemented a fix yet. |
Crazy, it can be that simple right? Just use an When I discovered this issue, I only could come up with a general mathematical formula for perpendicular vectors (hint, it's an anti-symmetric matrix with zero trail) but I tried for many hours to find a simple function which would always produce a perpendicular non-zero vector from any non-zero vector. I think, it's impossible with linear or multilinear maps and a kind of case distinction (such as The next night before sleep, suddenly the idea plopped up, that the minimum absolute value of an input vector needs to be mapped to the maximum absolute value in the perpendicular vector. My own idea therefore is to cross-produce with an approximate perpendicular vector that is 1 at the minimum absolute value and zero elsewhere, and then normalize. Technically, the 1 doesn't need to correspond to the minimum component, just to one component that is small enough. This allows for short-circuit evaluation. This idea results in the following replacement code which implements the previous paragraph (sorry, I am too busy to deal with forks and pull requests right now). The arguments are
The helper function better be private. Even better would be throwing a warning [in the Quaternion constructor], if the arguments are not unit vectors. This implementation requires up to 3 additional comparisons and 3 additional calls to |
@krisutofu I took some time this afternoon to implement your code. I should note that my understanding of the inner workings of quaternions is pretty limited but it doesn't seem like anyone else was going to do it. In general it went well except for one assertion I added to the test cases: Vector3 left(-1, 0, 0);
Vector3 right(1, 0, 0);
Quaternion left_to_right(left, right);
// When the two vectors lie on the XZ plane, the rotation axis should be the
// Y-axis for backwards compatibility.
Vector3 left_to_right_axis = left_to_right.get_axis();
CHECK(left_to_right_axis.abs().is_equal_approx(up)); As kleonc mentioned, when the parameter vectors are on the XZ plane, the resulting quaternion should rotate on the XZ plane because that's consistent with what the old code does. I briefly tried messing around with the code but I figured it's probably better to just ask. Could you let me know how I could adjust the code so that it stays backward compatible? Currently @AThousandShips It's very late but I was reading through the thread this morning and I'd like to apologize for the phrasing "since it's a volunteer position and all" because it sounded pretty dismissive when I read it back. I'm not great at social things and my intuition for which tone a text I'm writing has is occasionally pretty bad, but I had always meant this in the sense of "especially since it's a volunteer position." I really didn't mean to be ungrateful. Sorry for that! |
No offence taken, thank you for taking the time to clarify! |
Great 😊 . Sorry for my response latency. It went into email Spam. The change you need is quite simple, it only needs to swap two things. I assumed X -> Y -> Z order but you'd like to have X -> Z -> Y order. Therefore the change is to swap these two lines: || set_perpendicular_to(v0.y, v0.z, v0.x, y, z, x)
|| set_perpendicular_to(v0.z, v0.x, v0.y, z, x, y); to || set_perpendicular_to(v0.z, v0.x, v0.y, z, x, y)
|| set_perpendicular_to(v0.y, v0.z, v0.x, y, z, x); XZ has a positive normal that faces downwards (-Y), given that X precedes Z (as it is implemented in the vector comparison). Therefore it gives a -Y as rotation axis (when v0 is Right and v1 is Left). If you still need that to be Y instead of -Y, you can swap the minus sign in the helper function which then gives the negated cross product. With negated normal, it looks like this: out_y = in_z / length;
out_z = -in_y / length; Thank you for taking the time for the implementation. We will be grateful. EDIT: If it relieves you, it's not just you. My communication and social skills are bad in my experience. It takes me a lot of time to find more precise natural language expressions and sometimes I am overthinking (emotionally and such and this makes me hide). As non-native speaker I may notice my phrasing issues less often. But, we are always seeking the benefit of the community, that's the good thing. |
@krisutofu I thought your reply was plenty fast, there's really no need to apologize. I'm not sure if flipping the axis makes a significant difference, both directions seem to work equally well, but based on the old code I guessed that it used to always give a +Y rotation axis, so I've nevertheless added the change for flipping the axis. Thank you for the code and the kind words! |
Godot version
4.1
System information
Linux 5.15
Issue description
When using the arc-based quaternion constructor very specifically with the arguments
Vector3.Up
&Vector3.Down
, the resulting quaternion is wrong.Steps to reproduce
Sorry that it's in C# instead of GDScript, but I don't have much experience with GDScript.
The text was updated successfully, but these errors were encountered: