Added scene management #379

favreau · 2018-05-02T13:42:50Z

Introduced the notion of models.

A scene has a set of models than can handle various assets such as meshes, point clouds, circuits, etc.
Each models contains the materials that are required by the geometry it handles.
Models can manage several instances of the geometry it handles, with a different transformation for every instance.
Models also contain metadata that are exposed to the HTTP interface via the v1/scene end-point.
Models provide a simple API to add primitives such as spheres, cones, cylinders, and triangle meshes.

tribal-tec

Pretty good overall!

I would like to have a test of adding a second model in the uploadpath and/or -binary test.

Also, the change of CMake/common should be reverted too I think.

tribal-tec · 2018-05-03T13:38:01Z

brayns/Brayns.cpp

@@ -482,6 +468,12 @@ struct Brayns::Impl : public PluginAPI
        _engine->getStatistics().setSceneSizeInBytes(
            _engine->getScene().getSizeInBytes());

+        // Set default camera position
+        const auto frameSize = Vector2f(_engine->getFrameBuffer().getSize());


_engine->setDefaultCamera(); ?

tribal-tec · 2018-05-03T13:38:45Z

brayns/Brayns.cpp

@@ -1226,17 +1183,20 @@ struct Brayns::Impl : public PluginAPI

    void _gradientMaterials()
    {
-        _engine->initializeMaterials(MaterialsColorMap::gradient);
+        _engine->getScene().setMaterialsColorMap(MaterialsColorMap::gradient);
+        _engine->getFrameBuffer().clear();
    }

    void _pastelMaterials()


no longer used, remove?

tribal-tec · 2018-05-03T13:38:58Z

brayns/Brayns.cpp

@@ -1287,10 +1247,9 @@ struct Brayns::Impl : public PluginAPI

    ParametersManager _parametersManager;
    EngineFactory _engineFactory;
-    EnginePtr _engine;
+    EnginePtr _engine{nullptr};


tribal-tec · 2018-05-03T13:39:08Z

brayns/Brayns.cpp

    KeyboardHandler _keyboardHandler;
-    AbstractManipulatorPtr _cameraManipulator;
-    MeshLoader _meshLoader;
+    AbstractManipulatorPtr _cameraManipulator{nullptr};


tribal-tec · 2018-05-03T13:40:08Z

brayns/common/BaseObject.h

@@ -55,6 +55,6 @@ class BaseObject
    }

 private:
-    std::atomic_bool _modified{true};
+    bool _modified{true};


can we revert this too? we can have a look offline if this is needed though be changed

tribal-tec · 2018-05-03T14:13:17Z

plugins/RocketsPlugin/SnapshotTask.h

@@ -77,7 +77,6 @@ class SnapshotFunctor : public TaskFunctor
            *_camera = *_params.camera;
        else
            *_camera = engine.getCamera();
-        _camera->setAspectRatio(float(_params.size.x()) / _params.size.y());


you sure? the snapshot has a different ratio. it's handled in the assignment now?

tribal-tec · 2018-05-03T14:14:21Z

plugins/RocketsPlugin/jsonSerialization.h

-    h->add_property("bounds", &s->getWorldBounds(),
-                    Flags::IgnoreRead | Flags::Optional);
-    h->add_property("materials", &s->getMaterials());
+    auto bounds = s->getBounds();


hmm, the bounds would ideally be cached by the scene, so they don't have to computed here. Let's discuss offline maybe.

tribal-tec · 2018-05-03T14:15:25Z

plugins/engines/ospray/OSPRayMaterial.h

+#define OSPRAYMATERIAL_H
+
+#include <brayns/common/material/Material.h>
+#include <ospray_cpp/Data.h>


not used? in any case, don't use they're cpp API, it's terrible :)

tribal-tec · 2018-05-03T14:26:08Z

plugins/engines/ospray/OSPRayModel.cpp

