Add Fly camera + fix gimbal locking issue

[AlexRamallo]

I decided to do some work on the viewport cameras. The main changes here are:

• A 'Fly' mode for camera rig (activated by right-click), which acts like a typical first-person camera. This mode seamlessly preserves the orbit distance, similar to how omniverse does it.
  • Control via WASD/Arrows, elevation with Q/E or PageUp/Down
  • Increase/decrease speed with mouse wheel
  • Double/halve speed with left shift/control
• A toggleable mouse locking feature, which keeps the mouse locked to a viewport while manipulating the camera
• Changes to the transform functions in CameraRig to get rid of gimbal locking issue

I exposed the camera fly speed and mouse locking prefs via ViewportSettings/ImagingSettings. Not sure if it's the best way to do that. Storing the camera speed in user prefs probably isn't very useful, but currently it doesn't seem like per-viewport settings get saved anywhere.

The fly mode controls are implemented in CameraRig.cpp using ImGui APIs, and so I ended up having to pull in the Gui.h header. This is probably not the best place to put it. It might be better to move that into CameraManipulator.cpp, and pass the inputs as member variables of CameraRig?

Also, I noticed that the viewport cameras are initialized to be looking 'down'. If you load up the kitchen scene, the camera is on top and looking down at the roof, instead of looking into the kitchen as you'd expect. I wasn't sure why this was, so I didn't change it. The new orbit code correctly initializes at that same angle, but I can change it if desired.

[cpichard]

Hi Alex,
Awesome, thanks for this addition !
I had all those features in mind so looking forward to merge your PR.
I quickly tried your changes this morning, I really enjoyed the mouse lock on viewport this avoids the pumping when the viewports are small. Although, even if the mouse disappears, it seems like it is still processed by imgui and hovered widgets will light up if the hidden mouse is over them. I am not sure if this is easy to fix or not, but it’s not a huge deal anyway compared to the benefit of this feature.
The gimbal lock is gone, thanks for that !
The flyby works well and I am looking forward to have this ability in the viewport when playing with 3d scans :)
The only issue I see now is the camera randomly “jumping” when I start moving it, I am not sure why it does that yet and couldn’t really reproduce the definite steps to get to this issue. Do you see the same problem on your machine ?

Exposing the camera fly speed and mouse locking prefs via ViewportSettings/ImagingSettings is the way to go. I think the mouse locking can be the new behaviour, replacing the older one, as I don’t see why we would use the current behaviour over the locked mouse. So may be removing the option to select it and make it default is better

The code interacting with the mouse should ideally go in the camera manipulator as you figured it out. But the camera rig Move function only deals with mouse positions deltas while your code needs to deal with key pressed. I had in mind originally to have several camera manipulators: one for the “arc ball, turntable, etc” and one for the fly and walk navigation, adding the ability to choose between them via a button, a bit like how you would do it in blender. In that case the new camera manipulator could call another Move function with "movements", up down, forward, back etc. What do you think ? Having the right mouse click to enable this camera manipulator is also cool.

The camera pointing downwards is due to the fact that usd can have the space oriented differently, with Z or Y as up axis and UT is not alway handling it correctly. There is an option is the stage to specify the orientation and before loading the stage we don't know which one it will be. It’s on my list to deal with the different situations happening with the upAxis difference between stages.

I’ll add a some comments on the code, overall looks great !

Cheers,

[AlexRamallo]

Hmm, I didn't notice the camera jumping randomly until you pointed it out. I couldn't get it to reproduce consistently either, but with print debugging I can see the cause is due to random large spikes in the mouse delta coming into the Move function. I suspect it's related to this bug in GLFW, which other people report when using the same API to lock the mouse. It seems like a common solution is to always ignore the first delta after locking the mouse. I'll need to investigate a bit more to find the best place to implement that.

I like the idea of having a dedicated manipulator for the fly camera, as it'll provide a cleaner way to implement more advanced features like input rebinding, support for more peripherals (gamepad, etc).

