4
\$\begingroup\$

I want to make a camera class with help of the Eigen library. The camera will be used to perform certain calculations or rather the values used to setup the camera will be used later on in multiple matrix calculations. The camera can be defined by the 9D vector and the near/far plane and width/height values alone.
Redundant definitions like a and c are just for convenience as those are the mathematical symbols we use for those vectors.

Main point to be reviewed:
I am not sure if and how to store dependent values that can be derived from other data but will be accessed frequently.
Context:
Derived values like position, up vector, forward vector etc will be needed often, so I figured I store them as well. But whenever one wants to change one of those, the others will have to be recalculated. If I don't store them they will have to be constructed every time I need them. In the big picture, there will be an optimization problem to solve, so the camera will change often.
In this case, would it be more efficient to store them or construct them if needed? If stored, might it be better to use pointers?

I have programming experience in C# but C++ is something I struggle with, especially when to use a pointer, references or value. I am also not sure how to use the Eigen types in an efficient way. I read a little in the documentation but am still unsure.
It currently works as intended but I have concerns it will be either slow or memory consuming if used intensively.

I am using g++ 7.4.0, C++11, Eigen (master branch, as I like the new matrix slicing syntax and the 3.4 seems not so far in the future any more)

Any general advice and references are appreciated as well. Thanks :)

Edit1: Added clarity Edit2: Added example

matrix_types.h

#ifndef MATRIX_TYPES_H_
#define MATRIX_TYPES_H_

#include<Eigen/Dense>

typedef Eigen::Matrix<double, 9, 1> Vector9d;
typedef Eigen::Matrix<double, 9, 2> Matrix92d;
typedef Eigen::Matrix<double, 2, 9> Matrix29d;
typedef Eigen::Matrix<double, 9, 9> Matrix9d;

typedef Eigen::Matrix<double, 3, 2> Matrix32d;
typedef Eigen::Matrix<double, 2, 3> Matrix23d;

#endif

Camera.h

#ifndef CAMERA_H_
#define CAMERA_H_

#include <Eigen/Dense>
#include <Eigen/Geometry>
#include "matrix_types.h"

class Camera
{
public:

    EIGEN_MAKE_ALIGNED_OPERATOR_NEW
    typedef Eigen::Vector3d Vector3d;

    Camera(
        const Vector9d& camVector,
        float width,
        float height,
        float near,
        float far ) 
        : 
        m_width(width),
        m_height(height),
        m_near(near),
        m_far(far)
    {
        m_setCamVec(camVector);
    }

    Camera(
        const Vector3d& pos,
        const Vector3d& forward,
        const Vector3d& up,
        float width,
        float height,
        float near,
        float far ) 
        : 
        m_width(width),
        m_height(height),
        m_near(near),
        m_far(far)
    {
        m_setCamVec(pos, forward, up);
    }

    const Vector3d& Position(){
        return m_position;
    };
    const Vector3d& c(){
        return Position();
    }

    const Vector3d& Forward(){
        return m_forward;
    };

    const Vector3d& a(){
        return Forward();
    }

    const Vector3d& up(){
        return m_up;
    };

    Matrix32d A(){
        Matrix32d n_A;
        n_A << m_forward, m_forward;
        return n_A;
    }

    Matrix32d R11(){
        Matrix32d n_R11;
        n_R11 << m_r1, m_r1;
        return n_R11;
    }

    Matrix32d R12(){
        Matrix32d n_R12;
        n_R12 << m_r1, m_r2;
        return n_R12;
    }

    float AspectRatio(){
        return m_width/m_height;
    }

    float r(){
        return AspectRatio();
    }

    /// ()-operator so one can do camera() and get the Vector9 of its components
    const Vector9d& operator () (){
        return m_camVec;
    }

    /// assignment operator
    Camera& operator = (const Vector9d& camVec){
        m_setCamVec(camVec);
        return *this;
    }

    Camera& operator += (const Vector9d& camOffset){
        m_setCamVec(m_camVec + camOffset);
        return *this;
    }

    Camera& operator -= (const Vector9d& camOffset){
        m_setCamVec(m_camVec - camOffset);
        return *this;
    }

    void SetPosition(const Vector3d& pos){
        m_position = pos;
        m_camVec(Eigen::seq(0,2)) = pos;
    }

