-
-
Notifications
You must be signed in to change notification settings - Fork 365
Add getters for camera parameters #1419 #2724
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
Conversation
…ickly for further processing
…n to compute the distance from the camera position to the focal point
|
You are modifying libf3d public API! |
|
Hello there @mwestphal and thank you for the quick reply, I implemented the getters using the camera’s position and focal point with standard mathematical functions (sqrt, atan etc.) to provide world-space azimuth, elevation, and distance. This makes the code more simple, self-contained, and does not rely on renderer-specific environment vectors. Based on the issue description I think this is more desirable and less complex both to maintain and to understand. If this is alright with you I will start working on the bindings ASAP. |
Indeed, that looks fine, but I defer to @snoyer and @Meakk for the actual implementation review. shouldn't the method be called simply getElevation though ?
Lets add test first :) |
It does rely on the assumption that the environment's horizontal plane is XY and Z is up, which is not always the case in |
Right! It couldnt be that simple :) |
|
Need any help @Dtsitos ? |
|
I am sorry for not responding earlier I was packing for vacation. I have made progress and I think my solution is correct now. I will try to send it here when I get back home in three days. If I am causing a delay in the project and someone else wants to solve it feel free to unassign me and thank you for your co-operation. |
|
No worries! enjoy your vacation :) |
|
I think my solution should be complete now at least mathematically. I did work on the previous attempt another user made and mostly improved by avoiding code reuse with the helper function and doing the floating point check before normalising. Also I added some basic tests. If this solution seems alright I will try to work on the bindings. To be totally honest I have never worked with bindings before but I started reading up on that today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, do you think you can look at this test and add similar test cases ?
Indeed, it looks fine :), lets focus on improving the testing first, but you can work on the bindings as well. |
mwestphal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please rebase on master and also push a to a feature branch instead of master, even on your own branch.
| class internals; | ||
| std::unique_ptr<internals> Internals; | ||
| void getPositionToFocalVector(vector3_t& vec) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| class internals; | |
| std::unique_ptr<internals> Internals; | |
| void getPositionToFocalVector(vector3_t& vec) const; | |
| /** | |
| * Add some doc | |
| */ | |
| void GetPositionToFocalVector(vector3_t& vec) const; | |
| class internals; | |
| std::unique_ptr<internals> Internals; |
|
|
||
| /** Return the camera azimuth angle in degrees */ | ||
| [[nodiscard]] virtual double getWorldAzimuth() const = 0; | ||
| /** Return the camera elevation angle in degrees */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /** Return the camera elevation angle in degrees */ | |
| /** Return the camera elevation angle in degrees */ |
| vector3_t cross; | ||
| vtkMath::Cross(right, horizontal.data(), cross.data()); | ||
| double sign = (vtkMath::Dot(cross.data(), up) >= 0.0) ? 1.0 : -1.0; | ||
|
|
||
| double angleRad = vtkMath::AngleBetweenVectors(right, horizontal.data()); | ||
| return sign * vtkMath::DegreesFromRadians(angleRad); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can that be simplified with vtkMath::SignedAngleBetweenVectors?
| double dot = vtkMath::Dot(view.data(), up); | ||
| dot = std::clamp(dot, -1.0, 1.0); | ||
|
|
||
| return vtkMath::DegreesFromRadians(std::asin(dot)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can that be written in terms of AngleBetweenVectors?
|
Good evening (in my timezone at least) and happy new year!! I improved my documentation and my implementation for Azimuth based on the comment from @snoyer. I will start working on testing now. I made a new feature branch on my local repo as you requested but I am not accustomed to github and not sure how to change the pull request to not pull into f3d-app:master, could you help me with that? |
I'm afraid you need to create a new PR, there is no work around for this :) |
|
Will do! |
|
Superseed by: #2767 |
Describe your changes
This PR adds read-only getters to the
cameraAPI to retrieve derived world-spacecamera parameters (azimuth, elevation, and distance).
The values are computed from the camera position and focal point and do not
modify camera state. This enables external applications and bindings to query
camera orientation consistently without duplicating math.
No existing behavior is changed.
Issue ticket number and link if any
Fixes #1419
#1419
Checklist for finalizing the PR
.github/workflows/versions.json, I have updateddocker_timestampContinuous integration
\ci fast