One approach could be to rename CameraManipulator to OrbitCameraManipulator, and introduce FlyCameraManipulator. In that case, the CameraRig class could probably be removed/merged into the orbit camera. Or instead it could act as a generic interface/API to the various camera manipulators. Viewport::FrameCameraOnSelection would use it, for example (addons could too). As a base class, CameraRig could also provide common math functions like FromCameraTransform. Although in that case it probably makes sense to rename it to CameraManipulatorBase or something like that. Multiple manipulators would mean more complex logic for choosing the right camera manipulator. That probably won't be a problem though, as MouseHoverManipulator is the only logical place where that decision will need to be made, right?

Alternatively, the current CameraManipulator could implement its own internal state machine, with dedicated state classes for each type of camera (OrbitCameraRig, FlyCameraRig, etc). That would help to keep all the camera logic isolated. Viewport would only need to store one manipulator type instead of multiple.

[cpichard]

Thanks Alex !
I think the approach to rename CameraManipulator to OrbitCameraManipulator, and add a new FlyCameraManipulator would be perfect. I had in mind to keep all the code for camera modification in CameraRig, to be reused elsewhere, may be this is overkill … I’ll simplify later in that case. I believe the decision to use the camera manipulator is only in MouseHoverManipulator. I can’t really look at the code right now (I am away), but I would think the viewport could have a function to return the chosen camera manipulator which could be either OrbitCamMan or FlyCamMan.

[AlexRamallo]

Alright, just had a 'fun' time doing a bunch of camera math :)

• Previously, I stored the yaw/pitch in a variable, but now that there are two separate manipulators it was a pain to keep them synchronized, so instead I'm manually computing them from the camera transform as needed. The math might not be the best, but it's much nicer than the previous solution.

• I think I got the Z/Y up stuff right, but not 100% sure. I rotated the kitchen scene in blender to create a +Y up version and used that for testing, and things seem to work fine. I made some changes so the default editor camera is in the correct orientation.

• CameraRig is a common base for the two manipulators, used to share some math, but also for Viewport to control them. The method Viewport::FrameCameraManipulatorsOnBBox iterates the fixed array of manipulators to call the framing function on all of them (although now that I think about it, orbit might be the only manip that needs to support framing?)

• I replaced To/FromCameraTransform with Set/GetCameraTransform. An important caveat is that these methods hide the real coordinate system, and assume +Y up for input/output. This was meant to simplify things, though there are some pitfalls, like I had to change CameraRig::FrameBoundingBox to convert that bbox centroid to be +Y up.

• I moved the mouse capture API into Editor, and created a method to access the Editor singleton. It's kind of hacky, but it's a workaround for the cleanup order issues in main (alternatively, could use TfSingleton and just call exit to let the os clean things up)

And now the ugly

• I tried implementing a workaround for the mouse jump bug, but it sucks. On my machine at least (Linux, Wayland, UT running under Xwayland), ignoring the first frame wasn't enough. The random jumping didn't disappear until I ignored 3 frames, which might result in some noticeable latency for viewport interactions. I wonder if the bug might be in the ImGui GLFW backend, or exacerbated by it? Maybe it'd be better to read glfw mouse coordinates directly and calculate deltas manually.

I did at least confirm that disabling mouse capture fixes the random jumping issues.

[cpichard]

since _mouseCapture and gSkippedFirstMouseEventAfterCapture are somewhat unique when the application run, they could go in the Viewport as static variables instead of the Editor. All the functions related to those variables could also move as static functions in Viewport. This will also remove the need for an editor singleton, the calls would look like Viewport::SetMouseCaptured(true);

[AlexRamallo]

I moved the mouse capture API into Editor because it makes more sense to have it only in one place, since mouse "locking" is a global flag unassociated with any viewport/widget. Having it in viewport makes it seem like it's valid to call SetMouseCaptured(true) simultaneously for multiple viewports. Doing so shouldn't break anything, but I think it's a confusing API. Additionally, other systems/widgets might want to lock the mouse too.

