Add getters for camera parameters #1419 by Dtsitos · Pull Request #2767 · f3d-app/f3d

Dtsitos · 2026-01-02T12:21:42Z

Describe your changes

This PR adds read-only getters to the camera API to retrieve derived world-space
camera 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.

Original PR: #2724

Issue ticket number and link if any

Fixes #1419

Checklist for finalizing the PR

I have performed a self-review of my code
I have added tests for new features and bugfixes
I have added documentation for new features
If it is a modifying the libf3d API, I have updated bindings
If it is a modifying the .github/workflows/versions.json, I have updated docker_timestamp

Continuous integration

Please write a comment to run CI, eg: \ci fast.
See here for more info.

github-actions · 2026-01-02T12:21:50Z

You are modifying libf3d public API! ⚠️Please update bindings accordingly⚠️!
You can find them in their respective directories: c, python, java, webassembly.

Dtsitos · 2026-01-02T12:22:07Z

I think this should be correct now, will work on better testing ASAP

mwestphal · 2026-01-03T15:30:51Z

please let us know when its ready for review @Dtsitos

Dtsitos · 2026-01-04T14:18:26Z

@mwestphal Hello there, I have implemented all the bindings and some basic testing for them. Full disclosure, I have never worked with bindings again and mostly deduced what to do based on the patterns from the existing implementations. Thank you for your patience

mwestphal · 2026-01-04T17:45:16Z

@mwestphal Hello there, I have implemented all the bindings and some basic testing for them. Full disclosure, I have never worked with bindings again and mostly deduced what to do based on the patterns from the existing implementations. Thank you for your patience

Thats perfectly fine and it looks good to me :)

mwestphal · 2026-01-04T17:45:58Z

@Dtsitos can you adress and resolve the suggestions made by @snoyer above ?
If you did adress them already, please resolve the discussions.

mwestphal · 2026-01-04T17:46:11Z

\ci full

Dtsitos · 2026-01-07T20:17:00Z

\ci full

mwestphal · 2026-01-07T20:56:42Z

not ready for full CI yet :)

mwestphal · 2026-01-10T08:25:03Z

do you need help with the style check @Dtsitos ?

Dtsitos · 2026-01-12T16:33:33Z

do you need help with the style check @Dtsitos ?

Yes please, I am really not sure what is wrong. Sorry for not replying earlier but exam season is starting and I am not having fun with that 🙃

mwestphal · 2026-01-16T07:43:56Z

do you need help with the style check @Dtsitos ?

Yes please, I am really not sure what is wrong. Sorry for not replying earlier but exam season is starting and I am not having fun with that 🙃

copy the comment above
paste in a file name file.patch
apply the patch on your code patch -p1 < file.patch
commit and push the change

Dtsitos · 2026-01-18T13:43:28Z

@mwestphal Hello there. Thank you for showing me how to fix styling. I changed the tests for the new getters radically because after careful consideration (bashing my head against the keyboard) I realised that the tests produced wrong values not because the math is wrong but because I cannot play around with custom environment vectors as the test file does not have access to the renderer methods to change the environmental vectors. The tests work now locally. I will push with the proper styling and then it should be good to go.

snoyer · 2026-01-18T14:10:12Z

The environment vectors are indeed not accessible from the camera, however you can set the up vector through the engine's options, see for example

f3d/library/testing/TestSDKDynamicUpDirection.cxx

Lines 53 to 57 in ae5488a

    
           opt.scene.up_direction = { 1, 0, 0 }; 
        
           win.render(); 
        
           newUp = cam.getViewUp(); 
        
           test("camera view up rotated to +X", compareVec(newUp, { 1, 0, 0 }));

you'll need to set options.scene.up_direction and call engine.render() before setting the camera state and performing your checks but that should let you test any case you need

Dtsitos · 2026-01-23T20:36:03Z

Alas, my testing and styling is proper. What should I do now to proceed with the PR? I am guessing I need a CI full check first.

mwestphal · 2026-01-24T09:50:25Z

What should I do now to proceed with the PR?

Ask for a review from a maintainer, top right corner of the PR :)

mwestphal

some questions

mwestphal

small questions and suggestions

…ng in TestSDKCamera.cxx

…ign issue in camera_impl.cxx

Dtsitos · 2026-01-28T10:57:01Z

So I am not sure if force pushing was a nuclear move but it seemed necessary for a proper rebase. The new tests integrate properly with the new format and work now, so I think it is ready for review again

mwestphal · 2026-01-28T13:10:31Z

So I am not sure if force pushing was a nuclear move but it seemed necessary for a proper rebase. The new tests integrate properly with the new format and work now, so I think it is ready for review again

Force push is indeed part of the normal rebase workflow.

mwestphal · 2026-01-29T07:23:48Z

