vicviper's picture

OpenTK.Vector3.TransformPerspective code is wrong

Project:The Open Toolkit library
Version:1.1-2014-01-02
Component:Code
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed
Description

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

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
the Fiddler's picture

#1

Status:open» fixed

Thanks, fixed in r2939.

c2woody's picture

#2

The implicit conversion mentioned is still there though:

Quote:

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)?

the Fiddler's picture

#3

There is no implicit or explicit conversion from Vector3 to Vector4. You have to use one of two constructors:

public Vector4(Vector3 v)
public Vector4(Vector3 v, float w)

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.

c2woody's picture

#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.

vicviper's picture

#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:

[Obsolete("This constructor has been considered ambiguous, please, use Vector4(Vector3 v, float w) instead.")]
public Vector4(Vector3 v)

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!

the Fiddler's picture

#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.

c2woody's picture

#7

Quote:

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.

the Fiddler's picture

#8

Done, thanks.

the Fiddler's picture

#9

Version:1.0.0-rc1» 1.1-2014-01-02
Status:fixed» closed

Closing bugs fixed in OpenTK 1.1.

If this is still an issue please file a new bug report at https://github.com/opentk/opentk/issues