+#include <brayns/common/material/Material.h>
+namespace
+{
+const size_t BOUNDINGBOX_MATERIAL_ID = 9999;


no more than 10000-2 materials? so sad 📦

I regularly use more than that actually. Maybe set it to size_t (-1)?

Actually, shouldn't these be in types.h class? Next to
const size_t NO_MATERIAL = std::numeric_limits<size_t>::max();

I like that! Done.

tribal-tec · 2018-05-03T14:29:26Z

tests/ClientServer.h

@@ -45,8 +45,9 @@ const size_t SERVER_PROCESS_RETRIES = 10; /*ms*/
 class ForeverLoader : public brayns::Loader
 {
 public:
-    void importFromBlob(brayns::Blob&&, brayns::Scene&, const brayns::Matrix4f&,
-                        const size_t) final
+    void importFromBlob(brayns::Blob&&, brayns::Scene&, const size_t = 0,


those default values are not needed here. they're already in the base class

karjonas · 2018-05-04T09:31:24Z

brayns/common/scene/Model.cpp

+    if (_materials.find(materialId) == _materials.end())
+        throw std::runtime_error("Material " + std::to_string(materialId) +
+                                 " is not registered in the model");
+    return _materials[materialId];


Double lookup of materialId in map.

karjonas · 2018-05-04T09:33:14Z

brayns/common/scene/Model.cpp

+        catch (const std::runtime_error&)
+        {
+            createMaterial(materialId, std::to_string(materialId));
+        }


I would prefer to just do a _materials.find() instead of trying and catching exceptions.

The really fishy thing is that getMaterial doesn't return anything. It should have a different name.

Fixed. Thanks.

karjonas · 2018-05-04T09:36:58Z

brayns/common/scene/Model.h

+{
+    ModelTransformation() = default;
+    ModelTransformation(ModelTransformation&& rhs) = default;
+    ModelTransformation& operator=(ModelTransformation&& rhs) = default;


All constructors/assignment are default, maybe remove all?

We cannot because there is another constructor with a different signature below :(

Okay, I missed that 👍

hernando · 2018-05-04T09:42:37Z

brayns/common/scene/Model.h

+private:
+    Vector3f _translation{0.f, 0.f, 0.f};
+    Vector3f _scale{1.f, 1.f, 1.f};
+    Vector3f _rotation{0.f, 0.f, 0.f};


I guess my opinion doesn't count very much here, but this looks like Euler angles, which are quite cumbersome to specify formally (is this xzx, xyz, xyx, intrinsic, extrinsic, ...?). The interpretation is not even documented here.
Why don't you use a quaternion or an angle, axis representation, which are unambiguous and self-explanatory?

I agree, I guess we will match these with what the user uses in the UI.

It's a discussion that comes and go on a regular basis ;) I don't have a strong opinion there, I simply find the translation/scaling/rotation more human readable, and since this is what is exposed to the HTTP interface, I decided to use that representation. I guess the JS guy's opinion also is to consider. I'll ask input from @rolandjitsu

This interface doesn't have to match what users see necessarily. This class could use a matrix to encode the transformations and the client could use whatever is preferable depending on the use case, converting it to a matrix at the last step. That would be my choice, in fact.

I know this discussion is recurrent and I don't what to start it again here. My criticism is that the way the rotation is expressed in this API is unclear (If you give this to two different people, the chances of ending up with a different transformation are quite high). At the very least, it should be documented better. And since it's not so easy to document easily, I suggest to use something which is unambiguous instead.

Any method for manipulating objects from the GUI in a natural way and relative to the camera position will have to do some transformations, no matter what this API is, and for that you need an algebraic representation (matrix or quaternion), which means that in the end you need to transform the user input to a matrix or quaternion.
If this API describes the rotation as something else, it means that you need to transform the algebraic representation to whatever this API requires. Any representation can be easily converted to a quaternion or a matrix, but I find the opposite more annoying.
For me these are good reasons to choose a quaternion.

karjonas · 2018-05-04T10:06:25Z

plugins/RocketsPlugin/jsonSerialization.h

-                    Flags::IgnoreRead | Flags::Optional);
-    h->add_property("materials", &s->getMaterials());
+    auto bounds = s->getBounds();
+    h->add_property("bounds", &bounds, Flags::IgnoreRead | Flags::Optional);


Are you sending a pointer to a temporary variable 'bounds' here?

Yes! :) Fixed ;)

karjonas · 2018-05-04T10:15:13Z

brayns/io/ProteinLoader.cpp

+            material->setDiffuseColor({colorMap[materialId].R / 255.f,
+                                       colorMap[materialId].G / 255.f,
+                                       colorMap[materialId].B / 255.f});
+            for (const auto& sphere : spheresPerMaterial.second)


Why do you use colorMap[i] and colorMap[materialId] for the same material?

karjonas · 2018-05-04T10:32:53Z

I cannot comment too much on the design since I am not familiar enough with all the pieces but to me it looks fine :)

favreau · 2018-05-04T15:15:40Z

Retest this please

favreau force-pushed the models branch 10 times, most recently from 372d694 to fc8907f Compare May 3, 2018 13:26

Cyrille Pierre Henri Favreau added 4 commits May 3, 2018 15:29

Added model management

5891434

Rebased from master

230c286

Rebased from master

237fc09

Fixed unit tests

6606f74

favreau force-pushed the models branch from fc8907f to 6606f74 Compare May 3, 2018 13:33

favreau requested review from tribal-tec and karjonas May 3, 2018 13:35

tribal-tec suggested changes May 3, 2018

View reviewed changes

karjonas reviewed May 4, 2018

View reviewed changes

hernando reviewed May 4, 2018

View reviewed changes

karjonas reviewed May 4, 2018

View reviewed changes

favreau force-pushed the models branch 5 times, most recently from 799710f to 3794847 Compare May 4, 2018 14:59

PR comments

9b8592d

favreau force-pushed the models branch from 3794847 to 9b8592d Compare May 4, 2018 15:11

tribal-tec approved these changes May 4, 2018

View reviewed changes

favreau merged commit 55fe57a into BlueBrain:master May 4, 2018

tribal-tec added the enhancement label May 30, 2018

rolandjitsu mentioned this pull request Jun 7, 2018

Add scene management #363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added scene management #379

Added scene management #379

favreau commented May 2, 2018

tribal-tec left a comment

tribal-tec May 3, 2018

tribal-tec May 3, 2018

tribal-tec May 3, 2018

tribal-tec May 3, 2018

tribal-tec May 3, 2018

tribal-tec May 3, 2018

tribal-tec May 3, 2018

tribal-tec May 3, 2018

tribal-tec May 3, 2018

karjonas May 4, 2018

karjonas May 4, 2018

favreau May 4, 2018

tribal-tec May 3, 2018

karjonas May 4, 2018

favreau May 4, 2018

karjonas May 4, 2018

hernando May 4, 2018

favreau May 4, 2018

karjonas May 4, 2018

favreau May 4, 2018

karjonas May 4, 2018

hernando May 4, 2018

karjonas May 4, 2018

favreau May 4, 2018

hernando May 4, 2018

hernando May 4, 2018

hernando May 4, 2018

karjonas May 4, 2018

favreau May 4, 2018

karjonas May 4, 2018

favreau May 4, 2018

karjonas commented May 4, 2018

favreau commented May 4, 2018

Added scene management #379

Added scene management #379

Conversation

favreau commented May 2, 2018

tribal-tec 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

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

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 commented May 4, 2018

favreau commented May 4, 2018