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

Animated view transforms R and T #54

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

gphysics
Copy link
Contributor

Try this. Pressing T merely rotates view to top. Pressing R rotates view to top, and centers and zooms on bounding box for selected geometries. View transforms animate in 10 frames and the incremental draw is not performed until the final frame. Will look at snapping view positions next. Currently left click and mouse move rotates. And with ctrl+ it moves cursor instead. Do we still need this functionality or can we use the ctrl modifier while rotating to perform the snapping? Not sure it makes sense to use a letter + rotate ...

@c42f
Copy link
Owner

c42f commented Jun 14, 2015

Oh man, this is super smooth! Will have a close look later.

@gphysics
Copy link
Contributor Author

A little less jarring ...

@gphysics
Copy link
Contributor Author

I think this is all done now. Key A aligned view position and up vector to nearest axes and key Z resets zoom to bound selected geometries. View changes and focus point reselection now smoothly animated.

@nigels-com
Copy link
Collaborator

That's slick indeed.

I couldn't resist tinkering with it (#56)

@gphysics
Copy link
Contributor Author

Glad you like it. I guess its OK to change to a time based animated transform. Displaz appears to adjust the amount that is drawn each render in order to preserve some minimum frame rate (and hence responsiveness) - so perhaps it doesn't really matter.

@c42f
Copy link
Owner

c42f commented Jun 15, 2015

I think it probably makes sense to use a time-based animation. Nigel's patch also seems to fix some problems which occur when I click around very quickly (before the previous animation has finished). We should definitely ensure robustness to the user doing "stupid things" like clicking during an animation or pressing camera keyboard shortcuts during a camera animation.

@c42f
Copy link
Owner

c42f commented Jun 15, 2015