    void SetForward(const Vector3d& forw){
        m_forward = forw;
        m_camVec(Eigen::seq(3,5)) = forw;
        m_recalculateRs();
    }

    void SetUp(const Vector3d& up){
        m_up = up;
        m_camVec(Eigen::seq(6,8)) = up;
        m_recalculateRs();
    }

private:
    Vector9d m_camVec;

    Vector3d m_position;
    Vector3d m_forward;
    Vector3d m_up;

    Vector3d m_r1;
    Vector3d m_r2;

    float m_width;
    float m_height;

    float m_near;
    float m_far;

    void m_setCamVec(const Vector9d& camVec){
        m_camVec = camVec;

        m_position = m_camVec(Eigen::seq(0,2));
        m_forward = m_camVec(Eigen::seq(3,5));
        m_up = m_camVec(Eigen::seq(6,8));
        m_recalculateRs();
    }

    void m_setCamVec(const Vector3d& pos, const Vector3d& forw, const Vector3d& up){
        m_camVec << pos, forw, up;
        m_position = pos;
        m_forward = forw;
        m_up = up;
        m_recalculateRs();
    }

    void m_recalculateRs(){
        m_r1 = (m_forward.cross(m_up)).normalized();
        m_r2 = (m_r1.cross(m_forward)).normalized();
    }

}; // end Camera

#endif

main.cpp

/*
The actual calculations here are nonsensical.

Still, the use of the camera here is similar to 
the way it will be used later on. Meaning, matrices
and camera vectors will be used for calculations.

The camera will be part of a bigger project and 
I want to avoid rewriting everything because it 
was set up stupidly to begin with.
*/

#include <iostream>
#include <cmath>
#include <Eigen/Dense>
#include <limits>
#include "camera.h"

