zoomx and zoomy are poor controls for the 3D camera

[GoogleCodeExporter]

Using zoomx and zoomy to control the position of the 3D camera has several 
problems:

1) It is not obvious what these numbers refer to.  In general, they must be 
adjusted by trial and error

2) Increasing the zoom number moves the camera further from the view location, 
instead of closer.

3) If axes.daspectAuto is False (as it often is), changing just one of the zoom 
factors will adjust both to the average value.  This means repeating the same 
view command can change the actual view each time.

I would recommend replacing the public interface with a single number that 
increases as the camera moves closer to the view location.  This would fix (2) 
and (3).  This would keep the two zoom factors the same, but it seems to me 
that axes.daspect is a better way to accomplish different magnifications.  What 
I don't have is a nice physical value to use for this number, to solve (1).  
Maybe we'll have to leave it as a somewhat arbitrary number.

Original issue reported on code.google.com by rschr...@gmail.com on 5 May 2011 at 2:41

[GoogleCodeExporter]

Firtsly, I want to note that point 2 and 3 also apply to the 2D camera.

I would have to (partially) disagree on 1.
These numbers refer to the amount of data (in world coordinates) shown in the 
axes. For the 2D camera they therefore make pretty good sense I think. For the 
3D camera it is a bit 'awkward' indeed; it is the amount of data (in world 
coordinates) that the 2D screen shows. So still, if you know the dimensions of 
your data, you can use such a zoom parameter pretty well.

Therefore I think that 2 is not such a bad thing. I think its unextpected 
behavior is compensated for by having a physical meaning.

I agree that point 3 is very bad behavior indeed.

I like the idea of using just one zoom parameter, and use daspect to apply 
different zooming per axis. The more I think of it, the more it actually 
makes sense. For example, currently daspect does not at all correspond with
the actual daspect of the axes if daspectAuto is True.

I think that the cameraclass onMotion() method should change
axes.daspect if daspectAuto is False. The daspect is then indeed automatically
set, as intended! There's one thing to be carefull about though, and that's
to keep the values of daspect sensible. I suggest setting daspect so that
one of its values is 1.0 and the other(s) have a value >= 1.0. For the 3D camera
the x and y values in dasect should be kept equal.

So what we then have is a scalar zoom factor and the daspect. The view_zoom_x 
and view_zoom_y can be calculated from it to set ortho. If "view_zoom_x = 
daspect[0] * zoom", we have a zoom factor that is still reversed, but it does 
represent something: the amount of data to fit on screen. Actually, it would 
represent that value better than it does now, as currently it is relative to 
daspect.

Alternatively, you could replace the multiplication with a devision, but then 
you'd
get very small values in most cases. I also thought about making the zoom 
factor relative to the zooming in the reset state, but that would mean that the 
zoom factor depends on the data insided the scene, which is stupid. Therefore I 
really prefer the multiplication, even though it would mean our zoom factor is 
reversed.

Original comment by almar.klein@gmail.com on 6 May 2011 at 7:50

• Changed state: Started

[GoogleCodeExporter]

Re the 2D camera:
Since the view can be adequately set through axes.SetLimits(), I'm not so 
worried about the behavior of zoomx and zoomy.  (But as you point out, their 
interpretation is obvious.)

As for the 3D camera:
I was about to suggest a scalar zoom and easier controls for daspect, so it's 
good to see you reached the same conclusion.  Another way to handle its 
normalization would be to keep the product of the three daspect values equal to 
1.  Without trying it, I can't say which would feel better in use.  A problem 
with either is that you would need to adjust both the zoom (of the camera) and 
the daspect (of the axes) to do things nice and smoothly.

Perhaps daspect shouldn't be normalized, and the zoom factor should report the 
"size" of daspect.  Here's a quick idea I had:
>>> def zoom(val=None):
...     a = vv.gca()
...     currval = (a.daspect[0]*a.daspect[1]*a.daspect[2])**(1/3.)
...     if val is None:
...         return currval
...     a.daspect = [d * val/currval for d in a.daspect]
I haven't really tested it much, though.

I'm not sure I see why the x and y daspect values should be the same; if you're 
trying to plot different units on these two axes, you'd probably like to be 
able to scale them independently.  If we're worried about mouse control, I 
think we can use the same code as for translation to project the mouse movement 
onto the axes.

If we keep the zoom in the same direction it is now (higher => farther away), 
maybe we should rename it (width, extent, area?)

Original comment by rschr...@gmail.com on 7 May 2011 at 12:07