If you don't like Editor becoming a singleton, it could instead go in another singleton dedicated to global window manager/platform stuff. In the future, it might be expanded to expose things like tray icons, multiple windows, system notifications, etc.

...but actually think I should just remove all the mouse locking stuff for now and leave a stub, until I can find the time to properly track down and fix the mouse jumping thing. What do you think?

[AlexRamallo]

btw, if it wasn't clear from my comment, the gSkippedFirstMouseEventAfterCapture stuff doesn't really solve the problem. Making it skip 3 frames appears to solve it on my machine, but as I can't reliably reproduce the bug, I'm not 100% sure, and I feel like it's just sweeping the problem under the rug...

[cpichard]

The mouse capture is really cool, if the problem is only happening when glfw hides the cursor, then I think it’s not a big deal if we don’t hide the cursor but keep the capture mechanism. This would avoid the workaround for sure.
Ok, I understand why you wanted to add the capture in the Editor. My point was more about trying to avoid storing the Editor pointer in a global variable, I think it is better to store the mouseCapture variable as static variable instead. In the Editor or Viewport then have static functions access it:

class Editor {

    static void SetMouseCaptured(bool captured);
    static bool GetMouseCaptured() { return _mouseCaptured; }
    static bool _mouseCaptured;
};

This way you can access the mouse capture functions without the need of the GetInstance() function like this:

 ….
 Editor::SetMouseCaptured(true);
…

Personally I would add mouseCapture in the Viewport since it’s only used there and refactor it if it is needed somewhere else later on, but I don’t mind if it is in the Editor as it also makes sense, your call :)

The code proposed here is not “technically” a singleton in the sense that a singleton would prevent the creation of objects to make sure there is only one possible in the application. Here it is still possible to create more Editor and GetInstance would point to one of them only. If we wanted to have a singleton we would have to rewrite the constructor and all the Editor function calls with GetInstance(), that’s a lot of refactoring for not additional feature or functionalities. Also I have in mind that we might need to have multiple editors living at the same time, possibly in a wasm port, so I tried to avoid making it a singleton since there is no compelling reason to do so.

Thanks for all the update, that’s really cool !

[AlexRamallo]

I spent some time debugging the mouse issue more thoroughly, and I've at least managed to confirm why skipping 3 frames works. The TL;DR is that it has to do with the way the delta is calculated using cached mouse position from previous frames in imgui_impl_glfw. I think there's a path to solve the problem by skipping less frames, but doing so would require quite a bit of refactoring and adding new APIs, and probably not worth the effort. FWIW I did a blind test with someone, and they didn't notice the initial 3 frame latency when controlling the camera.

I refactored the existing solution a bit to get rid of Editor::GetMouseDelta to avoid further bloating the Editor class, and I got rid of GetInstance too as your idea for making the mouse capture static is certainly better. However, I left the SetMouseCaptured API in Editor. This can change in the future if needed, but for now I think Editor::SetMouseCaptured is simple enough.

The commit I just pushed contains the refactored mouse lock code, which didn't exhibit the jumping behavior in my local testing, but more testing from other people on other platforms would be needed. If it does still happen, tweaking gSkipCapturedMouseDelta = 2; (in Editor::SetMouseCaptured) to a higher value will likely fix it.

[cpichard]

bool set I generally avoid to name variables with names which can be confused with other things like data structures. Ideally renaming set to something else like mouseCaptured or other would be great

[cpichard]

remove printf ?

[cpichard]

would be better to have an else if instead of ìf`since it allows to avoid testing more conditions and save some cpu work

[cpichard]

same as above if->else if

[cpichard]

Could you a formatting pass the parts of the code you added/modified with clang-format ? There is a rule file a the root of the project so clang-format will pickup the correct formatting. (Try not to format all the project otherwise there will be a lot of diffs :) )
If not I'll do it later once it's merged.
Cheers !

[cpichard]

Hi @AlexRamallo , are you happy with your last changes ? To me it's all good, let me know and I'll merge.
I have seen that there is still the global variable pointing to the editor, I can remove it later since it shouldn't be used anymore.
Thanks again for this contribution !