Orthographic projection for mplot3d by txxia · Pull Request #8326 · matplotlib/matplotlib

txxia · 2017-03-18T05:00:27Z

Fixes #537

implemented ortho_transformation() in proj3d
added a flag persp and a setter set_persp()

Here is a simple demo:

import matplotlib.pyplot as plt
from mpl_toolkits.mplot3d import proj3d
fig = plt.figure()
def init_axes(ax):
    ax.set_aspect("equal")
    ax.set_xlabel('x')
    ax.set_ylabel('y')
    ax.set_zlabel('z')
ax = fig.add_subplot(121, projection='3d', persp=False)
init_axes(ax)
ax = fig.add_subplot(122, projection='3d', persp=True)
init_axes(ax)
plt.show()

dstansby

Neat little new feature! Just a couple of implementation changes I've suggested.

dstansby · 2017-03-19T16:42:04Z

          *elev*             Elevation viewing angle (default 30)
          *zscale*           [%(scale)s]
          *sharez*           Other axes to share z-limits with
+          *persp*            Perspective projection (default True)


I think it would be better to have a proj argument instead, which (at the moment) could take either 'persp or ortho values, and default to persp.

I agree. In that case, should I provide the setter set_proj? There is a get_proj which has different meaning.

Maybe use projection instead of proj to avoid breaking get_proj?

I think a better alternative would be to rename the current get_proj to get_proj_matrix - might be worth seeing what other devs think about that.

dstansby · 2017-03-19T16:44:51Z

+    fig = plt.figure()
+    ax = fig.gca(projection='3d')
+    ax.set_persp(False)
+    ax.set_xlabel('X')


This test doesn't really need any text, so could the label lines be removed.

- renamed persp to proj - renamed get_proj to get_proj_matrix - add test for ortho_projection

txxia · 2017-03-20T00:01:18Z

I am not too sure about this. The name projection conflicts with the flag in add_subplot (see above demo), but renaming get_proj may break users' code if they happen to be using it.

txxia · 2017-03-20T00:08:58Z

    R = np.array([100, 100, 100])
    V = np.array([0, 0, 1])
    viewM = proj3d.view_transformation(E, R, V)
-    perspM = proj3d.persp_transformation(100, -100)


I am not sure about this either. This seems to assume zfront and zback refer to the coordinates on the camera's Z axis (which is backwards), but judging from other places where it's called, they seem to be equivalent to near / far plane.
https://github.com/matplotlib/matplotlib/blob/master/lib/mpl_toolkits/mplot3d/axes3d.py#L1001

it is entirely possible this test was originally backwards. It came from some commented out code at the bottom of proj3d.py. Seemed like it might have been some debugging code for the original developer. We recently lifted it out of there and put it in as a unit test because it helped us solve a problem.

Ah, thanks for the clarification. I will leave it as is then.

WeatherGod · 2017-03-29T15:09:05Z

it is unfortunate name choice, but get_proj() has been a part of the public API for a very long time now. I do know of code in the wild that uses it. Please do not change that.

WeatherGod · 2017-03-29T15:10:40Z