[GoogleCodeExporter]

If we're going to change this, I think we should change it at the lowest level, 
so also for the 2D camera. In this case too, AFAIK, we can have a single zoom 
factor and the daspect.

> A problem with either is that you would need to adjust both the zoom (of the 
camera) and the daspect (of the axes) to do things nice and smoothly.

Yes, but the daspect is constrained easily (I like your idea too, but note that 
daspect can have negative values, their sign should be preserved), and after 
that the zoom factor would have to be scaled. Should not be too hard.

> Perhaps daspect shouldn't be normalized, and the zoom factor should report 
the "size" of daspect.  Here's a quick idea I had:

Mmm, but then daspect would get its value changed even if daspectAuto is False. 
I don't think that's a good idea.

On a side-note, I would apply Pythagoras' rule instead, since daspect can have 
negative values:
currval = (a.daspect[0]**2 + a.daspect[1]**2 + a.daspect[2])**2)**0.5

> I'm not sure I see why the x and y daspect values should be the same; if 
you're trying to plot different units on these two axes, you'd probably like to 
be able to scale them independently.  If we're worried about mouse control, I 
think we can use the same code as for translation to project the mouse movement 
onto the axes.

That would be better indeed.

Original comment by almar.klein@gmail.com on 7 May 2011 at 8:04

[GoogleCodeExporter]

Not hard, but possibly confusing by mixing axes and camera properties. 
But now that I think about it more, I don't know that you'll need to 
make the zoom readjust itself after changing daspect.  Under mouse 
control, both will need to be adjusted, but that code can be uglier, 
since the user doesn't have to see it.

I guess I really don't understand what daspectAuto is doing, but I'll 
take your word that this isn't good.

Thinking about my suggestion some more, I think it's a bad idea for 
another reason: daspect will scale with the data range.  {daspect = 
[1000000, 1000000]} is less clear than {daspect = [1, 10], zoom = 1000000}.

This would not be all that much different than just using the largest 
value of daspect.  The product treats them more equitably, in that 
doubling any element would produce the same change in the scalar.  (But 
this is moot now.)

I haven't done any coding on this yet, but I'm willing to give it a go.

Robert

Original comment by rschr...@gmail.com on 8 May 2011 at 1:09

[GoogleCodeExporter]

Firstly, I forgot to answer your question about renaming zoom if increasing the 
value gets you further away. I find it hard to make my mind up about this zoom 
issue. I can imagine that the name 'zoom' might be confusing to some people. On 
the other hand the name 'zoom' is by far the best name for what we're trying to 
achieve with this parameter: zooming. Having said that, I suggest making zoom 
1/zoom_what_it_is_now. This way the zoom factor is intuitive and users can 
still easily convert their data-range to a zoom value.

Maybe we should first make clear what daspect, daspectAuto and zoom mean. Or 
what we should make them to mean. The way I see it now:

* daspect represents the data aspect of the scene, the values should typically 
be around 1.0, and the default (for fixed axis) is (1,1,1). If a user wants to 
stretch the y dimension, he will set daspect to (1,2,1). It is also used to 
flip dimensions, for example for images (1,-1,1). 

* daspectAuto can be set to True if there is no direct relation between the 
dimensions of the axises. The limits of the axises can then be set 
independently and zooming can be done per dimension. Currently, it means that 
axes.daspect is not used (only its sign is) for scaling the scene. But I think 
it *should* mean that daspect is automatically set when changing limits and 
doing mouse interaction. If daspectAuto is False, daspect should not be 
changed, except by the user.

* zoom should very simply be a zoom factor. 

> I haven't done any coding on this yet, but I'm willing to give it a go.
Great! If you need more help/discussion, let me know. By the way, the release 
date is not fixed or anything, we'll just release when we've applied and tested 
these changes.

Original comment by almar.klein@gmail.com on 8 May 2011 at 9:19

[GoogleCodeExporter]

I implemented the changes a couple of days ago. I know that you're now playing 
with it so I think its better not to change the code as well. I found that the 
initial limits
are not chosen correctly. I tracked it down and it is due to the fact that 
Reset() is called for ALL cameras, which makes all cameras update the daspect. 

This makes me believe more that daspect should be a propery of the camera after 
all. 

Original comment by almar.klein@gmail.com on 25 May 2011 at 7:51

[GoogleCodeExporter]

This was exactly the issue I was trying to understand.  I just worked 
out what was going on and then checked my email.  I would have saved 
time had I checked email first.