+  test("getWorldAzimuth finite after camera operations", std::isfinite(cam.getWorldAzimuth()));
+
+  test("getWorldElevation finite after camera operations", std::isfinite(cam.getWorldElevation()));


why do you test isfinite ? test just expected value ?

Due to the other changes requested I changed testing drastically, mostly to improve me own understanding of the math and to be more thorough

mwestphal

changes needed

Meakk · 2026-01-29T08:13:11Z

+void camera_impl::getPositionToFocalVector(vector3_t& vec) const
+{
+  point3_t pos, focal;
+  this->getPosition(pos);
+  this->getFocalPoint(focal);
+
+  vtkMath::Subtract(pos.data(), focal.data(), vec.data());
+}


(pos - focal) is not "position to focal" vector, it's the opposite. Do you mean vtkMath::Subtract(focal.data(), pos.data(), vec.data()); or name getFocalToPositionVector?

To avoid causing problems downstream (with bindings and such) I changed the subtract order

Meakk · 2026-01-29T08:17:08Z

+  double* up = ren->GetEnvironmentUp();
+  double* right = ren->GetEnvironmentRight();
+
+  // Project view vector onto plane orthogonal to up


The comment is not correct. You are not projecting the view vector to the plane orthogonal to up, you are projecting the view vector to the up direction.

I changed the azimuth implementation based on the convention you ask for in a comment further down so that is resolved.

Meakk · 2026-01-29T08:26:08Z

+  vtkMath::Normalize(horizontal.data());
+
+  // Signed angle between right and horizontal projection
+  double angleRad = vtkMath::SignedAngleBetweenVectors(right, horizontal.data(), up);


From my comments above, the horizontal direction computed is pointing behind the camera. Make sure it gives the same result here as I expect the sign to be flipped here.

Meakk · 2026-01-29T08:30:23Z

+  double angleRad = vtkMath::SignedAngleBetweenVectors(right, horizontal.data(), up);
+
+  return vtkMath::DegreesFromRadians(angleRad);


I agree with @snoyer. If we just query the azimuth after opening a file without changing the camera, I'd expect 0 to be returned.

…tion. Improved testing based on new approach

Dtsitos · 2026-01-29T17:01:07Z

I changed the testing quite a lot to make it more thorough (edge case and custom environment for each getter) and changed my world azimuth convention to align with expectations from an above discussion (about azimuth being zero when looking forward). I hope my code is more up to spec now

mwestphal · 2026-01-29T18:55:18Z

Ill let @Meakk review this one :)

Meakk · 2026-02-18T08:33:02Z

+  const double* up = ren->GetEnvironmentUp();
+  const double* right = ren->GetEnvironmentRight();
+
+  // Derive environment forward = up × right


It looks like × is not an ascii character. I don't think it's necessary a problem but just wanted to make sure we agree with that @mwestphal

lol yeah, please use ASCII @Dtsitos

snoyer · 2026-02-18T08:35:17Z

+  vector3_t v;
+  this->getPositionToFocalVector(v);
+  return vtkMath::Norm(v.data());


Suggested change

vector3_t v;

this->getPositionToFocalVector(v);

return vtkMath::Norm(v.data());

return this->GetVTKCamera()->GetDistance();

is that equivalent?

Meakk · 2026-02-18T08:38:46Z

+
+  cam.setPosition({ 0., -10., 0. });
+  cam.setFocalPoint({ 0., 0., 0 });
+  test("Distance: 1", cam.getDistance(), approx(10.0));


Suggested change

test("Distance: 1", cam.getDistance(), approx(10.0));

test("Distance: 10", cam.getDistance(), approx(10.0));

Meakk

Typo and question but LGTM

Meakk · 2026-02-23T15:08:54Z

+  cam.setPosition({ -10., 0., 0. });
+  cam.setFocalPoint({ 0., 0., 0. });
+  cam.setViewUp({ 0., 1., 0 });
+  test("Azimuth (Y-up): environment forward (+X)", cam.getWorldAzimuth(), approx(90.0));


Let's add the code below to test setter/getter equivalence (credits to @snoyer):

std::vector<f3d::direction_t> up_directions = { { 0, 0, +1 }, { 0, +1, 0 }, { +1, 0, 0 }, { 0, 0, -1 }, { 0, -1, 0 }, { -1, 0, 0 }, { -1, +2, +3 }, { +4, -5, -6 }, }; const std::vector<std::pair<double, double>> azimuths_elevations = { { 0, 0 }, { +12, +34 }, { +12, -34 }, { -12, +34 }, { -12, -34 }, }; for (const auto up : up_directions) { for (auto [a, e] : azimuths_elevations) { f3d::engine eng = f3d::engine::create(true); f3d::window& win = eng.getWindow(); f3d::camera& cam = win.getCamera(); opt.scene.up_direction = up; win.render(); cam.azimuth(a).elevation(e); const std::string title = " after .azimuth(" + f3d::options::format(a) + ").elevation(" + f3d::options::format(e) + ") with up = " + f3d::options::format(up); test("azimuth " + title, cam.getWorldAzimuth(), approx(a, 1e-10)); test("elevation" + title, cam.getWorldElevation(), approx(e, 1e-10)); } }

