Camera Controller: expose complete API to python #418

Tucchhaa · 2024-08-23T13:37:13Z

In this PR:

New props added to camera controller to control it's reset behaviour: defaultPosition, defaultUpVector, defaultViewCenter, defaultLookSpeed, defaultLinearSpeed
All these props are exposed to python
Camera controller python API was rewritten to use def_property using macros. Tests also were updated
Minor issue was fixed: When reset camera, aspect ratio was incorrect

Tucchhaa · 2024-08-23T19:58:36Z

cpp/modmesh/view/R3DWidget.cpp

-void R3DWidget::resetCamera() const
-{
-    Qt3DRender::QCamera * camera = m_view->camera();
-
-    // Set up the camera.
-    camera->lens()->setPerspectiveProjection(45.0f, 16.0f / 9.0f, 0.1f, 1000.0f);
-    camera->setPosition(QVector3D(0.0f, 0.0f, 10.0f));
-    camera->setViewCenter(QVector3D(0.0f, 0.0f, 0.0f));
-    camera->setUpVector(QVector3D(0.f, 1.f, 0.f));
-
-    // Set up the camera control.
-    auto * control = m_scene->controller();
-    control->setCamera(camera);
-    control->setLinearSpeed(50.0f);
-    control->setLookSpeed(180.0f);
-}
-


resetCamera moved to CameraController

Moving it to CameraController makes it more testable. I like it.

Tucchhaa · 2024-08-23T19:59:00Z

cpp/modmesh/view/R3DWidget.hpp

 public:

-    explicit RScene(Qt3DCore::QNode * parent = nullptr)
-        : Qt3DCore::QEntity(parent)
-        , m_controller(new ROrbitCameraController(this))
+    explicit RScene(QNode * parent = nullptr)
+        : QEntity(parent)
    {
+        m_controller = new ROrbitCameraController(this);
    }

-    Qt3DExtras::QAbstractCameraController const * controller() const { return m_controller; }
-    Qt3DExtras::QAbstractCameraController * controller() { return m_controller; }
+    CameraController * controller() const { return m_controller; }
+    Qt3DExtras::QAbstractCameraController * qtController() const { return m_controller->asQtCameraController(); }

-    void setCameraController(Qt3DExtras::QAbstractCameraController * controller)
+    void setCameraController(CameraController * controller)
    {
-        m_controller->deleteLater();
+        qtController()->deleteLater();


small refactoring to controller() getters. Logic didn't change, but because our CameraController class is used more often than Qt's less dynamic_cast's will happen

Is there a reason why we do not make (our) CameraController a sub class of QAbstractCameraController?

I like your question. Before I intended to not repeat functionality of Qt in our code. But now since I already duplicated almost all properties to expose it to python, we can remove mixin and make it as you suggest.

Either way works for me. Qt prefers inheritance, but HPC code usually uses templates and/or mixins. There is probably not a definite answer for everything. But for the Qt code here, inheritance might not be wrong.

Tucchhaa · 2024-08-23T20:00:56Z

cpp/modmesh/view/RCameraController.cpp

@@ -212,6 +212,18 @@ void RCameraInputListener::initKeyboardListeners() const
    m_ty_axis->addInput(m_keyboard_ty_neg_input);
 }