The other solution would be to have the 2 and 3D cameras agree on which 
axis is the reference.  (I would suggest the x-axis.)  But I think I 
agree that it'd be better to have daspect be a camera property rather 
than an axis property.  The camera constantly needs these values; the 
axes rarely do.  This would also make it natural to include the daspect 
with the ViewParams.

Speaking of daspect, I wonder if it makes sense to normalize it in some 
way.  Right now, one element is left at 1 by the built-in code, but the 
user can change that.  daspect.fset could add a normalization step, so 
that daspect = (2,2,2) would result in daspect == (1,1,1) (perhaps with 
the zoom factor adjusted).  Alternatively, daspect for a n-dim camera 
could be a (n-1)tuple, with the final value implicitly taken as 1.

Original comment by rschr...@gmail.com on 26 May 2011 at 4:12

[GoogleCodeExporter]

> This was exactly the issue I was trying to understand.  I just worked 
out what was going on and then checked my email.  I would have saved 
time had I checked email first.

Ah, sorry about that. I did do a couple of quick tests, but the bug did not 
show, because if SetLimits() is called more than once, the scene looks fine.

> The other solution would be to have the 2 and 3D cameras agree on which 
axis is the reference.  (I would suggest the x-axis.) 

But the problem is that the 3D camera sets both the x and y component of 
daspect, which should not happen if the user has a 2D camera.

Let's discuss this daspect thing a bit more ...
If daspectAuto is False, daspect "should" be a propery of the axes, because it 
says something about the scene, independent of the camera. However, if 
daspectAuto is True, daspect becomes a camera property. 

One idea that comes to mind is using a user-defined daspect at the Axes and an 
automatically set daspect on the camera. But that would be too confusing I 
guess.

The thing that bothers me is this: say that I show an image using a 2D camera. 
The y dimension is twice as large, so I set daspect to (1,2,1). Now I switch to 
the 3D camera because I am visualizing lines or something that extends in the 
z-dimension. But now my daspect is (1,1,1) again!  This would not happen often, 
and is not a very serious problem. However, it does make me feel that there 
might be a better solution.

Any ideas?

Original comment by almar.klein@gmail.com on 26 May 2011 at 8:05

[GoogleCodeExporter]

The suggestion here is that the 2D camera should set daspect[1] and the 
3D camera should set daspect[1] and daspect[2], with daspect[0] == 1 for 
both.  Then, they should agree on both daspect[0] and daspect[1].  (But 
if the 2D axes are a rectangle while the 3D axes are a square, maybe 
this doesn't work.  I think this is separately accounted for, though.)

The only way this would work, I think, is if the selection of which to 
use happened entirely behind the scenes, so the user didn't notice.  But 
I think there are better ways to handle the problem.

I don't see this being a big problem, but it would be nice to get right. 
  One idea would be to have axes.cameraType.fset set the daspect of the 
new camera from that of the old one.

My worry is that someone (maybe us, maybe a user) will write code that 
assumes that daspect[i] == 1.  This could be in setting daspect, but it 
could also be setting the zoom factor.  I think my main complaint is 
that I don't like having two ways to adjust the overall zoom.  Right 
now, I can zoom in either by doing zoom=2 or daspect=[2,2,2].

Original comment by rschr...@gmail.com on 26 May 2011 at 3:21

[GoogleCodeExporter]

I like the idea of having x as a reference, I played a bit with this, and think 
it might work.

> I think my main complaint is 
that I don't like having two ways to adjust the overall zoom.  Right 
now, I can zoom in either by doing zoom=2 or daspect=[2,2,2].

Mmm, that is indeed not what we'd want to happen. What about the camera has a 
simple method that produces a normalized version of daspect, which is used in 
SetView. So that really only the ratio between the numbers matters? I also 
tried this, and think it's not that hard, but it requires some changes to the 
zoom code that I could not work out yet.

Original comment by almar.klein@gmail.com on 27 May 2011 at 3:41

[GoogleCodeExporter]

I think we should always apply the correction for the window size. Right now 
its not done when daspectAuto=True. This means that if I set daspect to (1,1,1) 
my data is still scaled with my window. 

This does imply, however, that the daspect should be changed when the window 
size changes. I already implemented this, and it works pretty easy and natural. 
I'm still stuck on the zooming bit though.

I'll commit and push my current changes. Please note that I implemented it only 
halfway, and only for the 3D camera (the 2D should be much easier). If you want 
to play with it, maybe you can get the zooming right. 

Original comment by almar.klein@gmail.com on 27 May 2011 at 11:26

[GoogleCodeExporter]

I've just pushed a few patches to fix some of this.  The changes include:

- Changing the normalization of daspect so that Normalized(daspect[0]) 
== 1.  This makes working out zoom factors easier, since you don't have 
to counteract the change in this value.

- Moving the normalization from a camera method to an axes property.

- Switching the 2D camera over to the current conventions for daspect.

- Removing the short-circuit code in the various Reset methods.  This 
code needs to run before a camera is used, and it's not run by changing 
the camera, so we need to run it at creation.  There may be a better 
method here; this was just a quick fix to get things working again.

What I haven't done is change the 2D camera to include the figure aspect 
ratio in daspect.  I'm a bit worried that doing so will make the 2D and 
3D cameras fight over daspect[1].  Frankly, the current behavior seems 
sensible to me.

Original comment by rschr...@gmail.com on 29 May 2011 at 12:54

[GoogleCodeExporter]

I pushed a couple of changesets.

At first I tried to make it work for daspect being normalized as a vector, 
because I found it more elegant. After a couple of hours of frustration I 
decided that dividing by daspect[0] is probably best. (at least a whole lot 
easier) :)

