Three.js: Matrix4.makePerspective() is never used

[looeee]

I'm updating the Matrix4 docs and there's a method called Matrix4.makePerspective( fov, aspect, near, far )

Internally this calculates the values of left, right, top and bottom and calls
Matrix4.makeFrustum( left, right, bottom, top, near, far )
and it looks like it is intended for use in PerspectiveCamera.updateProjectionMatrix()

However the PerspectiveCamera calculates its own versions of left, right, top and bottom in a slightly more complex way, taking into account zoom, view (if set) and filmOffset, and then calls Matrix4.makeFrustum directly.

So it seems that Matrix4.makePerspective() is superfluous - is there a use case that I am missing?

[looeee]

Actually there is one place that it's used, although I'm not entirely sure of its purpose there:
https://threejs.org/examples/#webgl_materials_cubemap_dynamic2

[mrdoob]

Nice catch! I don't think that's needed on that example.

[mrdoob]

And yes, I think we could remove Matrix4's .makePerspective() if we're not using it.
/cc @WestLangley

[mrdoob]

We can also try to move the code from PerspectiveCamera to Matrix4.

[Mugen87]

Is it okay if i refactor webgl_materials_cubemap_dynamic2? I've seen some more little things to change (e.g. Geometry-> BufferGeometry)

[Mugen87]

@looeee The dependency in webgl_materials_cubemap_dynamic2 to .makePerspective() is now resolved.

[Mugen87]

Because OrthographicCamera calculates its left, right, top, bottom values in updateProjectionMatrix, PerspectiveCamera should also do this.

[Mugen87]

Besides, parameters like fov or aspect are camera related and should not be present in Matrix4. IMHO, i think we can remove makePerspective().

[looeee]

@Mugen87 thanks! Looks good.

[mrdoob]

Besides, parameters like fov or aspect are camera related and should not be present in Matrix4. IMHO, i think we can remove makePerspective().

What do you think @WestLangley?

[WestLangley]

We have Matrix4 methods to create a rotation matrix, a translation matrix, a scale matrix, a shear matrix, etc., so why not have methods to create an orthographic projection matrix and a perspective projection matrix?

Maybe makeFrustum could be renamed MakePerspective. (Thing is, the order of the args is different than those in MakeOrthographic... That's unfortunate.)

makePerspective, then, could be removed. It is just a different way of parameterizing the args. I am not sure if users rely on it or not.

This is sort of a mess, but I do not have a strong opinion on this one.

[Mugen87]

I just wanted to highlight that i'm not a fan of the current method signature of makePerspective (so fov, aspect etc.). But renaming makeFrustum to makePerspective is a good idea 👍

[mrdoob]

I just wanted to highlight that i'm not a fan of the current method signature of makePerspective (so fov, aspect etc.). But renaming makeFrustum to makePerspective is a good idea 👍

This is our chance then 👍

[looeee]

Closed via #10375

[bhouston]

BTW:

(1) Matrix4makePerspective was intended to be an implementaiton of venerable gluPerspective function from OpenGL here: https://www.opengl.org/sdk/docs/man2/xhtml/gluPerspective.xml

(2) Matrix4.makeFrustum was intended to be an implementaiton of the standard OpenGL function glFrustum: https://www.opengl.org/sdk/docs/man2/xhtml/glFrustum.xml

I am okay with removing makePerspective but you shouldn't then rename makeFrustum to makePerspective and change makePerspective's signature.

You are just messing with non-standard terminology and also breaking a bunch of code - like mine. I am getting warnings for what is clearly incorrect behavior. I would have preferred that makePerspective just was completely removed rather than essentially breaking it.

I would obsolete the old makePerspective if you feel strongly about it, but this renaming is confusing as we were following OpenGL terminology and now we are not.

[WestLangley]

I was afraid of that...

Thing is, we support orthographic cameras, so Matrix4.makeFrustum() is not clear. Orthographic cameras have a frustum, too.

As I said above, I do not feel that strongly about this ... but thinking outside the box to suggest solutions, how about removing all the methods and making new ones with new names?

Matrix4.makePerspectiveProjection( left, right, top, bottom, near, far )
Matrix4.makeOrthographicProjection( left, right, top, bottom, near, far )

Now we are not confusing names with OpenGL nomenclature -- which is also good because the order of our args is different from OpenGL.

[bhouston]

There is also in OpenGL glOrtho: https://www.opengl.org/sdk/docs/man2/xhtml/glOrtho.xml

Maybe something like makePerspective() (which we could obsolete, I am okay with that), makePerspectiveFrustum(), makeOrthographicFrustum()? I think the Frustum is a good term for specifying all of the frustum bounds.

[WestLangley]

makePerspectiveFrustum(), makeOrthographicFrustum()?

We are not making a frustum, though. We are making a projection matrix...

This is such a mess because in the orthographic case, the args are the frustum parameters; in the perspective case, the args in our case are (I think) the dimensions of the front face of the frustum's near plane (plus the near/far distances) -- instead of the more-natural fov/aspect parameters.

As implemented with the left/right/top/bottom params, I can't imagine a user using the perspective method at all. How is the user supposed to know how to compute the args to pass into the function? As an internal method, it is fine.

[bhouston]

As implemented with the left/right/top/bottom params, I can't imagine a user using the perspective method at all. How is the user supposed to know how to compute the args to pass into the function? As an internal method, it is fine.

What you are arguing for then is going back to the way it was. :) gluPerspective was intended to be a user friendly means of setting up perspective matrices. :)