int main()
{
    Vector9d camVec {{1.0},{1.0},{5.0},{0.0},{1.0},{0.2},{0.0},{0.0},{1.0}};
    Camera cam(camVec, 12.3f, 5.4f, 1.0f, 300.0f);

    double epsilon = 0.0001;

    Vector9d bestCamVec = camVec;

    double opt = std::numeric_limits<double>::max();

    for(int j = 0; j < 100; ++j){

        Matrix9d P = Matrix9d::Constant(0);
        Vector9d camEps = Vector9d::Random() * epsilon;
        cam += camEps;

        for(int i = 0; i < 1000; ++i){

            Eigen::Vector2d p = Eigen::Vector2d::Random();
            double d = p.norm();

            double alpha = std::atan2(cam.a().cross(cam.up()).norm(), cam.a().dot(cam.up()));
            double a = cam.a().norm();
            double b = (cam.a() + cam.R12()*p).norm();

            Eigen::Matrix2d Y  = p.asDiagonal();
            Eigen::Matrix2d Y_ = Y.inverse();

            Matrix92d B;
            B << b/(d*a*a)*cam.A()*Y_, -std::tan(alpha)/a*cam.R11()*Y, cam.R12()*Y*std::sin(alpha);
            P += B*B.transpose();
        }

        double det = std::abs(P.determinant());
        if(opt > det){
            opt = det;
            bestCamVec = cam();
        }
    }

    std::cout << "Best camera vector is \n" << bestCamVec << "\n with " << opt;
}
```
\$\endgroup\$
5
  • \$\begingroup\$ @Lisa I have installed EIgen and compilled your code. Caveat: I am well familiar with Linear algebra but I do not know Eigen. There are some unused member variables: m_near and m_far (turn on -Wall -Wextra). I have had a look at how Eigen stores these vectors. They seem to be just "value types", ie an Eigen::Vector3d is literally 3 doubles (ie 24 bytes) on the stack. If, from my cursory glance, i have understood correctly, then you are worried about recomputing/storing m_r1 & m_r2. Don't be, they are only of type Vector3d. \$\endgroup\$ Jan 29 '20 at 3:13
  • 1
    \$\begingroup\$ @Lisa Please provide a main() which shows how you intend to use Camera and I can give a fuller answer. I suspect the answer is: you are prematurely worrying about copying/pointers etc. C++ is extremely good and fast at "value types". So at 2x 24bytes I wouldn't worry too much at all until you have a real example to test with. Why are m_width and m_height and the resulting AspectRatio() of type float? When everything else is a double? I would just stick to double. With H/W acceleration they are almost the same speed and there is probably no issue with storage here. \$\endgroup\$ Jan 29 '20 at 3:17
  • \$\begingroup\$ @OliverSchonrock Added an example. Sorry it took so long. You are right, making it all to double type makes more sense \$\endgroup\$
    – LivelyLiz
    Jan 29 '20 at 18:36
  • \$\begingroup\$ @Lisa. Thanks for the example. Initially, I failed to read the comment at top. Wrecked my brain for 5mins trying to understand what it does, before concluding "either the code is crazy or I am". Glad it's the former. ;-) \$\endgroup\$ Jan 29 '20 at 19:30
  • \$\begingroup\$ @Lisa I added some notes about making getters const and [[nodiscard]] below, just for style. \$\endgroup\$ Jan 29 '20 at 19:36
5
\$\begingroup\$

Scope

I cannot fully comment on use of the class or the linear algebra involved, because:

  1. It's not clear how it will be used
  2. There are no real operations shown, just setup/config of the class plus "camera move".

General

  • Code compiles fine on a recent compiler (I am using clang-9 ). Only 2 warnings with -Wall -Wextra:
camera.cpp:110:9: warning: private field 'm_near' is not used [-Wunused-private-field]
  float m_near;
        ^
camera.cpp:111:9: warning: private field 'm_far' is not used [-Wunused-private-field]
  float m_far;
        ^
  • Given you are using gcc7.4 and that I assume you have -std=C++17 enabled, then I do not believe you need EIGEN_MAKE_ALIGNED_OPERATOR_NEW. As documented here and explained in more detail in this bugzilla entry. All of Eigen's aligment challenges are solved in recent C++17 compilers apparently. Your question mentions C++11, but if you can you should be compiling with -std=c++17 to get best behaviour from Eigen and have access many other useful features. The only reason to not do this would be if you have some other 3rd party dependency which won't compile in C++17 mode, or if you also need to support platforms with older , more limited compilers, where C++17 is not available.
  • You are using double for all your vector/matrix coefficients, yet you are using float for m_width and m_height and therefore AspectRatio(). Is there a specific reason? If not I would just standardise on double, which can often be faster on modern hardware with FP co-processors.
  • Instead of your typedefs you could consider writing this, which is considered "more modern practice":
using Vector9d = Eigen::Matrix<double, 9, 1>;
  • Your constructors and operator methods look fine to me
  • You could get "extra points" for C++ style by making your "getter" methods const and [[nodiscard]] like this:
  [[nodiscard]] const Vector3d& Forward() const { /* ... */ };
  [[nodiscard]] const Vector3d& a() const { /* ... */ };
  [[nodiscard]] const Vector3d& up() const { /* ... */ };
  [[nodiscard]] Matrix32d A() const { /* ... */ };
  [[nodiscard]] Matrix32d R11() const { /* ... */ };
  [[nodiscard]] Matrix32d R12() const { /* ... */ };
  • When you compile for performance (ie "Release mode") ensure you pass -DNDEBUG=1 -O3 similar to below. The NDEBUG bypasses runtime checks on matrix dimensions and -O3 will bring huge benefits, especially with Eigen's expression templates.
g++ -DNDEBUG=1 -O3 -Wall -Wextra -std=c++17 -I include/eigen/ -o build/camera apps/camera.cpp

Memory structure: layout, allocation and operations

You seemed concerned about speed / copying / memory usage and therefore whether you should be using pointers.

Firstly the vectors/matrices you are using are fixed size and very very small. The biggest one is 9 elements of doubles which are 8 bytes each so 72 bytes (you don't seem to use the Matrix92d). You don't appear to have std::vectors or other collections of these vectors/matrices or of the camera object itself.

Your concern seems to be related to "moving and therefore changing the camera object often". You have decided to recompute the internal m_r1 and m_r2 during m_setCamVec which is called from eg operator-=.

I have read up on the Eigen operations and studied how Eigen stores its structures. They are very simple: essentially, under the hood they are C-style arrays. On my machine, sizeof (Camera) reports that your objects will be 208 bytes, which is very manageable on the stack. This includes all the vectors/matrices which are all packed into the object (no heap allocations here).

If you needed to pass one or a few Camera objects around between functions, you should pass them by reference. Not least because Eigen recommends that for its structures.

If you were to make thousands of Camera objects then these should probably go on the heap, and that would happen naturally if you made a std::vector<Camera> of them. But even then, each Camera object on the heap would contain all its 208 bytes of vectors/matrices within it.

When you call operator+= or other methods which reposition the Camera it looks like there are just a few assignment statements for m_position etc and then recalculateRS() which does 2 tiny cross-products and assignments.

Therefore, there is no need to worry about copying during the recalculation and re-assigning to the member fields. Apart from any possible temporaries, (which Eigen tries to minimise), Eigen and C++ will use the same memory which is already part of your 208-byte Camera object. Very little (if any!) extra memory will be used and everything (!) will be cleaned up each time you reposition the camera object.

Expected costs

This should all be super fast because:

  • You have chosen a good quality linear algebra library
  • You are using fixed sized matrices
  • Your matrices are very small
  • If you compile with -O3 -DNDEBUG=1 then Eigen & the compiler should be able to optimise away any temporaries.
  • There are no heap allocations here at all (unless you have thousands of camera objects). Also, if Eigen cannot avoid a temporary, it will likely use the stack for it:

EIGEN_STACK_ALLOCATION_LIMIT - defines the maximum bytes for a buffer to be allocated on the stack. For internal temporary buffers, dynamic memory allocation is employed as a fall back. For fixed-size matrices or arrays, exceeding this threshold raises a compile time assertion. Use 0 to set no limit. Default is 128 KB.

you can further force this with:

EIGEN_NO_MALLOC - if defined, any request from inside the Eigen to allocate memory from the heap results in an assertion failure. This is useful to check that some routine does not allocate memory dynamically. Not defined by default.

Note that, if Eigen did allocate memory (very unlikely from what we have seen of your application), then Eigen will free that memory as soon as it's not needed. This is the standard behaviour in C++.

Better to store or construct?

Impossible to tell without knowing the usage pattern. Need to know the actual number of times camera is repositioned vs the number of times these quantities would have to be computed if they were not stored. Neither option seems expensive. Since there is no malloc, this is just two SIMD optimised cross products of tiny matrices. I have no idea how long that takes but if it's longer than a couple of hundred cycles I would be surprised.

Performance mockup

I hacked this basic loop to get an idea. The escape() is to avoid the compiler just removing our program altogether.

static void escape(void *p) {
  asm volatile("" : : "g"(p) : "memory");
}

int main() {
  Vector9d init = Vector9d::Random();
  auto cam = Camera{init, 2, 1, 3, 4};
  std::cout << cam.Position() << "\n";
  for (std::size_t i = 0; i < 1'000'000; ++i) {
    cam += init;
    Matrix32d r12 = cam.R12();
    escape(&r12);
  }
  std::cout << cam.Position() << "\n";
  return 0;
}

On my (quite old i7 2600) machine with -O3 -DNDEBUG that runs in 65ms. So a single camera move takes around (very roughly!) 65ns. Here is the, slightly modified to eliminate the std::cout printing, version in machine code. A few hundred lines of assembly, packed full of SIMD instructions. Scary fast?

The bottlenecks for your application are likely to lie elsewhere in your calling code. In general you are slightly in danger of "premature optimisation" concerns. It's not at all clear that there is anything expensive here. Need to measure first when application is up and running.

I hope that helps. I had to make some assumptions. Come back with comments if they were incorrect.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Does NDEBUG need to be defined to 1 specifically? If Eigen is using standard assert, then it's sufficient to define it (-DNDEBUG) and it doesn't need a value. \$\endgroup\$ Jan 29 '20 at 18:10
  • \$\begingroup\$ @TobySpeight Yes that's probably correct. It's an old habbit of mine. Make it "something truthy". \$\endgroup\$ Jan 29 '20 at 18:19
  • \$\begingroup\$ Thank you very much for your answer. I usually use C# or scripting languages, so I am very paranoid and start to overthink stuff when it comes to C++ and how to manage memory and pointers, references etc. Your notes on Eigen were really insightful for me and take away the paranoia a little. \$\endgroup\$
    – LivelyLiz
    Jan 29 '20 at 18:31
  • \$\begingroup\$ @Lisa Thanks! I understand the paranoia. It comes naturally when you work with "managed languages" Always trying to optimise, but "not quite in control" of what is really happening. But yeah: C++ with Eigen just eats this for breakfast it seems. Since you're new: If you found the answer useful, please upvote it and accept it. \$\endgroup\$ Jan 29 '20 at 18:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.