OpenTK.Vector3.TransformPerspective code is wrong
Posted Wednesday, 2 June, 2010 - 11:24 by vicviper| Project: | The Open Toolkit library |
| Version: | 1.0.0-rc1 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | fixed |
Jump to:
I've been trying to use Vector3.TransformPerspective with an orthographic projection matrix, and after getting wrong results, I found a bug in the code.
Current code is:
public static void TransformPerspective(ref Vector3 vec, ref Matrix4 mat, out Vector3 result) { Vector4 v = new Vector4(vec); Vector4.Transform(ref v, ref mat, out v); result.X = v.X / v.W; result.Y = v.Y / v.W; result.Z = v.Z / v.W; }
this code works with perspective projection matrices only, but not with orthographic projection matrices because the W of vector v is 0 instead of 1
to solve the problem:
public static void TransformPerspective(ref Vector3 vec, ref Matrix4 mat, out Vector3 result) { Vector4 v = new Vector4( vec, 1.0 ); // notice here, W is 1; Vector4.Transform(ref v, ref mat, out v); result.X = v.X / v.W; result.Y = v.Y / v.W; result.Z = v.Z / v.W; }
This also happens with Vector3d.
And, as a personal opinion, implicit vector3 to vector4 constructors should be disallowed, because the results greatly depend on W being 0 or 1, and there's no "standard" that says that W should be set to "0" or to "1".... in fact, from the maths point of view, an implicit conversion from vector3 to vector4 should automatically set W to 1, not to 0
I would preffer to leave only a contructor like this: Vector4(vector3 v3, float w);
Comments
#1
Thanks, fixed in r2939.
#2
The implicit conversion mentioned is still there though:
And, as a personal opinion, implicit vector3 to vector4 constructors should be disallowed, because the results greatly depend on W being 0 or 1, and there's no "standard" that says that W should be set to "0" or to "1".... in fact, from the maths point of view, an implicit conversion from vector3 to vector4 should automatically set W to 1, not to 0
I'm also in favor of removing such conversions because of this ambiguity, usually x,y,z->x,y,z,1 makes much more sense than ->x,y,z,0. Any reason to stick to the latter/not disabling the conversions (explicit/implicit)?
#3
There is no implicit or explicit conversion from Vector3 to Vector4. You have to use one of two constructors:
Removing the first would break backwards compatibility, which should not be done without a really good reason. I do not think this specific case qualifies.
#4
You are right, I was talking about the public Vector4(Vector3 v) constructor which is usually basis of the implicit operator.
Indeed it would break backwards compatibility but at least it should be documented what it's really doing, for the usual matrix transformations a Vector4 is used in the OpenGL context the w==0 is certainly not what you'd want/expect.
The XNA framework doesn't have such a constructor so it might even be best to deprecate the one-parametric Vector2 and Vector3 constructors of Vector4. People can check their code for correctness then and use the respective value for w.
#5
Hi, glad to see this one's finally fixed, I was using a placeholder call waiting for this one, which looked extremely ugly in the code.
About removing
public Vector4(Vector3 v)constructor, I understand that other projects might already be using it and would certainly break compatibility.I would suggest using the obsolete attribute, and keep it there for as much time as needed:
that way, people will get some warnings when using that particular constructor.
Btw, can't check the code now, it's been fixed also for Vector4d < Vector3d ?
Thanks!
#6
Yes, this is fixed on Vector3d, too.
The math library needs a revamp (+ unit tests), but the way forward is not yet clear. Which is why I avoid breaking compatibility: better make one clean break, if necessary, than several small ones.
Edit: I've updated the documentation to reflect that the w component is initialized to 0.
#7
Which is why I avoid breaking compatibility: better make one clean break, if necessary, than several small ones.
Agreed. Thanks for updating the documentation accordingly, you may want to write (Vector3 instead of Vector3d, float instead of double) though in the Vector4 case.
#8
Done, thanks.