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

Movable Camera pivot point Fix #265 #266

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

Conversation

sdumetz
Copy link
Contributor

@sdumetz sdumetz commented Apr 15, 2024

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
Copy link

oM4Uo commented Apr 18, 2024

@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
Copy link
Collaborator

gjcope commented Apr 18, 2024

@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
Copy link

oM4Uo commented Apr 18, 2024

@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
Copy link
Contributor Author

sdumetz commented Apr 19, 2024

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
Copy link

oM4Uo commented Apr 19, 2024

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
Copy link
Contributor Author

sdumetz commented Apr 22, 2024

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
Copy link

oM4Uo commented Apr 22, 2024

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
Copy link
Contributor Author

sdumetz commented Apr 22, 2024

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
Copy link

oM4Uo commented Apr 22, 2024

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
Copy link

oM4Uo commented Apr 23, 2024

@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
Copy link
Contributor Author

sdumetz commented Apr 23, 2024

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
Copy link

oM4Uo commented Apr 23, 2024

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
Copy link
Contributor Author

sdumetz commented Apr 30, 2024

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
Copy link
Contributor Author

sdumetz commented Apr 30, 2024

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
Copy link
Collaborator

gjcope commented Apr 30, 2024

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
Copy link
Contributor Author

sdumetz commented May 2, 2024

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
Copy link
Collaborator

gjcope commented May 2, 2024

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.

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.

None yet

3 participants