- Fixed that daspect resizes nicely with the window. 
- Improved the reset method of the 3D camera (better separation of world and 
screen coordinates)
- Made _SetDaspect only change the daspect if that axes has this camera as it 
currents camera. This solves the issue you describe above.

Further I did a bit of refactoring:
- I changed how cameras are initialized and attached to an axes. This makes it 
easier (and less of a hack) for people to connect one camera to multiple axes, 
or to implement their own camera model.
- I made all attributes private.
- I gave the cameras some properties, to enable the user to change the view 
parameters from the camera instance.

I did not yet fix the Get/SetViewParams() method.

Original comment by almar.klein@gmail.com on 31 May 2011 at 11:25

[GoogleCodeExporter]

I've pushed a small change to make the Get/SetViewParams methods to 
work.  Two possible issues:

- The key and attribute names are the same for all but loc/location.  If 
we fix this, it would be easier to switch between the ViewParams and the 
attributes.  Changing the key name would break backwards compatibility, 
so maybe it's better to change the attribute name.  (But we're already 
breaking things in changing zoom.)

- daspect is got and set in axes.Get/SetView() instead of 
camera.Get/SetViewParams().  I did this so that a camera could be used 
on two axes with different daspect.  But right now, the camera assumes 
that all its axes have the same daspect.  This isn't a big problem, but 
it may be something to look at in the future.

Original comment by rschr...@gmail.com on 1 Jun 2011 at 5:21

[GoogleCodeExporter]

> If we fix this, it would be easier to switch between the ViewParams and the 
attributes. 

My thoughts exactly. I renamed 'location' to 'loc'. This is no problem at all, 
because 'location' exists only since yesterday :) Now the viewparams can become 
a list of names instead of key-attr pairs.

The camera uses the daspect of the first camera as its reference, but keeps all 
other axes up to date when auto-scaling. Its probably best to have the same 
behavior for get/setview: GetView() uses the daspect of the first axes, and 
setView sets the daspect of all axes. To make this easier (and more explicit) I 
gave the camera a daspect property that 'links' to camera.axes.daspect. 

The problem with your proposal is that calling axes.SetView() only changes the 
daspect of THAT axes, and not the other ones (which will thus only work as 
expected when that axes is the first axes of the camera). 

Original comment by almar.klein@gmail.com on 1 Jun 2011 at 8:44

[GoogleCodeExporter]

Things seem to be working for me.  I'm a bit worried that daspect is 
being shared a bit too freely between the camera and the axes, 
especially given the many-to-many relationship between the two.  daspect 
is nominally an axes attribute, but it ends up mostly being set by the 
camera.  Setting it on one axes could end up change another, if they 
happen to share a camera.

However, I don't have any examples of where this breaks things, so maybe 
I shouldn't worry so much.

Original comment by rschr...@gmail.com on 2 Jun 2011 at 5:00

[GoogleCodeExporter]

You're right that things get kinda fussy with the daspect parameter when
multiple cameras are used. However, the camera only changes the axes.daspect
if it is its current camera. Also, one has to set the daspect of the
reference axes (the first in the cameras list) in order for the daspect of
other cameras to change, AND daspectAuto should be True.

