Replaced lookat, target, position by quaternion, position in the camera #591

chevtche · 2018-10-16T14:42:31Z

No description provided.

tribal-tec · 2018-10-16T14:45:51Z

tests/shadows.cpp

-    camera.setUp(brayns::Vector3f(0.f, 1.f, 0.f));
-    camera.setPosition(brayns::Vector3f(0.f, 2 * height, -1.5f));
+    //camera.setTarget(brayns::Vector3f(0.f, 0.f, 0.f));
+    //camera.setUp(brayns::Vector3f(0.f, 1.f, 0.f));


tribal-tec · 2018-10-16T14:46:06Z

apps/BraynsBenchmark/main.cpp

-            const brayns::Vector3f up = {0.f, 1.f, 0.f};
-            engine.getCamera().set(origin, target, up);
-
+            const brayns::Vector3d center = bounds.getCenter();


const auto&

I prefer to use "const brayns::Vector3d&". It's easier to understand and more consistent with the following lines.

tribal-tec · 2018-10-16T14:46:18Z

apps/BraynsBenchmark/main.cpp

-            engine.getCamera().set(origin, target, up);
-
+            const brayns::Vector3d center = bounds.getCenter();
+            const brayns::Quaterniond quat(frame * M_PI / 180.0f, brayns::Vector3d(0.0,1.0,0.0));


mix of float and double

tribal-tec · 2018-10-16T14:46:56Z

brayns/common/camera/AbstractManipulator.h