+void CameraController::reset()
+{
+    camera()->lens()->setPerspectiveProjection(45.0f, camera()->lens()->aspectRatio(), 0.1f, 1000.0f);


Now don't reset aspectRatio. Because if resize a window and reset, then mesh will look strange. Qt automatically already updates aspectRatio when window is resized

Better to have a comment to explain what is 45.0, 0.1, and 1000.0. Or, you can have constexpr float angle = 45.0f, which can better explain the code.

It is a good idea to add local constexpr for constants. It will be better document than comments.

Angles need special treatment. If it's in degree, the (local) name should contain it like front_deg. If it's in radian, the name should contain it accordingly.

It's impossible to exclusively use either degree or radian for angles. Both are needed by different libraries. Conversion is a must. Local variable names should make it clear which unit is used.

Names for member data and function parameters of angles are trickier.

Tucchhaa · 2024-08-23T20:01:29Z

cpp/modmesh/view/RCameraController.hpp

+    QVector3D defaultPosition() const { return m_default_position; }
+    void setDefaultPosition(QVector3D value) { m_default_position = value; }
+
+    QVector3D defaultViewCenter() const { return m_default_view_center; }
+    void setDefaultViewCenter(QVector3D value) { m_default_view_center = value; }
+
+    QVector3D defaultUpVector() const { return m_default_up_vector; }
+    void setDefaultUpVector(QVector3D value) { m_default_up_vector = value; }
+
+    float defaultLinearSpeed() const { return m_default_linear_speed; }
+    void setDefaultLinearSpeed(float value) { m_default_linear_speed = value; }
+
+    float defaultLookSpeed() const { return m_default_look_speed; }
+    void setDefaultLookSpeed(float value) { m_default_look_speed = value; }


new props in CameraController to control reset behaviour

Tucchhaa · 2024-08-23T20:02:33Z

cpp/modmesh/view/wrap_view.cpp

-                    bool shift_key,
-                    float dt)


remove dt parameter, because no need of it in python.

Tucchhaa · 2024-08-23T20:03:20Z

cpp/modmesh/view/wrap_view.cpp

-            .def(
-                "resetCamera",
-                [](wrapped_type & self)
-                {
-                    self.resetCamera();
-                })
-            .def("cameraController", &wrapped_type::cameraController);
-
-#define DECL_QVECTOR3D_PROPERTY(NAME, GETTER, SETTER)          \
-    .def_property(                                             \
-        #NAME,                                                 \
-        [](wrapped_type & self)                                \
-        {                                                      \
-            QVector3D const v = self.camera()->GETTER();       \
-            return py::make_tuple(v.x(), v.y(), v.z());        \
-        },                                                     \
-        [](wrapped_type & self, std::vector<double> const & v) \
-        {                                                      \
-            double const x = v.at(0);                          \
-            double const y = v.at(1);                          \
-            double const z = v.at(2);                          \
-            self.camera()->SETTER(QVector3D(x, y, z));         \
-        })
-
-        (*this)
-            // clang-format off
-            DECL_QVECTOR3D_PROPERTY(position, position, setPosition)
-            DECL_QVECTOR3D_PROPERTY(up_vector, upVector, setUpVector)
-            DECL_QVECTOR3D_PROPERTY(view_center, viewCenter, setViewCenter)
-            // clang-format on
-            ;
-
-#undef DECL_QVECTOR3D_PROPERTY


this is now available through widget.camera

Tucchhaa · 2024-08-23T20:04:03Z

cpp/modmesh/view/wrap_view.cpp

+#define DECL_QVECTOR3D_PROPERTY(NAME, GETTER, SETTER)          \
+    .def_property(                                             \
+        #NAME,                                                 \
+        [](wrapped_type & self)                                \
+        {                                                      \
+            QVector3D const v = self.GETTER();                 \
+            return py::make_tuple(v.x(), v.y(), v.z());        \
+        },                                                     \
+        [](wrapped_type & self, std::vector<double> const & v) \
+        {                                                      \
+            double const x = v.at(0);                          \
+            double const y = v.at(1);                          \
+            double const z = v.at(2);                          \
+            self.SETTER(QVector3D(x, y, z));                   \
+        })
+
+        (*this)
+            // clang-format off
+                    DECL_QVECTOR3D_PROPERTY(position, position, setPosition)
+                    DECL_QVECTOR3D_PROPERTY(up_vector, upVector, setUpVector)
+                    DECL_QVECTOR3D_PROPERTY(view_center, viewCenter, setViewCenter)
+                    DECL_QVECTOR3D_PROPERTY(default_position, defaultPosition, setDefaultPosition)
+                    DECL_QVECTOR3D_PROPERTY(default_view_center, defaultViewCenter, setDefaultViewCenter)
+                    DECL_QVECTOR3D_PROPERTY(default_up_vector, defaultUpVector, setDefaultUpVector)
+            // clang-format on
+            ;
+#undef DECL_QVECTOR3D_PROPERTY
+
+#define DECL_FLOAT_PROPERTY(NAME, GETTER, SETTER) \
+    .def_property(                                \
+        #NAME,                                    \
+        [](wrapped_type & self)                   \
+        {                                         \
+            return self.GETTER();                 \
+        },                                        \
+        [](wrapped_type & self, float v)          \
+        {                                         \
+            self.SETTER(v);                       \
+        })
+
+        (*this)
+            // clang-format off
+                DECL_FLOAT_PROPERTY(linear_speed, linearSpeed, setLinearSpeed)
+                DECL_FLOAT_PROPERTY(look_speed, lookSpeed, setLookSpeed)
+                DECL_FLOAT_PROPERTY(default_linear_speed, defaultLinearSpeed, setDefaultLinearSpeed)
+                DECL_FLOAT_PROPERTY(default_look_speed, defaultLookSpeed, setDefaultLookSpeed)
+            // clang-format on
+            ;
+#undef DECL_FLOAT_PROPERTY