[WestLangley]

What you are arguing for then is going back to the way it was. :)

Hehe. I don't like any of the solutions, quite frankly.

How about restoring what we had before, and adding

Matrix4.makePerspectiveProjection( fov, aspect, near, far )
Matrix4.makeOrthographicProjection( left, right, top, bottom, near, far )

Then, we can remove duplicate functionality in a later release.

BTW, according to the docs

glFrustum — _multiply_ the current matrix by a perspective matrix

[bhouston]

How about restoring what we had before, and adding
Matrix4.makePerspectiveProjection( fov, aspect, near, far )
Matrix4.makeOrthographicProjection( left, right, top, bottom, near, far )

So we would have 3 functions then?

makePerspectiveProjection, makeOrthographicProjection, makeFrustum?

With regards to naming, both makeFrustum and makePerspectiveProjection are both perspective projection creation functions, just with different parameterizations.

Maybe we should use something like makePerspectiveFromFOV and makePerspectiveFromNearPlane? This way both creators explicitly state in their function names their specification. (This is similar to makeRotationFromQuaternion and makeRotationFromEuler.)

And we can keep makeOrthographic without any "From" specifier because no disambiguation is required?

glFrustum — multiply the current matrix by a perspective matrix

All gl matrix functions multiple the current matrix. E.g.: https://www.opengl.org/sdk/docs/man2/xhtml/glTranslate.xml https://www.opengl.org/sdk/docs/man2/xhtml/glScale.xml https://www.opengl.org/sdk/docs/man2/xhtml/glRotate.xml

[WestLangley]

An attempt to summarize @bhouston's suggestion...

r.83 makeOrthographic( left, right, top, bottom, near, far ) remains unchanged

r.83 makePerspective( fov, aspect, near, far ) renamed to makePerspectiveFromFOV( fov, aspect, near, far )

Add makePerspectiveFromNearPlane( left, right, top, bottom, near, far ) - same as makeFrustum() with args in a different order

r.83 makeFrustum( left, right, bottom, top, near, far ) add to Legacy deprecated list; calls the new method makePerspectiveFromNearPlane()

[bhouston]

Add makePerspectiveFromNearPlane( left, right, top, bottom, near, far ) - same as makeFrustum() with args in a different order

The reason why the top/bottom parameters are ordered the way they are in glFrustum/glOrtho is because in OpenGL's normalized device coordinate system, top is positive y, bottom is negative y. Thus the glFrustum/glOrtho parameter order of (left, right, bottom, top) is actually: (xmin, xmax, ymin, ymax) - which makes sense.

I suspect that @looeee and @Mugen87 didn't know this is OpenGL's NDC and mistakenly believed that WebGL/OpenGL follows the convention of screenspace in Windows and Linux desktops in which y axis is facing downwards. Thus they have inverted the parameters mistakenly.

This is OpenGL's NDC:

[WestLangley]

I believe OpenGL's NDC space is left-handed, so the image above is not correct.

Thus they have inverted the parameters mistakenly.

I believe makeOrthographic() and makeFrustum() have had their top/bottom args in a different order for many years. I may have recently suggested they be made to have identical signatures.

[bhouston]

+Y is up though in both NDC and the after projection space.

There is a difference between NDC and the coordinate space after projection (which is really the one we want to be dealing with here.) In NDC, +z is into the screen. The space after projection has -z into the screen - that difference between NDC and after projection is evidence on the page you linked. Particularly this image:

http://www.songho.ca/opengl/files/gl_projectionmatrix01.png
http://www.songho.ca/opengl/files/gl_projectionmatrix02.png

[looeee]

I suspect that @looeee and @Mugen87 didn't know this is OpenGL's NDC...

I can't speak for @Mugen87 but personally I had no idea these functions had OpenGL equivalents. Perhaps going forward, whatever final signature we end up with, comments could be added to the code linking to the related OpenGL functions to prevent this kind of confusion in the future?