Movable Camera pivot point Fix #265

[sdumetz]

I'm putting this here to gather feedback, it is a good proof of concept but has not been tested at all for bugs and omissions.

In particular I know CameraController.updateController() is broken, making the pose tab unusable.

However it demonstrates the feature's design :

• Adds a single "pivot" vector to rotate around
• Default behavior should be 100% consistent with existing scenes (to be tested).
• double-click on a model sets the pivot point there, resets the offset

The whole thing should ideally be animated but this will be the subject of another PR to keep things simple.

I'll show this around a bit and clean it up in a few days/weeks once I'm confident enough about the feature design.

@oM4Uo feel free to test it and comment if you had something else in mind.

[oM4Uo]

@sdumetz I'm really interested in trying and test your modifications but I wasnt able to build from source by following the https://smithsonian.github.io/dpo-voyager/introduction/installation/. The Docker container gives me an error and doesn't start (my Docker and in general my "coding" experience is poor, I'm sorry)

if you have time and willingness, and send me a build dist, I will be more than happy to try it and give you my feedback

[gjcope]

@oM4Uo if you want to start another issue (or a discussion) and provide some more details on the error you are getting, I can try and help with the build issues.

[oM4Uo]

@gjcope Thank you, but I was able to fix the issue just now. Specifically the container stops and print the error "Exited (127)"
I had to:

• run this command "git config --global core.autocrlf false" (I'm on windows)
• update the node.js used by the docker image; after several tests the one that worked for me was the version 16.15.1

@sdumetz As you wrote, double-clicking on the model resets the offset, and it works as expected. I also tested it on existing scenes, and it works. I also noticed that the 'orthographic' view is broken,

However, I was considering a different option, and for sure I didn't explain myself well the first time. Double-clicking on the model, or even a single click, should 'move' the point of rotation (around which the model rotates) to the spot that has been clicked. If I'm correct, right now the model rotates always around the center of the scene.

@gjcope, @sdumetz and all the people behind the prejoct I really thank you for your hard work

[sdumetz]

Hi,

thanks for taking the time to test it. It is really important to help improve the software.

I'll check why the orthographic view broke, I actually didn't test it at all.

I'm not sure I understand what you describe, maybe you can elaborate on that? Ideally point me to something that has the desired behavior? Or a minimal set of steps to reproduce what you would expect vs what happens here?

FWIW, what does happen internally is that the models never move (except if you edit their properties): It's the camera that does "orbit" around a point. In the existing system, this point is implicit and always located at (0,0,0). I made this "pivot" explicit and when you click somewhere, it sets the camera's orbit center there.

[oM4Uo]

Hi @sdumetz , today I learned a little bit more about how tree.js (are you using that right?) scenes, cameras, and so on work, and now I totally understand why you are 'confused'... again, it was my lack of knowledge and I expressed myself badly, sorry.

So, if I understood correctly, in voyager the camera is orbiting around the center of the scene at (0,0,0) as shown in the next figure

What I meant is that when you double-click on a point on the model, the camera should 'target' that point (eventually zoom on it) and orbit around it, as shown in the next figure

As examples:

• ATON and you could play with this model Nuraghe La Prisgiona - in this case after double-click camera zoom on the point and orbit around it, double-click again and camera zoom a little bit more; to go back to the initial position ATON use the "Home" button that has a saved viewpoint attached (like the "center" button here in voyager)
• View3D, look under the "Option" tab to find a model to play with - in this other one after double-click camera zoom on the point and orbit around it, double-click again bring the camera back to the initial position
• Potree, even though it is primarily designed to handle point clouds after double-click camera zoom on the point and orbit around it

Where I'm using the word "zoom", I don't know if the camera simply changes the FOV, moves closer to the point, or a combination of both, or something else. However, I hope I don't mislead you again.

Thank you again, and I'm more than happy to help, test, and provide feedback. I'm sorry I can't assist with coding.

Cheers

[sdumetz]

Unless I'm mistaken, you describe the default Voyager behavior and what you need is in this PR. Are you sure you checked-out the right branch?

The main difference between my implementation and your drawings is that I set the pivot point on the model's center while I set it on its surface (where the user clicked in 2D space, projected into the scene).

I'm pretty confident pivoting on the model's center not a good idea because it makes no sense for the user as soon as the center is far from where he clicked.

[oM4Uo]

Ouch! So, to be clear, for you is working like ATON, View3D or Potree after your modification or even without it?

I'm pretty confident pivoting on the model's center not a good idea because it makes no sense for the user as soon as the center is far from where he clicked

I´m totally agree, that is why I started this thread

[sdumetz]

After my modification it's working (kind of) like the others. Before, it was always pivoting around the scene's (0,0,0) point, which might not be the same as the model's center: Models have a center point (mesh's origin point, defined in the GLB) and a "transform" point, initially always set to (0, 0, 0) but that can be moved around.

[oM4Uo]

The issue for me was that I was using the scene.svx.json file created from the Voyager Story Standalone. To have the double-click work as intended (i.e., similar to the other viewer that I mentioned), I had to manually create a new scene file using this simple template. Afterward, I also tried using this manually-created scene file in a local Voyager Story, made some modifications, saved it again, and finally everything was working fine.

I don't know which parameter is missing or set differently when generated from Voyager Story Standalone, but I will look into it in the next few days.

Regarding the issue with the "broke orthographic view," I don't think it's properly broken. However, when you click on it or, for instance, on every button under the "View" toolbox, the model most likely goes off-camera.

Finally, the camera behaves oddly if you are in "Tour mode" and double-click on the model.

[oM4Uo]

@sdumetz So, when the model's pose is modified (e.g., position or rotation is different from 0,0,0 - in the JSON scene file, they are defined under model translation and rotation), the double-click doesn't work as expected. Ultimately, it was this setting that caused the unexpected double-click behavior for me, as I always used it to center the model in the scene.

I apologize for the confusion and any inconvenience caused by me. I'm patiently waiting for the final implementation of this feature. Again, sorry for any inconvenience

[sdumetz]

Oh no worries, thanks for the time you took to test it.

Now I know what I need to fix 😄

I'll try to wrap this up before the end of the week.

[oM4Uo]

if you want to start another issue (or a discussion) and provide some more details on the error you are getting, I can try and help with the build issues.

@gjcope a small update, I saw that on the rc-40 branch, among other things, you updated the ubuntu and node version and with this branch I had no issue on windows

Thanks

[sdumetz]

I pushed cb74df4 with some fixes.

@gjcope I don't think I agree with ff-three's zoomExtents. If I understand it correctly (and I might not, my vector math is bad...) it uses the offset to fit the model's bounding box. Which feels wrong to me? Don't we want the view to stay centered? I'd like to have your opinion on this.

Apart from this, I still have to properly take into account the model's "pose" and I think I'll be done.

[sdumetz]

Everything looks OK now, except maybe the behavior of zoomExtents (which may as well be left as it is IMO).

Resetting the pivot point in an existing tour requires disabling the "navigation" and enabling it again. AFAIK it's not a bug in this code but a shortcoming of the current tour system that should be fixed separately. If a user does move the pivot point before/during/after a tour that was created without a pivot point, the view gets totally broken and I don't think I can do anything about it.

This whole feature could easily be toggled with a flag but my preference would be to have it "always on".

[gjcope]

I pushed cb74df4 with some fixes.

@gjcope I don't think I agree with ff-three's zoomExtents. If I understand it correctly (and I might not, my vector math is bad...) it uses the offset to fit the model's bounding box. Which feels wrong to me? Don't we want the view to stay centered? I'd like to have your opinion on this.

Apart from this, I still have to properly take into account the model's "pose" and I think I'll be done.

Do you have an example of behavior that feels wrong? zoomExtents definitely needs to be improved, but I'm not sure centering is the issue; it's using the center of the bounding box.

[sdumetz]

The "pose" viewports that are centered around the model instead of the origin feels very wrong to me. However that could be tackled differently by adding a visual "origin" helper in the pose task.

As it is now and if we want to keep its current behaviour, zoomExtents doesn't zoom properly when autoZoom is enabled and the pivot point is not (0, 0, 0) because composeOrbitMatrix doesn't account for the pivot point.

thinking about it, I might just add a boolean toggle to zoom with offset (desired in autoZoom) or without ( desired(?) in PoseTask's Viewports).

[gjcope]

I see. I think zoomExtents functions as intended by framing the 'extents' of the model/models. The bigger question is probably how helpful is this in regular use.

I think the use case for us is in the alignment of multiple models. Objects that were scanned independently (like a jar and lid) sometimes appear in dramatically different positions and orientations in the coordinate space. It can be helpful to quickly see both objects and reframe as adjustments (mainly translation) are made. That being said, this process can still be difficult due to the need for local transformations when posing, but I think that's a related but different topic.

[sdumetz]

@gjcope Do you think this is possibly OK to merge or is there still some roadblocks for you?

No pressure, just checking if there is something more I need to do or if I can cross this off my list for now?

[gjcope]

Based on the thread, it seems like there is still an unresolved issue with the pose task zoom working correctly? Other than that I think it just needs testing on our end.

[sdumetz]

Hi,
there is a debate over how the zoom should behave, but I don't think I have a good solution for it yet so I've left it no better or worse than it was, unless I introduced a bug somewhere.

I intend to work on better visual indicators to help position models in a scene in a few months because we have some users creating scenes with lots of heterogeneous data and the current system is only helpful for one to a few models. I'll look at zoomExtents again in this context then and see if I can improve it on the way.

[oM4Uo]

Hi! I noticed that the saved annotation viewpoints take as reference the last camera scene position (the one set after double-clicking), but they are not updated after successive changes, in other words, when double-clicking after the annotation viewpoint is set. This causes the desired camera position not to be viewed correctly when moving to the saved annotation viewpoint. When switching to orthometric view, the camera moves far from the model, but as soon as you right or left-click on the screen, the camera goes back to the correct position. Lastly, the perspective/orthometric options are not synced between the options in the left panel and the toolbar.

[sdumetz]

Hi, thanks for taking time to test!

Annotation viewpoints were not saving the "Pivot" property. Fixed in d0cf871.

When switching to orthometric view, the camera moves far from the model, but as soon as you right or left-click on the screen, the camera goes back to the correct position

I couldn't reproduce this, when I switch from Ortho to perspective the view stays roughly the same. Maybe it's a scale thing or something similar, i'll admit I tinkered on the Ortho/Persp conversion algorithm and am not sure at all of its correctness. Could you check if it still happens after b7c23cf (pushed today) and if it does, share a scene where you see this happening?

the perspective/orthometric options are not synced between the options in the left panel and the toolbar

It's unrelated to those changes but I was able to track it down to both fields referencing a different property with only a one-way binding. Fixed in b7c23cf. @gjcope I did the same thing in as in #280 and removed the duplicate as it served no meaningful purpose that I could find. The binding is a little bit convoluted in CVViewTool, going through document.setup.navigation.scene.activeCameraComponent to find the actual property. Maybe there is a shortcut that I couldn't find?

[gjcope]

@sdumetz I haven't had a chance to test your change yet but taking out the projection property binding means the component update won't get triggered when projection updates.

[sdumetz]

Yes but it doesn't need to, does it?

The actual camera is updated from CVCamera->CCamera when the projection updates. and that was the only thing this updates did. Or did I miss something?

I didn't notice anything failing to update, but yet again those changes are hard to really test beside clicking-around a bit and see if something is off.

[sdumetz]

7ef3a3c fixes a bug that happens when you nest models as children of another model. That's slightly unconventional but we use this as a workaround for #255

[sdumetz]

Force-push to rebase on v0.43.0 and include changes from Smithsonian/ff-three#3

[gjcope]

Sorry for the delay, I was finally able to take a closer look at this.

1. I see the same Ortho view issue noted by @oM4Uo in this thread. The projection changes with a zoom and needs a re-render (click, etc.) to update correctly. This seems to have been introduced by 47c5c8b - it goes away if I revert that commit.
2. The perspective view in the pose task doesn't zoom extents correctly with a modified pivot.
3. It's also noted in this thread, but I think the snapshot transformation issue is a barrier. Having tours or annotation viewpoints that don't provide the intended view seems like a blocker. If the state machine always animates on stored parameters, it should be workable no? Except for legacy scenes with no stored pivot, in which case it seems like we can assume the center of the original bound. Deserves some more though but I think needs to be solved...
4. Is this actually meant to be in the PR? source/client/ui/story/AddNodeMenu.ts

[sdumetz]

I Fixed 4.. Still looking into 2. and 3..

Concerning 1. I'd really like to keep 47c5c8b because I really hate duplicated properties that would make the problem reappear if we somehow changed the camera's projection from another place.

The underlying issue is that CCamera has no clue of the render process so it won't force CameraController to update. 342ab8f is a workaround that adds back a stored value for camera projection in CVOrbitNavigation but without the property ambiguity.

It works, but I don't like it. Having camera state flow back to the orbit navigation component seems especially fragile if we are looking into implementing alternative navigation modes in the near future.

[gjcope]

Thanks for the updates. Agree with you on the fragility. I anticipate the future work will involve a generalized navigation component from which the different modes inherit so slotting this check there could make it a little more robust.

[sdumetz]

Pivot coordinates were not properly rotated before application in zoomExtents. That's done and the results now looks OK.

zoomExtents still does not feel natural (to me) when applied to the autoZoom feature but I can't figure out what to make of it.