I think that it legit in this situation for this to happen, although it
might seem a bit like dark magic to the user :)  We should at least document
the behavior well.

Original comment by almar.klein@gmail.com on 5 Jun 2011 at 12:55

[GoogleCodeExporter]

Just pushed a changeset in which I improved some docstrings. Does this fix the 
issue for you?

Original comment by almar.klein@gmail.com on 12 Jun 2011 at 6:02

[GoogleCodeExporter]

Original comment by rschr...@gmail.com on 15 Jun 2011 at 5:14

• Changed state: Fixed

[GoogleCodeExporter]

I did find one problem yesterday (after releasing 1.5).

I had a 2D plot and decided to fly through it. When I switched back to my 2D 
camera the daspect value for y was slightly changed, making the plot a bit more 
zoomed in.

What happens is that when changing from the 2D-camera to any of the other 
cameras, the axes does not need the extra space to draw tickmarks, so the axes 
is resized. The resize operation causes the camera to change the daspect. But 
when resizing back, a different camera does the resizing (the fly camera does 
nothing), so there is a net effect.

Not the worst thing in the world, but I thought I'd document it here.

Original comment by almar.klein@gmail.com on 23 Jun 2011 at 8:06

[GoogleCodeExporter]

Follow-up:

Your worries and the problem in the previous post kept me wondering; maybe the 
daspect should be a camera property after all.

My worries where this:
 - If I change daspect and then switch cameras, I'd have to change daspect again (so I though)
 - the daspect value is by nature a property of the axes (that is, if daspectAuto is False)

As for the first point: axes.daspect.fset() can set the daspect of all cameras. 
Further it can update its axes._daspect value, which is used by all cameras 
when they reset, and more importantly, by all new cameras.

As for the second point: when daspectAuto is True, the daspect is more 
naturally a camera propery, and doing otherwise cause things to be kinda weird. 

By making daspect a property of the camera it acts as follows (if daspectAuto 
if True). Setting axes.daspect sets the daspect of all cameras. The cameras 
will also change the daspect themselves when zooming, resizing and resetting. 
Consequently, changing cameras also results in axes.daspect yielding a 
different value. I think that this is much better than having a consistent 
daspect, at the cost of having the view changed when you switch cameras.

So I implemented this (really tiny change actually), could you check it out, 
Robert? And tell me if this feels better (or worse, in which case we will fall 
back to the previous behavior).
http://code.google.com/p/visvis/source/detail?r=5bbf576eefd43e6094d97adddedf33c7
84a5a3b3

Original comment by almar.klein@gmail.com on 1 Jul 2011 at 11:43

[GoogleCodeExporter]

Just played around with it a little.  One odd thing is if you have 
daspectAuto true and set different daspects for different cameras, and 
then set daspectAuto to False, the cameras keep different daspects until 
daspect is set.  From that point, they then all have the same daspect.

My gut feeling is that making this behavior too complicated is a bad 
idea.  Users need a mental model of what's going on, and this setup 
requires them to have two and switch between them appropriately.  But 
give me a few days to play around with it and see if it grows on me.

Robert

Original comment by rschr...@gmail.com on 2 Jul 2011 at 5:31

[GoogleCodeExporter]

The issue that you describe can be fixed by making daspectAuto.fset() apply the 
user-set daspect value to all cameras. At least it will ensure that if 
daspectAuto is False, the cameras all have the same daspect.

When daspectAuto is True, does it make sense, in your opinion, that the cameras 
have different daspects? Or would you prefer the old behavior in which they 
would influence each-others daspect? 

Original comment by almar.klein@gmail.com on 2 Jul 2011 at 8:16

• Changed state: Started

[GoogleCodeExporter]

After using the current code for some time, my main conclusion is that I don't 
switch camera types enough to notice these sorts of problems one way or the 
other.  My gut feeling is still the same as in comment 21, but if your actual 
experience in switching camera types shows that this isn't a problem, then 
let's not worry about it.

Original comment by rschr...@gmail.com on 25 Jul 2011 at 4:07

[GoogleCodeExporter]

I think you're right. Any issues with this will be very rare indeed. 

Anyway, I applied the idea from comment 23, which should help a bit. For the 
rest I think we now got the most natural solution. Maybe not perfect, but there 
does not seem to be a perfect solution. Any remaining problems are mildly 
irritating at worst. It doesn't crash your hard disk or anything.

Let's close this issue!

Original comment by almar.klein@gmail.com on 25 Jul 2011 at 9:39

• Changed state: Fixed