Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Volume rendering & camera changes #612

Merged
merged 27 commits into from Mar 25, 2015
Merged

Conversation

@almarklein
Copy link
Member

@almarklein almarklein commented Nov 8, 2014

This PR implements volume rendering. It's basically a port of the technique that I use in Visvis.

  • New Volume visual with two rendering styles (iso and mip).
  • Volume example in examples/scene/volume.py Try it, and use the keys to switch cameras etc.
  • Refactored cameras a bit in general
  • Refactored TurnTableCamera: no more mode and distance. You can just change the fov while getting the same zoom level. Setting fov to 0 means orthographic. Zooming is done by moving camera backward.
  • New FlyCamera to fly around your data.

This is about ready. There are more things to do, but that can be picked up in another PR. (I added todo items for this in volume.py)

I wish to do more with the camera's, but we should probably discuss this first. Will make a new issue for this.

todo:

  • avoid warning when swithching render styles
  • camera's

closes issue #725

volume

Closes #725

@larsoner
Copy link
Member

@larsoner larsoner commented Nov 8, 2014

What is the relationship to the existing WIP volume rendering PR? Does it
supersede it, or is it complementary?

@almarklein
Copy link
Member Author

@almarklein almarklein commented Nov 8, 2014

It's another technique. And I believe that @lcampagn had yet another technique in mind. So I guess they can be complimentary. We probably have to make a list of advantages / disadvantages of the different techniques to see whether we need all of them.

@almarklein almarklein mentioned this pull request Nov 8, 2014
nodes = transforms._node_path(self, transforms.viewbox.scene)
T = [n.transform.inverse for n in nodes]
view_tr = transforms._transform_cache.get(T)
self._program.vert['viewtransform'] = view_tr

This comment has been minimized.

@almarklein

almarklein Nov 8, 2014
Author Member

@lcampagn this is the code to get the transform from camera to scene, required to calculate the view direction vector in the vertex shader.

This comment has been minimized.

@campagnola

campagnola Nov 8, 2014
Member

It's not valid because it depends on scenegraph; this kind of behavior is only allowed for VisualNode subclasses.
You could try using the method described in my visual tutorial 5, but perhaps we should wait until that PR is settled..

This comment has been minimized.

@almarklein

almarklein Nov 9, 2014
Author Member

Sure, this needs to be implemented in a better way. One idea is to let the transformsystem object have a "cameratransform" (or something with a better name). On the event object this transform will be implemented in the way that I did it here.

@@ -286,6 +286,7 @@ def node_transform(self, map_to=None, map_from=None):

# starting node must come _last_ in the transform chain
fwd_path = fwd_path[::-1]
# todo: AK: must we also not reverse order in rev_path?

This comment has been minimized.

@campagnola

campagnola Nov 8, 2014
Member

No; I think rev_path is already in the correct direction because it begins with map_to. See above:

rev_path = self._node_path(map_to, map_from)

Perhaps you could change this to be less ambiguous:

rev_path = self._node_path(start=map_to, end=map_from)
@@ -78,7 +78,7 @@ def transforms(self):
def Linear(self):
b = True
for tr in self._transforms:
b &= tr.Linear
b &= bool(tr.Linear)

This comment has been minimized.

@campagnola

campagnola Nov 8, 2014
Member

Why was this necessary? tr.Linear should always be boolean.

This comment has been minimized.

@almarklein

almarklein Nov 9, 2014
Author Member

I ran into a situation where it was None

This comment has been minimized.

@campagnola

campagnola Nov 9, 2014
Member