Now use macro instead of defining each getter and setter

Long live C pre-processor. It is very handy in C++.

Tucchhaa · 2024-08-23T20:04:43Z

tests/test_view.py

+@unittest.skipIf(GITHUB_ACTIONS, "GUI is not available in GitHub Actions")
+class ViewCommonCameraTC(ViewCameraTB, unittest.TestCase):
+    camera_type = "fps"  # no difference when use orbit camera
+
+    def setUp(self):
+        self.camera.reset()
+
+    def test_value_get_set(self):
+        c = self.camera
+
+        c.linear_speed = 123.0
+        self.assertEqual(c.linear_speed, 123.0)
+
+        c.look_speed = 456.0
+        self.assertEqual(c.look_speed, 456.0)
+
+    def test_vector_get_set(self):
+        c = self.camera
+
+        c.position = (1, 2, 3)
+        c.view_center = (4, 5, 6)
+        c.up_vector = (7, 8, 9)
+
+        self.assertEqual(c.position, (1, 2, 3))
+        self.assertEqual(c.view_center, (4, 5, 6))
+        self.assertEqual(c.up_vector, (7, 8, 9))
+
+    def test_default_values(self):
+        c = self.camera
+
+        c.default_position = (1, 2, 3)
+        c.default_view_center = (4, 5, 6)
+        c.default_up_vector = (7, 8, 9)
+        c.default_linear_speed = 123.0
+        c.default_look_speed = 456.0
+
+        c.reset()
+
+        self.assertEqual(c.position, (1, 2, 3))
+        self.assertEqual(c.view_center, (4, 5, 6))
+        self.assertEqual(c.up_vector, (7, 8, 9))
+        self.assertEqual(c.linear_speed, 123.0)
+        self.assertEqual(c.look_speed, 456.0)
+
+


test common things of both camera types. For now it's default properties

Tucchhaa · 2024-08-23T20:05:55Z

tests/test_view.py

-        initial_view_vector = self.view_vector()
-        initial_view_center = self.view_center()
-        initial_up_vector = self.up_vector()
+        c = self.camera


refactored all camera tests:

removed usage of dt in camera.move function

use properties instead of getters and setters

Tucchhaa · 2024-08-23T20:07:06Z

@yungyuc ready for review

tigercosmos · 2024-08-24T01:18:52Z

cpp/modmesh/view/RCameraController.hpp

+    RCameraInputListener * m_listener = nullptr;
+
+private:
+    QVector3D m_default_position = QVector3D(0.0f, 0.0f, 10.0f);


Why (0, 0, 10)?

Because meshes are placed at (0, 0, 0), and we don't want the camera to be inside the mesh

I can guess that, but my point is, better to have some comments for it.

Because meshes are placed at (0, 0, 0), and we don't want the camera to be inside the mesh

To put the camera outside the mesh, it is not good to hard-code the default value of the member m_default_position, because the mesh may be placed anywhere.

We do not have a reasonably large number of mesh examples for the default location to show disadvantage.

I can guess that, but my point is, better to have some comments for it.

I concur, but it's OK to leave it here and deal with it in the future. As we make the code more useful, some meshes will cover the position sooner or later. We will immediately know and fix it.

tigercosmos · 2024-08-24T01:20:51Z

cpp/modmesh/view/wrap_view.cpp

+            ;
+#undef DECL_QVECTOR3D_PROPERTY
+
+#define DECL_FLOAT_PROPERTY(NAME, GETTER, SETTER) \