-    def get_proj(self):
+    def set_proj(self, proj):
+        """
+        Set the type of projection.


Need to expand on this; detail what are the accepted inputs and what they mean.

Do you think it will be okay if I rename the flag and this setter to proj_type? since I will keep get_proj, using set_proj would cause confusion.

Yes, I think your new setter/getter can be proj_type. I am a little leery of using the word "type" in a variable name when one is not referring to a type object, but I think it is ok here, especially if it is documented well.

WeatherGod · 2017-03-29T15:16:01Z

    R = np.array([100, 100, 100])
    V = np.array([0, 0, 1])
    viewM = proj3d.view_transformation(E, R, V)
-    perspM = proj3d.persp_transformation(100, -100)


it is entirely possible this test was originally backwards. It came from some commented out code at the bottom of proj3d.py. Seemed like it might have been some debugging code for the original developer. We recently lifted it out of there and put it in as a unit test because it helped us solve a problem.

WeatherGod · 2017-03-29T15:28:55Z

+                     [0,2,0,0],
+                     [0,0,-2,0],
+                     [0,0,a,b]
+                     ])


Can you provide a reference for this formulation? I am not an expert in 3d transforms, but this doesn't match anything I am seeing on the wikipedia page for orthographic projection.

Sorry, I should have justified it.
This matrix is almost equivalent to the one in the Wikipedia page. Based on persp_transformation, I made these assumptions:

zfront is the value for near, left and bottom

zback for far, right, top

the X and Y translations are omitted (I guess so the image will always be centered at the origin)

It's a simple derivation from the one from Wikipedia (v1), but since v2 and v1 are equivalent in homogeneous coordinates, I thought the second will look nicer.
$$$ \vec{v_1} = \begin{bmatrix} \frac{2}{b-f} & 0 & 0 & -\frac{b+f}{b-f} \\ 0 & \frac{2}{b-f} & 0 & -\frac{b+f}{b-f} \\ 0 & 0 & \frac{-2}{b-f} & -\frac{b+f}{b-f} \\ 0 & 0 & 0 & 1 \end{bmatrix} \begin{bmatrix} x \\ y\\ z\\ 1 \end{bmatrix} = \begin{bmatrix} \frac{2}{b-f}x - \frac{b+f}{b-f}\\ \frac{2}{b-f}f - \frac{b+f}{b-f}\\ -\frac{2}{b-f}z - \frac{b+f}{b-f}\\ 1 \end{bmatrix} \\ \vec{v_2} = \begin{bmatrix} 2 & 0 & 0 & -b-f \\ 0 & 2 & 0 & -b-f \\ 0 & 0 & -2 & -b-f \\ 0 & 0 & 0 & b-f \end{bmatrix} \begin{bmatrix} x \\ y \\ z \\ 1 \end{bmatrix} = \begin{bmatrix} 2x-b-f\\ 2y-b-f\\ -2z-b-f\\ b-f\\ \end{bmatrix} = (b-f)\vec{v_1} $$ \\ M_{final} = \begin{bmatrix} 2 & 0 & 0 & 0 \\ 0 & 2 & 0 & 0 \\ 0 & 0 & -2 & -b-f \\ 0 & 0 & 0 & b-f \end{bmatrix}$

ah, and if I transpose that, it then matches up. Got it. Well, just include some notes about it in comments for some future poor soul.

Why is this the transpose?

Because numpy matrix is in row major order?

twmr · 2017-04-01T21:43:01Z

            self.azim = azim

-    def set_proj(self, proj):
+    def set_proj_type(self, proj):


why didn't you rename the argument name as well?

Thanks for pointing out, I just renamed the argument.

WeatherGod · 2017-04-03T16:10:52Z

This looks great. The only thing left to do is to add an entry to the what's new page: There should be some notes in doc/users/whats_new/

txxia · 2017-04-04T19:57:32Z

I am new to numpydoc. The file I created seems to compile, but I had trouble generating everything under doc/ (it gave KeyError: b'tcsi1728' at some point). Should I worry about that?

WeatherGod · 2017-04-04T20:22:40Z

we build the docs as part of our test suite. If it builds there, that should be sufficient.

WeatherGod · 2017-04-05T14:34:16Z

It would be nice to get more documentation for this feature, especially prose and graphics showing when this feature makes a significant improvement. However, that can be done in a separate PR.

WeatherGod · 2017-04-07T15:11:38Z

@dstansby , I take it that your concerns have been addressed?

dstansby · 2017-04-09T08:19:32Z

Sorry, this one dropped off my radar, looks like everything I mentioned has been fixed so feel free to dismiss my review. On 7 Apr 2017 4:11 pm, "Benjamin Root" <[email protected]> wrote: @dstansby <https://github.com/dstansby> , I take it that your concerns have been addressed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8326 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6RfH9KrjeDvP4Bw0nKEBxdhjyJWwnWks5rtlIwgaJpZM4MhS7y> .

WeatherGod · 2017-04-10T15:39:18Z

Thanks to everybody!

implemented orthographic projection

fa7784c

txxia force-pushed the fix-537 branch from 9e543ba to fa7784c Compare March 18, 2017 05:07

dstansby added the topic: mplot3d label Mar 18, 2017

dstansby added this to the 2.1 (next point release) milestone Mar 18, 2017

dstansby requested changes Mar 19, 2017

View reviewed changes

ortho projection fixes

579d1e4

- renamed persp to proj - renamed get_proj to get_proj_matrix - add test for ortho_projection

txxia commented Mar 20, 2017

View reviewed changes

WeatherGod requested changes Mar 29, 2017

View reviewed changes

txxia added 2 commits April 1, 2017 00:13

Revert changes to the existing test

acbad48

rename flag to proj_type, update tests

6025b0d

twmr reviewed Apr 1, 2017

View reviewed changes

rename parameter in the setter

21a8085

txxia added 2 commits April 3, 2017 22:13

added what's new file

51d8018

include 'axes3d' in the file name

cc425f6

WeatherGod approved these changes Apr 5, 2017

View reviewed changes

WeatherGod merged commit 6f4cc13 into matplotlib:master Apr 10, 2017

WeatherGod mentioned this pull request Jul 19, 2017

mplot3d projection results in non-orthogonal axes #8894

Closed

Conversation

txxia commented Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstansby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

txxia commented Mar 20, 2017

Uh oh!

txxia Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeatherGod commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

txxia Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeatherGod commented Apr 3, 2017

Uh oh!

txxia commented Apr 4, 2017

Uh oh!

WeatherGod commented Apr 4, 2017

Uh oh!

WeatherGod commented Apr 5, 2017

Uh oh!

WeatherGod commented Apr 7, 2017

Uh oh!

dstansby commented Apr 9, 2017 via email

Uh oh!

WeatherGod commented Apr 10, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

txxia commented Mar 18, 2017 •

edited

Loading

txxia Mar 20, 2017 •

edited

Loading

txxia Mar 29, 2017 •

edited

Loading