It should never be None. This should be fixed upstream instead..
Your example runs even if I remove bool(, can you remember what that situation was?

This comment has been minimized.

@almarklein

almarklein Nov 11, 2014
Author Member

I can't remember. But will revert it back and fix it in a better way when the issue does resurface.

t0, t1 = 0, 1

# So we draw the six planes of the cube (well not a cube,
# a 3d rectangle thingy). The inside is only rendered if the

This comment has been minimized.

@campagnola

campagnola Nov 8, 2014
Member

"cuboid"

from vispy import app, scene

import numpy as np
import imageio

This comment has been minimized.

@campagnola

campagnola Nov 9, 2014
Member

Don't forget to remove this.
I think we already have volumetric data in the demo-data repository..

This comment has been minimized.

@larsoner

larsoner Nov 9, 2014
Member

Indeed:

https://github.com/vispy/demo-data/blob/master/brain/mri.npz

And it's selfishly an example I want to see look nice :)

self._vtf = view_tr_f = transforms._transform_cache.get(Tf)
self._vti = view_tr_i = transforms._transform_cache.get(Ti)
self._program.vert['viewtransformf'] = view_tr_f
self._program.vert['viewtransformi'] = view_tr_i

This comment has been minimized.

@almarklein

almarklein Nov 11, 2014
Author Member

Note @lcampagn: I actually need both the forward and inverse transform in order to make it work for perspective devision (since the ray is different for each vertex).

This comment has been minimized.

@campagnola

campagnola Nov 12, 2014
Member

That's pretty common. You can simplify it a bit:

Tf = [n.transform for n in nodes]
self._vtf = view_tr_f = transforms._transform_cache.get(Tf)
self._vti = view_tr_i = view_tr_f.inverse

But also note that PerspectiveTransform.inverse is broken in master; it does not undo the perspective division. (did we already discuss that? I forget.)

This comment has been minimized.

@campagnola

campagnola Nov 12, 2014
Member

Actually, I think you can simplify it a lot:

view_tr_f = transforms.visual_to_document
view_tr_i = view_tr_f.inverse

Shouldn't that work?

This comment has been minimized.

@almarklein

almarklein Nov 13, 2014
Author Member

But also note that PerspectiveTransform.inverse is broken in master; it does not undo the perspective division. (did we already discuss that? I forget.)

I added code for the inverse perspective transform, which seems to work.

Shouldn't that work?

OMG, yes it does seem to work. Here am I, making it hard on myself while I could just have used transforms.visual_to_document :/

view_tr_i = view_tr_f.inverse

Would it not be better get this from the cache as well, since the inverse will now have to be calculated on each draw?

This comment has been minimized.

@almarklein

almarklein Nov 13, 2014
Author Member

OMG, yes it does seem to work.

Although, I suppose it would break down if between the document and the volume there is another viewbox, which has a transform other than translate-scale only. Can we have a transforms.visual_to_nearest_viewbox?

This comment has been minimized.

@campagnola

campagnola Nov 14, 2014
Member

Would it not be better get this from the cache as well, since the inverse will now have to be calculated on each draw?

Every transform instance has exactly one inverse transform instance, so no caching is necessary. This also means that if you modify the original transform, then the inverse is automatically updated as well.

Although, I suppose it would break down if between the document and the volume there is another viewbox, which has a transform other than translate-scale only.

In such cases, it will be necessary for the user to set visual.document = ... to select the appropriate coordinate system. These will be very rare and advanced cases like the SVG-editor example.

@almarklein
Copy link
Member Author

@almarklein almarklein commented Nov 11, 2014

Changed the data to the MRI data that is in our demo-data directory. Will also upload the stent volume soon. So you guys can check the render if you want. Note that this is still WIP though ...

One thing that I want to address is improving our camera models and implement a camera that you can fly around ala flight-sim. The current render technique is set up to allow cameras inside the volume, which would be really awesome if we could make it work correctly...

@almarklein almarklein force-pushed the almarklein:volume branch 2 times, most recently from fb1ec2a to c49d48c Nov 20, 2014
@almarklein
Copy link
Member Author

@almarklein almarklein commented Nov 26, 2014

This is finally ready for review. I invite you to try out the new volume example.

  • Be sure to press "1" to select the fly-camera, and then WASD + mouse to fly through the volume.
  • Press "i" and "m" to toggle between isosurface and mip render styles.
  • With the turntable camera, use CTRL+RMB to smoothly change the fov from orthographic to perspective.
@almarklein
Copy link
Member Author

@almarklein almarklein commented Nov 26, 2014

cc @izaid

@@ -10,6 +10,7 @@
from ..visuals import Visual


# todo: I though the visuals were mixed, but the base Node was *not* a visual?
class Node(Visual):

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

@lcampagn I though the visuals were mixed, but the base Node was not a visual?

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

This was just easier. We can separate Node / NodeVisual later if necessary.

@campagnola
Copy link
Member

@campagnola campagnola commented Nov 26, 2014

Looks great!

Artifact--looks like the texture is wrapping around?
anticipation1

@@ -15,7 +15,7 @@

fname = load_data_file('orig/triceratops.obj.gz')

canvas = vp.mesh(fname=fname, azimuth=-0, elevation=90, distance=2)
canvas = vp.mesh(fname=fname, azimuth=-0, elevation=90)

This comment has been minimized.

@@ -16,7 +16,7 @@
# Create a canvas with a 3D viewport
canvas = scene.SceneCanvas(keys='interactive')
view = canvas.central_widget.add_view()
view.set_camera('turntable', mode='perspective', up='z', distance=50,
view.set_camera('turntable', fov=60, up='z',
azimuth=30., elevation=30.)

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

This also?

@@ -32,8 +32,7 @@
parent=canvas.scene)
vb2.parent = canvas.scene
vb2.clip_method = 'viewport'
vb2.set_camera('turntable', mode='ortho', elevation=30, azimuth=30, up='y',
distance=10)
vb2.set_camera('turntable', elevation=30, azimuth=30, up='y')
#vb2.set_camera('turntable', mode='perspective',

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

And this..

@almarklein almarklein mentioned this pull request Nov 26, 2014
7 of 7 tasks complete
@almarklein
Copy link
Member Author

@almarklein almarklein commented Nov 26, 2014

@lcampagn the Turntable camera has changed, and does no longer have distance and mode properties.


# todo: user should do this via parenting. we should connect to re-parenting
# event and then call _find_viewbox.
# @viewbox.setter

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

This (was) automatically called by ViewBox.. why should this be done with parenting instead?

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

Because it would be nice if a user can just grab a camera and place it in a new scene: cam.parent = another_viewbox.scene and it would be able adjust itself.

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

But why is that better than viewbox.set_camera(...) or whatever we have already?

This comment has been minimized.

@almarklein

almarklein Nov 27, 2014
Author Member

Ah, I think what you were trying to do is allow cameras with no parents to be used. But this would allow a camera to be a child of one scene, while it uses in another (to which it is not a child), which is weird. We should probably discuss this further in #647 when we discuss allowing multiple cameras-multiple scenes.

return 0

@property
def scale_factor(self):

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

What is the intended use for this?

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

See #647. The idea is to provide a scale factor that each camera can use to get a notion of the scale. For most cameras this is analogue to zooming. For the fly camera this gives an indication of the intended size of the scene, used to determine speed of the camera.

self._scale_factor = float(zoom)

# todo: set_limits, set_range, or set_bounds, or other?
def set_range(self, xlim, ylim, zlim=None):

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

Confused about this.. what is the intended use? What coordinate system are the limits specified in? How should the limits be interpreted in the case of perspective cameras?

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

These limits are in the coordinates of the scene graph. This is the way of telling the camera "this is the region you need to visualize". See also #647

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

I guess you mean coordinates of the viewbox.scene ? I agree we should have a method for making a particular part of the scene visible, but I will need to be convinced about the best API for that..

There is a very tricky problem here in that the cameras themselves only have ~10 degrees of freedom (x,y,z,pitch,yaw,roll,xfov,yfof,near,far), but we might want to expose many more than 10 parameters for specifying the state of the camera (all the "natural" parameters I listed previously, plus scale_factor, center_loc, view limits, etc). We run into a problem where we want the user to be able to provide any combination of those parameters that fully specify the camera state, while not allowing them to overspecify it. This is a very common user interface problem--for example, SLR cameras have many more controls than degrees of freedom, and they solve this by giving the user different "modes" to work in: auto, AV, TV, M, P, etc., where each mode exposes only a subset of those degrees to the user at a time.

The set of parameters I picked for the cameras are pretty straightforward / natural, and I think we need to keep those available for users who want to think about physically placing their camera in the scene. At the same time, I like the approach you have taken (the user doesn't care about the camera so long as they can see their data), so we need to figure out how to combine these approaches. It might be best for now to keep each of these as separate camera classes, and we can try to join them together later on if that seems possible.

This comment has been minimized.

@almarklein

almarklein Nov 27, 2014
Author Member

Note that view limits are not part of the state of the camera. The idea is that you can set the limits once, and then when the camera is reset(), it uses these limits to set its parameters (i.e. state) to a reasonable initial view.

I think the state of the current cameras can all be boiled down to: center, scale_factor, fov, + rotations. (e.g. I don't think we need to incorporate speed/acceleration of the fly camera.)

@@ -282,6 +387,8 @@ def invert_y(self, inv):
self._update_transform()

def view_resize_event(self, event):
if self.viewbox.camera is not self:

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

This should not happen--doesn't the camera know when it is not being used?

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

Ha! It does happen when you have multiple camera's in your scene. Only the one that is currenlty active should respond to events.

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

In the current system, if I set view.set_camera(...) then that becomes the active camera, and no other cameras should respond to events coming from the view. Maybe that wasn't implemented correctly, but that's what the disconnect() method above is for.

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

Yes, but with the current system it was difficult to switch between camera's because it was not trivial to activate the camera again. Or that's how I remember it. Maybe there is a way to use the connect / disconnect in a better way so that we don't need this if's

This comment has been minimized.

@almarklein

almarklein Nov 27, 2014
Author Member

Fixed this: the viewbox connects/disconnects the camera on switching.

width : float
Width.
Field of view. Default 60.0 (which corresponds more or less
with the human eye).

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

I'm pretty sure we have better than 60 deg FOV :)

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

mmm, maybe needs adjusting. The value, or the docstring.

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

Just the docstring. The value is fine :)

elevation : float
Elevation in degrees.
azimuth : float
Azimuth in degrees.
distance : float

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

I'm probably going to want these back..

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

No you don't :) The beauty of the current system is that there is no need to specify distance. The scale_factor provides the means to indicate how "zoomed in" we are. From there, we put the camera at a distance (depending on fov) such that this zoom-level is maintained.

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

Unless you want a more "regular" perspective camera that flies around your center point, for which modifying fov does give an impression of zooming. But I suspect in most cases the new system is more useful.

I like how the fov does not affect the zoom level (at least not at the center), such that the ortographic and perspective "modes" give equal zoom level. I took these ideas from visvis, where Robert Scholl implemented this. Credit goes to him.

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

The beauty of the current system is that there is no need to specify distance.

And the problem with it is that there is no way to specify the distance. The scale factor might indeed be useful, but it should not come at the expense of making the cameras magical black boxes. I also suspect that the absence of a truly orthographic transform will be problematic in some situations due to fp issues.

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

Why would you want to specify the distance? If you need this much control, you probably need a PerspectiveCamera.

With fov==0, this is a truly orthographic system. The minimum fov higher than 0 is I think 0.01, for which the camera is indeed placed a great deal away from the scene. But for fov==0, the projection transform is a true orthographic one.

This comment has been minimized.

@almarklein

almarklein Nov 27, 2014
Author Member

We could perhaps create a PerspectiveTurntableCamera, which is more like a normal Perspective camera, with a given distance from the center.

""" Select camera and attach it to this ViewBox
If a camera of the given type already exists in the scene, that
camera is used. Otherwise it is create.

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

I don't think we should try to manage cameras for the user--set_camera("cam_type") should just create a new camera, and if the user wants to re-use cameras, they can do something like view.camera = my_camera.

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

The this function should be renamed make_camera. I think that we could certainly use a method/property to easily switch between types of cameras. e.g. switching between trackball and turntable. Maybe this:

viewbox.camera = camera_object
viewbox.camera = 'trackball'  # creates a trackball camera, because it does not yet exist
viewbox.camera = 'turntable'
viewbox.camera = 'trackball'  # resets the camera that was creates to lines up

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

I dunno.. it seems this makes some strong assumptions about the ways people will want to use the cameras in their applications. It's likely users will bring their own expectations about how the user interface should work, and we don't really save them much effort by doing this:

trackball = TrackballCamera(fov=60, up='z')
turntable = TurntableCamera(fov=90, azimuth=45)
viewbox.camera = trackball

Isn't it better to let them decide how many cameras they want, and how to refer to them? For example, it seems more likely that when switching between different camera types, they will want to preserve as closely as possible the current viewpoint, rather than just switching back to the last viewpoint used by a particular camera.

This comment has been minimized.

@almarklein

almarklein Nov 27, 2014
Author Member

I am thinking more of a high-level approach, where people just want to remember the name of a camera, not the class:

>>> from vispy import plot
>>> plot.mesh(my_mesh)
>>> view = plot.current_viewbox_howeve_we_expose_this()
>>> view.camera = 'trackball'

The viewpoint is inherently different between different types of camera's and sometimes is impossible to reproduce; I don't think we need to try to preserve it on camera-switching.

This comment has been minimized.

@campagnola

campagnola Nov 27, 2014
Member

Agreed we shouldn't try to preserve the camera viewpoint; let the user worry about that. I'm saying there's no need to preserve the cameras at all; let the user worry about that. The current view.set_camera("trackball", ...) is plenty convenient enough.

p = p / p.w;
//p.z = 0;
p.w = 1;
p = p / max(p.w, 0.0000001);

This comment has been minimized.

@campagnola

campagnola Nov 26, 2014
Member

What happened here? If we had p.w == 0, that suggests a different problem upstream..

This comment has been minimized.

@almarklein

almarklein Nov 26, 2014
Author Member

I don't know. But I got stuff moved to infinity without this fix. I also experienced this when I visualized the Image visual with the (old) turntable camera. It happened when looking very much up/down, and also in other horizontal directions.

It actually took me a while to find this was the problem; I spend quite some time inside the volume rendering looking for the cause of the resulting artifacts :/

But yeah, if you know how to prevent w from becoming zero, that would be even better.

@almarklein
Copy link
Member Author

@almarklein almarklein commented Nov 27, 2014

Updated: addressed some comments. Also fixed the artifact: that was related to a bug in gloo causing the texture wrapping in z-direction not to be set. I think the most "controversial" changes are related to the cameras. I propose we discuss what to do with that in #647. I can implement a new kind of turntable camera that is closer to the original version, if you want.

almarklein added 7 commits Feb 6, 2015
* aspect_ratio -> scale_ratio
* aspect_fixed -> fixed_ratio
* set_range() can be called before association with a viewbox
* set_range queries scene bounds more efficiently
* Line visual handles bounds better
* And some small details ...
@almarklein almarklein force-pushed the almarklein:volume branch from 77f6231 to fde101f Mar 10, 2015
@almarklein
Copy link
Member Author

@almarklein almarklein commented Mar 10, 2015

@campagnola Thanks for the review. Addressed comments. Travis is green (except that I managed to misspell "size skip").

@coveralls
Copy link

@coveralls coveralls commented Mar 10, 2015

Coverage Status

Coverage decreased (-1.83%) to 77.02% when pulling fde101f on almarklein:volume into 0444b69 on vispy:master.

@coveralls
Copy link

@coveralls coveralls commented Mar 10, 2015

Coverage Status

Coverage decreased (-1.83%) to 77.02% when pulling 196129c on almarklein:volume into 0444b69 on vispy:master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Mar 10, 2015

Coverage Status

Coverage decreased (-1.83%) to 77.02% when pulling 196129c on almarklein:volume into 0444b69 on vispy:master.

@almarklein almarklein changed the title WIP: Volume rendering WIP: Volume rendering & camera changes Mar 24, 2015
@coveralls
Copy link

@coveralls coveralls commented Mar 25, 2015

Coverage Status

Coverage decreased (-1.65%) to 77.14% when pulling 45b8335 on almarklein:volume into cce5011 on vispy:master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Mar 25, 2015

Coverage Status

Coverage decreased (-1.65%) to 77.14% when pulling 45b8335 on almarklein:volume into cce5011 on vispy:master.

almarklein added a commit that referenced this pull request Mar 25, 2015
WIP: Volume rendering & camera changes
@almarklein almarklein merged commit 3b19080 into vispy:master Mar 25, 2015
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
@almarklein almarklein deleted the almarklein:volume branch Mar 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.