Meakk · 2026-02-23T15:10:44Z

    return ret;
  }

+  JNIEXPORT jdouble JAVA_BIND(Camera, getWorldAzimuth)(JNIEnv* env, jobject self)


Missing functions in Camera.java

Meakk · 2026-02-23T15:12:41Z

Here's a suggested patch by @snoyer on discord to fix the flipped signed:

diff --git a/library/src/camera_impl.cxx b/library/src/camera_impl.cxx
index 2d4bd562b..5b47f0d68 100644
--- a/library/src/camera_impl.cxx
+++ b/library/src/camera_impl.cxx
@@ -2,6 +2,7 @@
 
 #include <vtkCamera.h>
 #include <vtkMatrix4x4.h>
+#include <vtkPlane.h>
 #include <vtkRenderWindow.h>
 #include <vtkRenderer.h>
 #include <vtkVersion.h>
@@ -116,56 +117,39 @@ void camera_impl::getPositionToFocalVector(vector3_t& vec) const
 //----------------------------------------------------------------------------
 double camera_impl::getWorldAzimuth() const
 {
-  vector3_t view;
-  this->getPositionToFocalVector(view);
-  vtkMath::Normalize(view.data());
-
   vtkRenderer* ren = this->Internals->VTKRenderer;
   const double* up = ren->GetEnvironmentUp();
-  const double* right = ren->GetEnvironmentRight();
-
-  // Derive environment forward = up × right
-  vector3_t forward;
-  vtkMath::Cross(up, right, forward.data());
-  vtkMath::Normalize(forward.data());
-
-  vector3_t projUp;
-  vtkMath::ProjectVector(view.data(), up, projUp.data());
 
-  vector3_t horizontal;
-  vtkMath::Subtract(view.data(), projUp.data(), horizontal.data());
+  double projectedView[3];
+  vtkPlane::ProjectVector(this->GetVTKCamera()->GetDirectionOfProjection(),
+    this->getFocalPoint().data(), up, projectedView);
 
   static constexpr double EPS = 128 * std::numeric_limits<double>::epsilon();
-  if (vtkMath::Norm(horizontal.data()) < EPS)
+  if (vtkMath::Norm(projectedView) < EPS)
   {
     return 0.0;
   }
 
-  vtkMath::Normalize(horizontal.data());
+  const double angleRad =
+    vtkMath::SignedAngleBetweenVectors(ren->GetEnvironmentRight(), projectedView, up);
 
-  double angleRad = vtkMath::SignedAngleBetweenVectors(horizontal.data(), forward.data(), up);
-
-  return vtkMath::DegreesFromRadians(angleRad);
+  return vtkMath::DegreesFromRadians(angleRad) - 90.0;
 }
 
 //----------------------------------------------------------------------------
 double camera_impl::getWorldElevation() const
 {
-  vector3_t view;
-  this->getPositionToFocalVector(view);
+  double* view = this->GetVTKCamera()->GetDirectionOfProjection();
 
   static constexpr double EPS = 128 * std::numeric_limits<double>::epsilon();
-  if (vtkMath::Norm(view.data()) < EPS)
+  if (vtkMath::Norm(view) < EPS)
   {
     return 0.0;
   }
-  vtkMath::Normalize(view.data());
 
   vtkRenderer* ren = this->Internals->VTKRenderer;
-  double* up = ren->GetEnvironmentUp();
-
-  double angleRad = vtkMath::AngleBetweenVectors(view.data(), up);
-  return 90.0 - vtkMath::DegreesFromRadians(angleRad);
+  const double angleRad = vtkMath::AngleBetweenVectors(ren->GetEnvironmentUp(), view);
+  return vtkMath::DegreesFromRadians(angleRad) - 90.0;
 }
 
 //----------------------------------------------------------------------------

mwestphal · 2026-03-22T12:25:51Z

Still working on this @Dtsitos ?

mwestphal mentioned this pull request Jan 2, 2026

Add getters for camera parameters #1419 #2724

Closed

5 tasks

snoyer reviewed Jan 2, 2026

View reviewed changes

Comment thread library/src/camera_impl.cxx Outdated

snoyer reviewed Jan 2, 2026

View reviewed changes

Comment thread library/src/camera_impl.cxx