Usability testing (using Nigel's branch): I played with this for a couple of hours using medium sized datasets (~30e6 points). From a raw usability viewpoint I think there's still a few things to do here.

  • I'm finding some minor motion sickness, possibly due to having the target frame rate too low. There's also the matter of shimmering between frames which has always slightly bothered me, but is particularly noticeable with everything animated. I managed to somewhat alleviate things by using a timeout of 0ms in m_incrementalFrameTimer->start() (originally set to 10ms since it seemed to improve responsiveness - not something I really understood at the time, and this may have gone away due to other changes), and by setting the target frame time down to 20ms. On the downside, a higher frame rate means that some point clouds will need more than one frame to draw points close to the camera, which will probably make shimmering worse. Some people (not generally myself) are very sensitive to this kind of thing so we probably need a user option to disable camera animations, or even better set the animation duration. Then I'll really have to get around to implementing Make displaz remember settings between runs #1!
  • Camera animations should probably have a fixed time limit (not quite sure), but small adjustments could arguably be done more quickly than this upper bound. It should be fairly easy to figure out the magnitude of the adjustment and choose a time accordingly.
  • I like the cubic animation curve for camera parameters. Very slick.
  • The zoom speed when zooming by large factors doesn't look even. I think this is because zooming tends to look unnatural unless you use exponential distance scaling. At a guess I'd say the right thing for animating the camera radius is probably something like
    pow(m_animatedViewTransformStartCamera.eyeToCenterDistance(), 1-xsi) * pow(m_animatedViewTransformEndCamera.eyeToCenterDistance(), xsi)
    Actually I just tested this now and it's definitely the right thing.
  • There's already some limited support for changing the camera from the command line (eg, run displaz -viewangles 10 10 10. I guess this should be hooked up to the same camera animation machinery.
  • Pressing a and then dragging the mouse causes unpleasant "fighting" between the user and the animation. Ditto for zooming etc.
  • I'm impressed that a seems to work for all orientations, even when upside down in trackball mode.
  • At some stage it's likely that displaz will have a "first person mode". I guess this means the wasd keys need to be reserved to keep every FPS player happy...
  • For geospatial work, it's basically true that there's only one blessed orientation, so I thought it was quite useful that you had a way to get directly to "map view" in an earlier commit.

src/view3d.cpp Outdated
m_camera.setCenter( (1 - xsi)*m_animatedViewTransformStartCamera.center()
+ xsi *m_animatedViewTransformEndCamera.center());
m_camera.setRotation( (1 - xsi)*m_animatedViewTransformStartCamera.rotation()
+ xsi *m_animatedViewTransformEndCamera.rotation());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use proper spherical interpolation with QQuaternion::slerp() as a matter of general principle. (Not really noticeable unless you're doing large rotations, but may as well.)

@c42f
Copy link
Owner

c42f commented Jun 15, 2015

General comment on the code: I suspect the camera animation details really belong in a separate object, not in View3D which is already getting a bit bloated. Seems like we have at least two choices:

  • Make the camera class itself do the animations.
  • Make a new class specifically to drive camera animations. This might be cleaner, since I feel like the camera class already has a few things which could arguably be split out. (Eg, the mouse interaction model is arguably separate from the camera transformation itself.) I suppose such a CameraAnimator class would include the timers, animation curves, etc

* Increase frame rate in an attempt to keep things smooth and
  non nausea-inducing for larger data sets.  (Or should we do this only
  during an animation?)

* Improve interpolation curves to use slerp() for angles and exponential
  scaling for radial distance.

* Ensure quaternions passed to InteractiveCamera are always normalized
@c42f
Copy link
Owner

c42f commented Jun 15, 2015

I've done a few of the usability hacks mentioned above on top of Nigel's branch, and pushed it to my axis_alignment branch (see 8661752).

@c42f
Copy link
Owner

c42f commented Jun 16, 2015

Cool thanks a lot, this is definitely improved. In terms of key shortcuts, I'd like to keep the number of distinct keys used small if possible. How about R for "rotate to map view", and SHIFT+R as "rotate to nearest axis"? I'm not sure whether Z should be a separate shortcut, or maybe R should be "rotate to map view and zoom to selected data". [Edit: I probably need to think more about this. I'll experiment myself.]. I do like having Z zoom to the extent of the selected data, I think this will be extremely useful.

In the current version, the camera rotation animation (with R) still fights the mouse if the user tries to rotate during the animation.

@c42f
Copy link
Owner

c42f commented Jun 16, 2015

Extra thought: double clicking in the "data sets" window centers on the given data set. We should connect this to the camera animation as well.

@c42f
Copy link
Owner

c42f commented Jun 16, 2015

Extra extra thought: If we have animation in the camera itself (or on a CameraAnimator class as suggested above) that makes it easy to ensure that all repositioning of the camera other than direct click+drag goes through the same system, with consistent animations applied.

@gphysics
Copy link
Contributor Author

Thought I had solved the fighting - will have another look. Ok with you if I work on building a camera animation class?

@c42f
Copy link
Owner

c42f commented Jun 16, 2015

Sure, go for it. Then we can rip most of the new logic here out of view3d.cpp.

By the way, I'm a bit inconsistent about file name capitalization. I'm trying to clean up my act: I've started putting new classes - which are nontrivial enough to go into their own file - into source files which are named exactly the same as the class itself. Hence CameraAnimator.h in this case I guess (or think of a more appropriate name.)

@gphysics
Copy link
Contributor Author

Didn't create a new class CameraAnimator - I felt it was more logical that the animation code was part of interactivecamera class. The setCenter and setEyeToCenterDistance slots can probably be removed as this data is passed in through m_camera.beginAnimateViewTransform(...) The setRotation slot is only used once in mainWindow.cpp - do we really need this call? Presently still using slots in view3d to toggle camera modes. Should we connect these directly to the camera in mainwindow.cpp? I think this would be neater.

@gphysics
Copy link
Contributor Author

Connected signals from main window menu items (trackball and animation) directly to interactivecamera slots. Also did a bit of tidying and name simplifying.

@nigels-com
Copy link
Collaborator

I'd suggest not driving the animation with a timer. If there are other redisplay events in play, the animation won't be smooth. It's the redraw itself that ought to be applying the animation based on the current timestamp. I played with putting View3D into a "continuously redraw" mode (AKA idle func) but I wound up with interactions between incremental drawing, mouse interaction and applying animations. I guess I can give it another try when I next book into Linux.

@c42f
Copy link
Owner

c42f commented Jun 19, 2015

Yes, it makes sense to drive the animation with a timer, but use a timestamp to compute the value of the animation curve parameter.

It may be time to consider taking control of the main Qt event loop ourselves, though I'm not sure it provides any real benefit in this case over a QTimer with zero timeout. It's possible to hook into the event system with various levels of control through QAbstractEventDispatcher etc.

On another note, I think parameter modifications I did to the target frame rate made it smoother, but definitely increased shimmering. I'll have to look at that again before this is merged.

@nigels-com
Copy link
Collaborator

Oh, the shimmering was due to the target frame rate change? I think there is a bit of a problem if the rate of mouse events (screen and mouse DPI dependent!) resemble the redraw rate. On my machine, the shimmering was pretty severe. I was thinking of disabling incremental rendering while a mouse button is held, but that didn't seem to resolve the problem entirely.

@c42f
Copy link
Owner

c42f commented Jun 19, 2015

I think there's a few issues which cause shimmering:

  • Shimmering due to incremental rendering is exacerbated by
    • Aliasing of points smaller than a single pixel, (MSAA helps a lot here, but has relatively high cost so I have it disabled. Uncomment the QGLFormat stuff at the bottom of main.cpp to enable it. Could be a use option I guess.)
    • Z fighting due to poor depth buffer precision (Dynamic clipping planes #52)
  • Dynamic draw quality adjustment. The code continuously tries to decide how much geometry to draw per frame. This must be continuously enabled because the user can arbitrarily change the shaders and geometry goes into and out of view as the user rotates or zooms. The downside is that any noise in the frame timing measurement will cause a different amount of geometry to be drawn for the next frame, so points pop into and out of view. (The frame timing is measured per frame by calling glFinish(); I think it would be much better to get an average time over several frames, but this is somewhat tricky in an event driven render loop. I've had two tries now at getting the draw cost model really solid, but it's still not really right.

@gphysics
Copy link
Contributor Author

Does the problem of shimmering need to moved to a separate issue? I think it is independent of the animated view transforms - when I keep the mouse button down and rotate I get some shimmering, but there are no animated transforms happening during the rotation process. If you are happy with the view alignment functionality and animated transform code then perhaps these should be pulled in and a separate issue raised for shimmering.

@c42f
Copy link
Owner

c42f commented Jun 20, 2015

Agreed that it's a separate issue to solve the shimmering problem. However, I'd like to avoid making it worse.

@gphysics
Copy link
Contributor Author

I'm working on it now - give me an hour and I might have a new commit.

@gphysics
Copy link
Contributor Author

Using state machine for draw process with incremental timer (now called m_drawTimer) running continuously. Have made it so during mouse rotates only the first frame is drawn each time. Now I get no shimmering. Let me know how it goes for you. I found the shimmering seems to be caused by incremental draws during mouse rotate.

@c42f
Copy link
Owner

c42f commented Jun 20, 2015

Only had time for a quick look. I'm still seeing some issues

  • pressing and holding the rotate button, but not moving the mouse keeps the redraw going. This produces ugly shimmering because the draw quality adjustment happens each frame and the frame time measurement is noisy.
  • I think 20 ms is too slow for the redraw interval. For trivial geometry we should be able to achieve a much higher frame rate.
  • animations still fight manual mouse movement, but the way you've refactored things into InteractiveCamera should make this easy to fix now.

@gphysics
Copy link
Contributor Author

There must be something different about your system. For me, holding the r, t or z buttons causes transformation once and then it finishes the draw and then all quiet. Also r, t, z do not fight mouse movement. However z and mouse wheel do fight.

@c42f
Copy link
Owner

c42f commented Jul 2, 2015

Sorry for the silence Greg, I definitely intend to get back to this. The reason I haven't merged it yet is that it's a rather big change to the feel of the user interface, and I really want to get it right.

@nigels-com
Copy link
Collaborator

I gave this a quick spin on Windows 7 with Intel graphics. Unfortunately there is a noticable flickering at startup, before any data set has been loaded. I'll take a closer look to see if it's something easily fixed.

@c42f c42f mentioned this pull request Nov 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants