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
base: master
Are you sure you want to change the base?
Conversation
@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 |
@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 Thank you, but I was able to fix the issue just now. Specifically the container stops and print the error "Exited (127)"
@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 |
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. |
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:
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 |
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. |
Ouch! So, to be clear, for you is working like ATON, View3D or Potree after your modification or even without it?
I´m totally agree, that is why I started this thread |
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. |
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. |
@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 |
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. |
@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 |
32d4e2c
to
4abe014
Compare
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. |
ac5cef9
to
ab18311
Compare
Everything looks OK now, except maybe the behavior of 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". |
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. |
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, thinking about it, I might just add a boolean toggle to zoom with offset (desired in |
I see. I think 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. |
85b980a
to
342eced
Compare
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 :
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.