The macro naming doesn't follow the style rules.
https://github.com/solvcon/modmesh/blob/master/STYLE.rst#c-pre-processor-macro

tigercosmos · 2024-08-24T01:22:15Z

tests/test_view.py


    def test_rotation(self):
-        dt = 0.01
-        angle = dt * self.look_speed()
+        c = self.camera


Personally, I don't like a shorthand like c, but maybe yyc has a different thought.

I did that due to flake8's rule to limit lines to 80 characters. If I write self.camera or camera everywhere I will get a lot of errors and I will have to shorten more other variables :/

A local c in test code is good. It can't be right to skim through testing code for a complex structure for 3D camera with only a glance. Writing c = self.camera forces a maintainer to pause for a second to keep the information in their brain's short-term memory block, and improves their understanding of the code.

yungyuc

In this PR:

New props added to camera controller to control it's reset behaviour: defaultPosition, defaultUpVector, defaultViewCenter, defaultLookSpeed, defaultLinearSpeed

All these props are exposed to python

Camera controller python API was rewritten to use def_property using macros. Tests also were updated

Minor issue was fixed: When reset camera, aspect ratio was incorrect

The new Python wrapper and tests are great.

It is unfortunate we cannot execute the tests with github actions. It's an improvement we should make in the future.

Clarify why Qt3DExtras::QAbstractCameraController * qtController() const is needed.
Is there a reason why we do not make (our) CameraController a sub class of QAbstractCameraController? (See discussion at setCameraController()).
Fix the naming of #define DECL_FLOAT_PROPERTY(NAME, GETTER, SETTER): Follow the macro naming style https://github.com/solvcon/modmesh/blob/master/STYLE.rst#c-pre-processor-macro
Clarify why change delta to places for assertAlmostEqual()?
Test both fps and orbit cameras.

yungyuc · 2024-08-24T02:04:46Z

cpp/modmesh/view/R3DWidget.cpp

-void R3DWidget::resetCamera() const
-{
-    Qt3DRender::QCamera * camera = m_view->camera();
-
-    // Set up the camera.
-    camera->lens()->setPerspectiveProjection(45.0f, 16.0f / 9.0f, 0.1f, 1000.0f);
-    camera->setPosition(QVector3D(0.0f, 0.0f, 10.0f));
-    camera->setViewCenter(QVector3D(0.0f, 0.0f, 0.0f));
-    camera->setUpVector(QVector3D(0.f, 1.f, 0.f));
-
-    // Set up the camera control.
-    auto * control = m_scene->controller();
-    control->setCamera(camera);
-    control->setLinearSpeed(50.0f);
-    control->setLookSpeed(180.0f);
-}
-


Moving it to CameraController makes it more testable. I like it.

yungyuc · 2024-08-24T02:05:35Z

cpp/modmesh/view/R3DWidget.hpp