mwestphal added the ci:full label Jan 4, 2026

mwestphal added ci:fast and removed ci:full labels Jan 7, 2026

Dtsitos requested a review from mwestphal January 24, 2026 11:39

mwestphal reviewed Jan 24, 2026

View reviewed changes

Comment thread library/src/camera_impl.cxx Outdated

mwestphal reviewed Jan 24, 2026

View reviewed changes

Comment thread python/testing/test_camera.py

mwestphal requested changes Jan 24, 2026

View reviewed changes

Dtsitos requested a review from mwestphal January 24, 2026 13:25

mwestphal requested changes Jan 25, 2026

View reviewed changes

Comment thread library/testing/TestSDKCamera.cxx Outdated

Comment thread library/src/camera_impl.cxx Outdated

mwestphal requested review from Meakk and snoyer January 25, 2026 11:09

Dtsitos added 7 commits January 28, 2026 12:07

Fixed styling and variable redeclarations in TestSDKCamera.cxx

154cdd0

Improved styling and test logic in TestSDKCamera.cxx

9a16c21

Fix failing TestSDKCamera.cxx by correcting elevation sign

f104268

Proper rebase of new tests

feab884

Changed include directive order in camera_impl.cxx

ef6c187

Removed redundant cmath include in camera_impl.cxx and improved testi…

9fe4b8d

…ng in TestSDKCamera.cxx

Rebased testing for getters in TestSDKCamera.cxx and fixed minor styl…

80df8d6

…ign issue in camera_impl.cxx

Dtsitos force-pushed the feature/getters-for-camera branch from 670e796 to 80df8d6 Compare January 28, 2026 10:35

Fixed styling for new tests at TestSDKCamera.cxx

fb6e9a7

Dtsitos requested a review from mwestphal January 28, 2026 10:57

mwestphal reviewed Jan 29, 2026

View reviewed changes

mwestphal requested changes Jan 29, 2026

View reviewed changes

Meakk reviewed Jan 29, 2026

View reviewed changes

Dtsitos added 2 commits January 29, 2026 18:41

Changed azimuth convention and fixed position to focal vector calcula…

75a383b

…tion. Improved testing based on new approach

Removed unknown include directive

5f48456

Dtsitos requested a review from mwestphal January 29, 2026 17:01

mwestphal requested a review from Meakk January 29, 2026 18:54

mwestphal removed their request for review January 29, 2026 19:29

Meakk reviewed Feb 18, 2026

View reviewed changes

snoyer reviewed Feb 18, 2026

View reviewed changes

Meakk reviewed Feb 18, 2026

View reviewed changes

Meakk approved these changes Feb 18, 2026

View reviewed changes

Meakk reviewed Feb 23, 2026

View reviewed changes

		test("getWorldAzimuth finite after camera operations", std::isfinite(cam.getWorldAzimuth()));

		test("getWorldElevation finite after camera operations", std::isfinite(cam.getWorldElevation()));

		double angleRad = vtkMath::SignedAngleBetweenVectors(right, horizontal.data(), up);

		return vtkMath::DegreesFromRadians(angleRad);

	test("Distance: 1", cam.getDistance(), approx(10.0));
	test("Distance: 10", cam.getDistance(), approx(10.0));

Uh oh!

Conversation

Dtsitos commented Jan 2, 2026 • edited by mwestphal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Issue ticket number and link if any

Checklist for finalizing the PR

Continuous integration

Uh oh!

github-actions Bot commented Jan 2, 2026

Uh oh!

Dtsitos commented Jan 2, 2026

Uh oh!

Uh oh!

Uh oh!

mwestphal commented Jan 3, 2026

Uh oh!

Dtsitos commented Jan 4, 2026

Uh oh!

mwestphal commented Jan 4, 2026

Uh oh!

mwestphal commented Jan 4, 2026

Uh oh!

mwestphal commented Jan 4, 2026

Uh oh!

Dtsitos commented Jan 7, 2026

Uh oh!

mwestphal commented Jan 7, 2026

Uh oh!

mwestphal commented Jan 10, 2026

Uh oh!

Dtsitos commented Jan 12, 2026

Uh oh!

mwestphal commented Jan 16, 2026

Uh oh!

Dtsitos commented Jan 18, 2026

Uh oh!

snoyer commented Jan 18, 2026

Uh oh!

Dtsitos commented Jan 23, 2026

Uh oh!

mwestphal commented Jan 24, 2026

Uh oh!

Uh oh!

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dtsitos commented Jan 28, 2026

Uh oh!

mwestphal commented Jan 28, 2026

Uh oh!

mwestphal Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dtsitos commented Jan 2, 2026 •

edited by mwestphal

Loading

mwestphal Jan 29, 2026 •

edited

Loading