@@ -31,7 +31,15 @@ namespace brayns
 class AbstractManipulator
 {
 public:
-    AbstractManipulator(Camera& camera, KeyboardHandler& keyboardHandler);
+
+    enum AxisMode


enum class, lower-case names for values

What do mean ? AxisMode -> axisMode ? or GLOBAL_Y -> globalY ?

Is AxisMode a value?

enum class AxisMode { globalY

Suggested change

enum AxisMode

enum class AxisMode

{

local_y,

global_y

}

We were using camelCase in other projects

tribal-tec · 2018-10-16T14:47:33Z

apps/BraynsBenchmark/main.cpp

+            const brayns::Quaterniond quat(frame * M_PI / 180.0f, brayns::Vector3d(0.0,1.0,0.0));
+            const brayns::Vector3d dir = quat.rotate(brayns::Vector3d(0.0, 0.0, -1.0));
+            engine.getCamera().setPosition(center + radius * -dir);
+            engine.getCamera().setQuaternion(quat);


why not keep using the set() function?

Call the method camera.setOrientation. The quaternion is the parameter type, not the attribute you're modifying. Otherwise it would be setVector instead of setPosition

tribal-tec · 2018-10-16T14:49:31Z

brayns/common/camera/Camera.h

-    const Vector3d& getUp() const { return _up; }
+    void setQuaternion(const Quaterniond& quat)
+    {
+        Quaterniond normalized = quat;


could just take a copy arg in the function then

tribal-tec · 2018-10-16T14:50:09Z

brayns/common/camera/Camera.h

@@ -52,13 +52,9 @@ class Camera : public PropertyObject
              camera is "looking at" or focused on


that comment is wrong now

also the one for the class

karjonas

LGTM. Just make sure to run clang format on all the changed files.

karjonas · 2018-10-16T14:59:31Z

brayns/common/camera/AbstractManipulator.cpp

-    if (updateTarget)
-        _camera.setTarget(_camera.getPosition() - newPivotToCam);
+    Vector3d axisY;
+    if(axisMode == LOCAL_Y)


It looks like no clang format has been run on this code

karjonas · 2018-10-16T15:01:35Z

brayns/common/camera/AbstractManipulator.cpp


-    if (updateTarget)
-        _camera.setTarget(_camera.getPosition() - newPivotToCam);
+    Vector3d axisY;


You can write this as const auto axisY = axisMode == LOCAL_Y ? _camera.getQuaternion().rotate(Vector3d(0.0, 1.0, 0.0)) : Vector3d(0.0, 1.0, 0.0);

karjonas · 2018-10-16T15:06:39Z

tests/webAPI.cpp

@@ -37,10 +37,10 @@ BOOST_AUTO_TEST_CASE(change_fov)

 BOOST_AUTO_TEST_CASE(reset_camera)
 {
-    const auto target = getCamera().getTarget();
-    getCamera().setTarget({1, 2, 3});
+    const auto quat = getCamera().getQuaternion();


const auto (&) quat

karjonas · 2018-10-17T07:20:23Z

brayns/common/camera/AbstractManipulator.cpp


 namespace brayns
 {
 namespace
 {
-const float DEFAULT_MOTION_SPEED = 0.001f;
-const float DEFAULT_ROTATION_SPEED = 0.005f;
+const float DEFAULT_MOTION_SPEED = 0.03f;


Suggested change

const float DEFAULT_MOTION_SPEED = 0.03f;

constexpr float DEFAULT_MOTION_SPEED = 0.03f;

rolandjitsu

Furthermore, the docker build is failing currently with:

[261/405] Building CXX object brayns/common/CMakeFiles/braynsCommon.dir/camera/AbstractManipulator.cpp.o
FAILED: brayns/common/CMakeFiles/braynsCommon.dir/camera/AbstractManipulator.cpp.o 
/usr/bin/c++  -DBOOST_ALL_NO_LIB -DBRAYNSCOMMON_DSO_NAME=\"libbraynsCommon.so.0.8.0\" -DBRAYNSCOMMON_SHARED -DBRAYNS_LITTLEENDIAN=1 -DBRAYNS_USE_ASSIMP=1 -DBRAYNS_USE_BOOST=1 -DBRAYNS_USE_BRION=1 -DBRAYNS_USE_CXX11=1 -DBRAYNS_USE_FREEIMAGE=1 -DBRAYNS_USE_HDF5=1 -DBRAYNS_USE_LIBARCHIVE=1 -DBRAYNS_USE_LIBJPEGTURBO=1 -DBRAYNS_USE_LIBUV=1 -DBRAYNS_USE_NETWORKING=1 -DBRAYNS_USE_OPENMP=1 -DBRAYNS_USE_OSPRAY=1 -DBRAYNS_USE_ROCKETS=1 -DBRAYNS_USE_SERVUS=1 -DBRAYNS_USE_VMMLIB=1 -DBoost_NO_BOOST_CMAKE -DLIBASYNC_STATIC -DLinux=1 -DOSPRAY_TASKING_TBB -DWARN_DEPRECATED -DbraynsCommon_EXPORTS -I../ -Iinclude -I. -I../vmmlib -Ivmmlib/include -Ivmmlib -I../async++/include -isystem /usr/lib/include -isystem /app/dist/include -isystem /app/dist/include/ospray/SDK -isystem /app/dist/include/ospray -isystem /usr/include/hdf5/serial -isystem ../uvw/src -fopenmp -O3 -DNDEBUG -fPIC   -Wuninitialized -Wnon-virtual-dtor -Wsign-promo -Wvla -fno-strict-aliasing -Wall -Wextra -Winvalid-pch -Winit-self -Wno-unknown-pragmas -Wshadow -Werror -fmax-errors=5 -pthread -std=gnu++14 -MD -MT brayns/common/CMakeFiles/braynsCommon.dir/camera/AbstractManipulator.cpp.o -MF brayns/common/CMakeFiles/braynsCommon.dir/camera/AbstractManipulator.cpp.o.d -o brayns/common/CMakeFiles/braynsCommon.dir/camera/AbstractManipulator.cpp.o -c ../brayns/common/camera/AbstractManipulator.cpp
../brayns/common/camera/AbstractManipulator.cpp: In member function 'void brayns::AbstractManipulator::translate(const Vector3f&)':
../brayns/common/camera/AbstractManipulator.cpp:81:35: error: 'class vmml::Quaternion<double>' has no member named 'rotate'
     const auto translation = quat.rotate(vector);
                                   ^~~~~~
../brayns/common/camera/AbstractManipulator.cpp: In member function 'void brayns::AbstractManipulator::rotate(const Vector3f&, float, float, brayns::AbstractManipulator::AxisMode)':
../brayns/common/camera/AbstractManipulator.cpp:89:52: error: 'const Quaterniond' {aka 'const class vmml::Quaternion<double>'} has no member named 'rotate'
     const Vector3d axisX = _camera.getQuaternion().rotate(Vector3d(1.0, 0.0, 0.0));
                                                    ^~~~~~
../brayns/common/camera/AbstractManipulator.cpp:93:41: error: 'const Quaterniond' {aka 'const class vmml::Quaternion<double>'} has no member named 'rotate'
         axisY = _camera.getQuaternion().rotate(Vector3d(0.0, 1.0, 0.0));
                                         ^~~~~~
../brayns/common/camera/AbstractManipulator.cpp:101:32: error: 'const Quaterniond' {aka 'const class vmml::Quaternion<double>'} has no member named 'rotate'
     const Vector3d dir = final.rotate(Vector3d(0.0, 0.0, -1.0));
                                ^~~~~~

rolandjitsu · 2018-10-17T07:47:44Z

apps/ui/BaseWindow.cpp

-    renderInput.target = camera.getTarget();
-    renderInput.up = camera.getUp();
-
+    renderInput.quat = camera.getQuaternion();


Suggested change

renderInput.quat = camera.getQuaternion();

renderInput.quaternion = camera.getQuaternion();

As mentioned in other comment, this should be called orientation, not quaternion

rolandjitsu · 2018-10-17T07:50:13Z

plugins/RocketsPlugin/jsonSerialization.h

@@ -217,12 +217,11 @@ inline void init(brayns::Renderer::PickResult* p, ObjectHandler* h)

 inline void init(brayns::Camera* c, ObjectHandler* h)
 {
-    h->add_property("look_at", Vector3dArray(c->_target), Flags::Optional);
+    h->add_property("quat", Vector4dArray(c->_quat), Flags::Optional);


Suggested change

h->add_property("quat", Vector4dArray(c->_quat), Flags::Optional);

h->add_property("quaternion", Vector4dArray(c->_quaternion), Flags::Optional);

should be also 'orientation' as all the other instances

hernando · 2018-10-17T09:20:36Z

CI is not passing either, it looks like the revision for vmmlib in .gitsubprojects and the nix recipe has not been updated to account for the function added there.

hernando · 2018-10-17T09:28:49Z

apps/BraynsBenchmark/main.cpp

+            const brayns::Quaterniond quat(frame * M_PI / 180.0f, brayns::Vector3d(0.0,1.0,0.0));
+            const brayns::Vector3d dir = quat.rotate(brayns::Vector3d(0.0, 0.0, -1.0));
+            engine.getCamera().setPosition(center + radius * -dir);
+            engine.getCamera().setQuaternion(quat);


Call the method camera.setOrientation. The quaternion is the parameter type, not the attribute you're modifying. Otherwise it would be setVector instead of setPosition

tribal-tec · 2018-10-17T09:32:11Z

plugins/RocketsPlugin/jsonSerialization.h

@@ -217,12 +217,11 @@ inline void init(brayns::Renderer::PickResult* p, ObjectHandler* h)

 inline void init(brayns::Camera* c, ObjectHandler* h)
 {
-    h->add_property("look_at", Vector3dArray(c->_target), Flags::Optional);
+    h->add_property("quat", Vector4dArray(c->_quat), Flags::Optional);
    h->add_property("origin", Vector3dArray(c->_position), Flags::Optional);


rename to 'position'

chevtche · 2018-10-19T08:36:07Z

retest this please

…the camera (BlueBrain#591)" This reverts commit 28e43de.

chevtche requested review from tribal-tec, karjonas and favreau October 16, 2018 14:42

tribal-tec reviewed Oct 16, 2018

View reviewed changes

karjonas reviewed Oct 16, 2018

View reviewed changes

karjonas reviewed Oct 17, 2018

View reviewed changes

rolandjitsu suggested changes Oct 17, 2018

View reviewed changes

hernando suggested changes Oct 17, 2018

View reviewed changes

tribal-tec reviewed Oct 17, 2018

View reviewed changes

chevtche force-pushed the master branch from 9ffe7df to c94016e Compare October 18, 2018 10:05

chevtche force-pushed the master branch from d4ed1a5 to 70e550d Compare October 19, 2018 11:38

Replaced lookat, target, position by quaternion, position in the camera

b752442

chevtche force-pushed the master branch from 70e550d to b752442 Compare October 19, 2018 12:08

Merge branch 'master' into master

5003af1

rolandjitsu approved these changes Oct 19, 2018

View reviewed changes

hernando approved these changes Oct 19, 2018

View reviewed changes

chevtche merged commit 28e43de into BlueBrain:master Oct 19, 2018

tribal-tec added a commit to tribal-tec/Brayns that referenced this pull request Oct 22, 2018

Revert "Replaced lookat, target, position by quaternion, position in …

7b3c5a6

…the camera (BlueBrain#591)" This reverts commit 28e43de.

tribal-tec added the enhancement label Feb 15, 2019

-    enum AxisMode
+    enum class AxisMode
+    {
+        local_y,
+        global_y
+    }

		@@ -52,13 +52,9 @@ class Camera : public PropertyObject
		camera is "looking at" or focused on

	const float DEFAULT_MOTION_SPEED = 0.03f;
	constexpr float DEFAULT_MOTION_SPEED = 0.03f;

	renderInput.quat = camera.getQuaternion();
	renderInput.quaternion = camera.getQuaternion();

	h->add_property("quat", Vector4dArray(c->_quat), Flags::Optional);
	h->add_property("quaternion", Vector4dArray(c->_quaternion), Flags::Optional);

Replaced lookat, target, position by quaternion, position in the camera #591

Replaced lookat, target, position by quaternion, position in the camera #591

Conversation

chevtche commented Oct 16, 2018

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

hernando Oct 17, 2018 • 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

karjonas left a 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

rolandjitsu left a 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

hernando commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chevtche commented Oct 19, 2018

hernando Oct 17, 2018 •

edited

Loading