@@ -46,24 +46,22 @@
 namespace modmesh
 {

-class RScene
-    : public Qt3DCore::QEntity
+class RScene : public Qt3DCore::QEntity


Did clang-format force you to make it in one line: class RScene : public Qt3DCore::QEntity?

If not, please change it back to use the two lines and avoid this unnecessary change of code.

yungyuc · 2024-08-24T02:07:14Z

cpp/modmesh/view/R3DWidget.hpp

-        : Qt3DCore::QEntity(parent)
-        , m_controller(new ROrbitCameraController(this))
+    explicit RScene(QNode * parent = nullptr)
+        : QEntity(parent)


It's good to separate the CameraController from the scene.

yungyuc · 2024-08-24T02:08:40Z

cpp/modmesh/view/R3DWidget.hpp

-    Qt3DExtras::QAbstractCameraController const * controller() const { return m_controller; }
-    Qt3DExtras::QAbstractCameraController * controller() { return m_controller; }
+    CameraController * controller() const { return m_controller; }
+    Qt3DExtras::QAbstractCameraController * qtController() const { return m_controller->asQtCameraController(); }


Why do we need an helper qtController() for casting? Is there a reason that the consumers cannot do it themselves?

Qt3DExtras::QAbstractCameraController * qtc = scene->controller()->asQtCameraController();

If you want to avoid Qt3DExtras::QAbstractCameraController const *, an overload of CameraController * controller() should work.

I'll make CameraController inherit QAbstractCameraController, so qtController will be removed

yungyuc · 2024-08-24T02:15:54Z

cpp/modmesh/view/R3DWidget.hpp

 public:

-    explicit RScene(Qt3DCore::QNode * parent = nullptr)
-        : Qt3DCore::QEntity(parent)
-        , m_controller(new ROrbitCameraController(this))
+    explicit RScene(QNode * parent = nullptr)
+        : QEntity(parent)
    {
+        m_controller = new ROrbitCameraController(this);
    }

-    Qt3DExtras::QAbstractCameraController const * controller() const { return m_controller; }
-    Qt3DExtras::QAbstractCameraController * controller() { return m_controller; }
+    CameraController * controller() const { return m_controller; }
+    Qt3DExtras::QAbstractCameraController * qtController() const { return m_controller->asQtCameraController(); }

-    void setCameraController(Qt3DExtras::QAbstractCameraController * controller)
+    void setCameraController(CameraController * controller)
    {
-        m_controller->deleteLater();
+        qtController()->deleteLater();


Is there a reason why we do not make (our) CameraController a sub class of QAbstractCameraController?

yungyuc · 2024-08-24T02:21:32Z

cpp/modmesh/view/RCameraController.cpp

@@ -212,6 +212,18 @@ void RCameraInputListener::initKeyboardListeners() const
    m_ty_axis->addInput(m_keyboard_ty_neg_input);
 }

+void CameraController::reset()
+{
+    camera()->lens()->setPerspectiveProjection(45.0f, camera()->lens()->aspectRatio(), 0.1f, 1000.0f);


It is a good idea to add local constexpr for constants. It will be better document than comments.

Angles need special treatment. If it's in degree, the (local) name should contain it like front_deg. If it's in radian, the name should contain it accordingly.

It's impossible to exclusively use either degree or radian for angles. Both are needed by different libraries. Conversion is a must. Local variable names should make it clear which unit is used.

Names for member data and function parameters of angles are trickier.

yungyuc · 2024-08-24T02:25:19Z

cpp/modmesh/view/wrap_view.cpp

+#define DECL_QVECTOR3D_PROPERTY(NAME, GETTER, SETTER)          \
+    .def_property(                                             \
+        #NAME,                                                 \
+        [](wrapped_type & self)                                \
+        {                                                      \
+            QVector3D const v = self.GETTER();                 \
+            return py::make_tuple(v.x(), v.y(), v.z());        \
+        },                                                     \
+        [](wrapped_type & self, std::vector<double> const & v) \
+        {                                                      \
+            double const x = v.at(0);                          \
+            double const y = v.at(1);                          \
+            double const z = v.at(2);                          \
+            self.SETTER(QVector3D(x, y, z));                   \
+        })
+
+        (*this)
+            // clang-format off
+                    DECL_QVECTOR3D_PROPERTY(position, position, setPosition)
+                    DECL_QVECTOR3D_PROPERTY(up_vector, upVector, setUpVector)
+                    DECL_QVECTOR3D_PROPERTY(view_center, viewCenter, setViewCenter)
+                    DECL_QVECTOR3D_PROPERTY(default_position, defaultPosition, setDefaultPosition)
+                    DECL_QVECTOR3D_PROPERTY(default_view_center, defaultViewCenter, setDefaultViewCenter)
+                    DECL_QVECTOR3D_PROPERTY(default_up_vector, defaultUpVector, setDefaultUpVector)
+            // clang-format on
+            ;
+#undef DECL_QVECTOR3D_PROPERTY
+
+#define DECL_FLOAT_PROPERTY(NAME, GETTER, SETTER) \
+    .def_property(                                \
+        #NAME,                                    \
+        [](wrapped_type & self)                   \
+        {                                         \
+            return self.GETTER();                 \
+        },                                        \
+        [](wrapped_type & self, float v)          \
+        {                                         \
+            self.SETTER(v);                       \
+        })
+
+        (*this)
+            // clang-format off
+                DECL_FLOAT_PROPERTY(linear_speed, linearSpeed, setLinearSpeed)
+                DECL_FLOAT_PROPERTY(look_speed, lookSpeed, setLookSpeed)
+                DECL_FLOAT_PROPERTY(default_linear_speed, defaultLinearSpeed, setDefaultLinearSpeed)
+                DECL_FLOAT_PROPERTY(default_look_speed, defaultLookSpeed, setDefaultLookSpeed)
+            // clang-format on
+            ;
+#undef DECL_FLOAT_PROPERTY


Long live C pre-processor. It is very handy in C++.

yungyuc · 2024-08-24T02:25:43Z

tests/test_view.py

@@ -30,6 +30,7 @@
 import os

 import modmesh
+


Please remove the unnecessary change of blank line.

yungyuc · 2024-08-24T02:27:59Z

tests/test_view.py

-        self.assertAlmostEqual(rotated_vector[0], view_vector[0], delta=1e-5)
-        self.assertAlmostEqual(rotated_vector[1], view_vector[1], delta=1e-5)
-        self.assertAlmostEqual(rotated_vector[2], view_vector[2], delta=1e-5)
+        self.assertAlmostEqual(rotated_vector[0], c.view_vector[0], places=5)


delta is clearer than places. Why change to the latter?

Because flake8's limitation to 80 characters makes me do weird things 😵. places=5 is two characters less thandelta=1e-5

That's a clear and valid reason. But it is also unfortunate 🤣.

Could you please rename rotated_vector as rot_vec or even rvec, and change back to use delta=1.e-5? Using delta gives us more correct behaviors. We will copy and paste the code for future testing. Behaviorally correct code gives us longer mileage.

We cannot change the argument name nor the float literals, but modmesh developers have the constitutive right to rename local variables. (We have not written how to treat local variable names in the style guide, but we should.)

yungyuc · 2024-08-24T02:32:36Z

tests/test_view.py

@@ -111,190 +103,231 @@ def normalize(self, vec):
        return vec / np.linalg.norm(vec)


+@unittest.skipIf(GITHUB_ACTIONS, "GUI is not available in GitHub Actions")
+class ViewCommonCameraTC(ViewCameraTB, unittest.TestCase):
+    camera_type = "fps"  # no difference when use orbit camera


I believe this comment suggests they behave the same in theory. If that is true, both fps and orbit cameras should be tested. Testing is required to make sure the theory is always the reality.

You may use the TB/TC pattern to test both.

Tucchhaa · 2024-08-24T20:14:06Z

In last commit:

Used constexpr for 'magic values' in reset function (code)
Made CameraController to inherit from QAbstractCameraController (code) and removed all now unnecessary functions, like qtCameraControler. And getters and setters from FPS and Orbit subclasses moved to CameraController
Fixed formatting issues
Test for both fps and orbit camera types in ViewCommonCameraTC (code)

@yungyuc @tigercosmos ready for your review 🙂

yungyuc

Thanks for clarifying the many things swiftly. You updated pretty quickly! Not sure what time zone you are in 🙂

Additional requests after that:

RCameraController.hpp:129: Rename CameraController to RCameraController.
test_view.py:225: Rename rotated_vector as rot_vec or something like rvec, and change back to use delta=1.e-5.
test_view.py:158: Is it possible to merge the testing code in ViewFPSCameraTC and ViewOrbitCameraTC to a base class (*TB)? (If it is too cumbersome to do it now or you don't like a *TB, we can leave the duplicated code here for now.)

yungyuc · 2024-08-25T07:08:56Z

cpp/modmesh/view/RCameraController.hpp

+    RCameraInputListener * m_listener = nullptr;
+
+private:
+    QVector3D m_default_position = QVector3D(0.0f, 0.0f, 10.0f);


Because meshes are placed at (0, 0, 0), and we don't want the camera to be inside the mesh

To put the camera outside the mesh, it is not good to hard-code the default value of the member m_default_position, because the mesh may be placed anywhere.

We do not have a reasonably large number of mesh examples for the default location to show disadvantage.

I can guess that, but my point is, better to have some comments for it.

I concur, but it's OK to leave it here and deal with it in the future. As we make the code more useful, some meshes will cover the position sooner or later. We will immediately know and fix it.

yungyuc · 2024-08-25T07:13:50Z

cpp/modmesh/view/RCameraController.hpp

@@ -128,77 +126,95 @@ class RCameraInputListener : public Qt3DCore::QEntity
    Qt3DInput::QButtonAxisInput * m_keyboard_tz_neg_input;
 }; /* end class RCameraInputListener */

-class CameraController
+class CameraController : public Qt3DExtras::QAbstractCameraController


This needs to be named as RCameraController because it is a Qt class customized (derived) in modmesh. See https://github.com/solvcon/modmesh/blob/master/STYLE.rst#qt for the modmesh Qt style.

yungyuc · 2024-08-25T07:14:45Z

cpp/modmesh/view/RCameraController.hpp


-    virtual void updateCameraPosition(const CameraInputState & state, float dt) = 0;
+    // Do nothing in QAbstractCameraController's moveCamera


Thanks for the informative comment. It will work even better to put the comment in the function body:

{ // Do nothing in QAbstractCameraController's moveCamera }

yungyuc · 2024-08-25T07:23:49Z

tests/test_view.py

-        self.assertAlmostEqual(rotated_vector[0], view_vector[0], delta=1e-5)
-        self.assertAlmostEqual(rotated_vector[1], view_vector[1], delta=1e-5)
-        self.assertAlmostEqual(rotated_vector[2], view_vector[2], delta=1e-5)
+        self.assertAlmostEqual(rotated_vector[0], c.view_vector[0], places=5)


That's a clear and valid reason. But it is also unfortunate 🤣.

Could you please rename rotated_vector as rot_vec or even rvec, and change back to use delta=1.e-5? Using delta gives us more correct behaviors. We will copy and paste the code for future testing. Behaviorally correct code gives us longer mileage.

We cannot change the argument name nor the float literals, but modmesh developers have the constitutive right to rename local variables. (We have not written how to treat local variable names in the style guide, but we should.)

yungyuc · 2024-08-25T07:32:31Z

tests/test_view.py

+            self.assertEqual(c.linear_speed, 123.0)
+            self.assertEqual(c.look_speed, 456.0)
+
+
 @unittest.skipIf(GITHUB_ACTIONS, "GUI is not available in GitHub Actions")
 class ViewFPSCameraTC(ViewCameraTB, unittest.TestCase):
    camera_type = "fps"


The testing code in ViewFPSCameraTC and ViewOrbitCameraTC looks very similar if not the same. Is it possible to merge to a base class (*TB)?

test_rest, test_rotation are actually different and test_translation indeed is the same in both fps and orbit. But it's not easy to move common tests to the base class, because unittest can't understand that structure.

I will move test_translation to ViewCommonCameraTC

It's good to move identical testing code to a single place. Thanks.

A side note. For testing, it is not rare to duplicate code intentionally. Compared to "feature" code, we more often change the testing code for experimenting without checking in the change (we want the testing code to be slightly more stable than the feature code).

In this PR, I do not see a reason to duplicate testing code. This is just FYI.

Tucchhaa · 2024-08-25T11:04:29Z

You updated pretty quickly! Not sure what time zone you are in 🙂

It's only 2 hours earlier for me. Btw, I will arrive to Taiwan very soon)

yungyuc · 2024-08-25T11:20:33Z

You updated pretty quickly! Not sure what time zone you are in 🙂

It's only 2 hours earlier for me. Btw, I will arrive to Taiwan very soon)

Around the end of the summer vacation one can be anywhere. I hope you had a good time this summer. Having said that, we will be more than happy to share some good ideas of traveling!

Tucchhaa · 2024-08-25T11:32:18Z

In last PR:

Renamed CameraController to RCameraController
moved duplicated test function test_translate to ViewCommonCameraTC
Replaced places=5 to delta=1e-5

Tucchhaa · 2024-08-25T11:34:01Z

@yungyuc
I have a question. Does it matter how we name commits? I try to give them meaningful names, but sometimes it's hard to do that. Like last to commits are just 'apply review' and 'apply review 2'. Is it ok for modmesh?

yungyuc · 2024-08-25T11:39:04Z

I'll review the code after the CI completes.

In last PR:

I think you meant to say commit (or update to the PR). But I am nitpicking. If you like to write accurately, it's a reminder. Whatever term you use does not bother me.

@yungyuc I have a question. Does it matter how we name commits? I try to give them meaningful names, but sometimes it's hard to do that. Like last to commits are just 'apply review' and 'apply review 2'. Is it ok for modmesh?

It depends. For a non-trivial enhancement like this PR, some mouthy commits are fine. But in general I do not want to be picky on commit logs. I guess I am "just reasonably picky". We'll see.

yungyuc

LGTM to me. Missed class ending marks (e.g., wrap_view.cpp:521) may be addressed in the future.

yungyuc · 2024-08-25T13:01:47Z

cpp/modmesh/view/wrap_view.cpp

+                MM_DECL_FLOAT_PROPERTY(default_look_speed, defaultLookSpeed, setDefaultLookSpeed)
+            // clang-format on
+            ;
+#undef MM_DECL_FLOAT_PROPERTY
    }
 };


Class ending mark is not here. It should be added in the future.

Tucchhaa added 5 commits August 23, 2024 19:17

expose complete camera controller to python + tests

c288a98

camera default values api + test

dcf2bc8

refactor cameraController getters

f527710

do not change aspect ration of camera when reest

24f2312

use properties in camera python api

09842d0

Tucchhaa force-pushed the expose_camera_api_python branch from 8aa25be to 09842d0 Compare August 23, 2024 19:50

Tucchhaa commented Aug 23, 2024

View reviewed changes

Tucchhaa marked this pull request as ready for review August 23, 2024 20:06

tigercosmos reviewed Aug 24, 2024

View reviewed changes

yungyuc requested changes Aug 24, 2024

View reviewed changes

yungyuc assigned Tucchhaa Aug 24, 2024

yungyuc added viewer Visualize stuff enhancement New feature or request labels Aug 24, 2024

apply review

3adeb70

Tucchhaa requested review from yungyuc and tigercosmos August 24, 2024 20:14

yungyuc requested changes Aug 25, 2024

View reviewed changes

yungyuc mentioned this pull request Aug 25, 2024

Improve camera control #70

Closed

2 tasks

apply review 2

42161b5

yungyuc approved these changes Aug 25, 2024

View reviewed changes

yungyuc merged commit f7195c7 into solvcon:master Aug 25, 2024
12 checks passed

yungyuc linked an issue Aug 25, 2024 that may be closed by this pull request

Improve camera control #70

Closed

2 tasks


		virtual void updateCameraPosition(const CameraInputState & state, float dt) = 0;
		// Do nothing in QAbstractCameraController's moveCamera

Camera Controller: expose complete API to python #418

Camera Controller: expose complete API to python #418

Conversation

Tucchhaa commented Aug 23, 2024 • edited Loading

Tucchhaa Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tucchhaa Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tucchhaa Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

tigercosmos Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tucchhaa Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tucchhaa Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Tucchhaa commented Aug 23, 2024

tigercosmos Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Tucchhaa Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tucchhaa commented Aug 24, 2024

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

yungyuc Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tucchhaa Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tucchhaa commented Aug 25, 2024

yungyuc commented Aug 25, 2024

Tucchhaa commented Aug 25, 2024

Tucchhaa commented Aug 25, 2024

yungyuc commented Aug 25, 2024

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tucchhaa commented Aug 23, 2024 •

edited

Loading

Tucchhaa Aug 23, 2024 •

edited

Loading

Tucchhaa Aug 23, 2024 •

edited

Loading

Tucchhaa Aug 23, 2024 •

edited

Loading

tigercosmos Aug 24, 2024 •

edited

Loading

Tucchhaa Aug 23, 2024 •

edited

Loading

Tucchhaa Aug 23, 2024 •

edited

Loading

tigercosmos Aug 24, 2024 •

edited

Loading

Tucchhaa Aug 24, 2024 •

edited

Loading

yungyuc Aug 25, 2024 •

edited

Loading

yungyuc left a comment •

edited

Loading

yungyuc left a comment •

edited

Loading

yungyuc Aug 25, 2024 •

edited

Loading

Tucchhaa Aug 25, 2024 •

edited

Loading

yungyuc left a comment •

edited

Loading