Added support for multiple volumes #37

favreau · 2016-10-19T14:31:07Z

No description provided.

jafyvilla · 2016-10-21T10:01:39Z

brayns/Brayns.cpp

@@ -424,7 +423,7 @@ struct Brayns::Impl
                TransferFunctionLoader transferFunctionLoader( brayns::Vector2f( 0, nbMaterials ));
                transferFunctionLoader.loadFromFile(
                    transferFunctionFilename, *_engine->getScene( ));
-                _engine->getScene()->commitSimulationData();


Not needed anymore?

No, done in the renderer implementation

jafyvilla · 2016-10-21T10:02:01Z

brayns/Brayns.cpp

        {
-            const Vector3ui& volumeDimensions = scene->getVolumeHandler()->getDimensions();
-            const Vector3f& volumeScale = volumeParameters.getScale();
+            const Vector3ui& volumeDimensions = scene->getVolumeHandler()->getDimensions( 0.f );


getDimensions with a parameter? looks weird...

Still there. It feels wrong. All the methods in VolumeHandler take the timestamp as an argument. Since it already is a member, couldn't you have a setTimeStamp method, and then call the getter (without argument) for the volume that is currently set?

I agree. I have to say that I was no so happy myself with that implementation. Simplified :-)

TBH, I had the same feeling. But having to set the timestamp first before accessing the class members sort of makes the volumeHandler transactional, which is not great either. But I also agree that the API looks ugly with the timestamp for every method. So well, I changed the code as you suggested, just because it's you ;-) And it does look better now.

I feel honored :)

jafyvilla · 2016-10-21T10:05:37Z

brayns/common/scene/Scene.cpp

@@ -199,7 +200,8 @@ void Scene::buildDefault( )
        _materials[material]->setColor( colors[material] );
        _materials[material]->setSpecularColor( WHITE );
        _materials[material]->setSpecularExponent( 10.f );
-        _materials[material]->setReflectionIndex( material == 4 ? 0.8f : 0.f );
+        _materials[material]->setReflectionIndex( 0.5f );
+        _materials[material]->setReflectionIndex( material == 4 ? 0.1f : 0.f );


choose a line

Any will do. Does not really matter.

jafyvilla · 2016-10-21T11:33:47Z

brayns/common/scene/Scene.cpp

+        if( !_volumeHandler && ( !volumeFile.empty() || !volumeFolder.empty() ))
+        {
+            _volumeHandler.reset(
+                new VolumeHandler( _parametersManager.getVolumeParameters(), TM_MODULO ));


It took me a while to realize what this TM_MODULO was. I suggest you use enums with a class name, describing the type. See https://github.com/BlueBrain/Fivox/blob/master/fivox/types.h#L80 for examples.

Much better indeed. But now I would need to change it everywhere else to be coherent. Done for this one, I will do the rest in a separate PR.

jafyvilla · 2016-10-21T11:38:39Z

brayns/common/volume/VolumeHandler.cpp

+float VolumeHandler::getEpsilon(
+    const float timestamp ,
+    const Vector3f& elementSpacing,
+    const uint16_t samplesPerRay )


jafyvilla · 2016-10-21T11:54:05Z

plugins/engines/optix/OptiXScene.cpp

@@ -563,37 +568,41 @@ void OptiXScene::commitVolumeData()
        return;
    }

+    float timestamp = _parametersManager.getSceneParameters().getTimestamp();


jafyvilla · 2016-10-21T11:54:31Z

plugins/engines/optix/OptiXScene.cpp


-    Vector3f diag = Vector3f( dimensions ) * scale;
+    Vector3f diag = Vector3f( dimensions ) * elementSpacing;
    const float volumeDiag = std::max( diag.x(), std::max( diag.y(), diag.z()));


ditto (find_max and const)

jafyvilla · 2016-10-21T11:55:36Z

plugins/engines/optix/cuda/renderer/Phong.h

+    const float n1,
+    const float n2)
+{
+    if( n2 == 0.f )


You shouldn't compare a float to 0

jafyvilla · 2016-10-21T11:56:51Z

plugins/engines/ospray/render/OSPRayRenderer.h

@@ -42,9 +42,13 @@ class OSPRayRenderer : public brayns::Renderer

    void setCamera( CameraPtr camera ) final;

+    const std::string& getName() { return _name; }


jafyvilla · 2016-10-21T11:57:20Z

plugins/engines/ospray/render/OSPRayRenderer.h

    OSPRenderer impl() { return _renderer; }

 private:
+
+    std::string _name;


Didn't check all the code, but it looks it can be const

jafyvilla · 2016-10-21T13:57:06Z

And rebase

favreau · 2016-10-24T15:57:05Z

Rebased and fixed issues in the OptiX plugin.

jafyvilla · 2016-10-25T09:29:00Z

brayns/common/scene/Scene.cpp

@@ -224,7 +223,7 @@ void Scene::buildDefault( )
    _materials[material]->setSpecularColor( WHITE );
    _materials[material]->setSpecularExponent( 100.f );

-    // Cylinder
+    // CyliindexOffsetnder


hum.... removed.

jafyvilla · 2016-10-25T09:30:08Z

brayns/common/scene/Scene.cpp

+        if( !_volumeHandler && ( !volumeFile.empty() || !volumeFolder.empty() ))
+        {
+            _volumeHandler.reset(
+                new VolumeHandler( _parametersManager.getVolumeParameters(), TM_MODULO ));


jafyvilla · 2016-10-25T09:31:50Z

Please add new commits instead of ammending, it's easier to review. Then you can "Squash and merge" to get rid of the unneeded commits.

favreau · 2016-10-25T11:20:35Z

Ok, good point. Sorry about that.

jafyvilla · 2016-10-25T15:50:55Z

🙃 now you forgot the squash...

favreau force-pushed the master branch from 1e96bbc to ab00aa4 Compare October 19, 2016 14:42

favreau assigned jafyvilla Oct 19, 2016

favreau force-pushed the master branch from ab00aa4 to 5d67b8a Compare October 19, 2016 14:47

jafyvilla reviewed Oct 21, 2016

View reviewed changes

favreau force-pushed the master branch from 5d67b8a to 2620852 Compare October 24, 2016 13:02

Added support for multiple volumes

947119f

favreau force-pushed the master branch from 2620852 to 947119f Compare October 24, 2016 15:55

jafyvilla reviewed Oct 25, 2016

View reviewed changes

Fixed PR comments from Jafet

17c9045

jafyvilla approved these changes Oct 25, 2016

View reviewed changes

favreau merged commit 50e84db into BlueBrain:master Oct 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for multiple volumes #37

Added support for multiple volumes #37

favreau commented Oct 19, 2016

jafyvilla Oct 21, 2016

favreau Oct 25, 2016

jafyvilla Oct 21, 2016

jafyvilla Oct 25, 2016

favreau Oct 25, 2016

favreau Oct 25, 2016

jafyvilla Oct 25, 2016

jafyvilla Oct 21, 2016

favreau Oct 25, 2016

jafyvilla Oct 21, 2016

jafyvilla Oct 25, 2016

favreau Oct 25, 2016

jafyvilla Oct 21, 2016

favreau Oct 25, 2016

jafyvilla Oct 21, 2016

favreau Oct 24, 2016

jafyvilla Oct 21, 2016

favreau Oct 24, 2016

jafyvilla Oct 21, 2016

favreau Oct 24, 2016

jafyvilla Oct 21, 2016

jafyvilla Oct 21, 2016

favreau Oct 25, 2016

jafyvilla commented Oct 21, 2016

favreau commented Oct 24, 2016 •

edited

jafyvilla Oct 25, 2016

favreau Oct 25, 2016

jafyvilla Oct 25, 2016

jafyvilla commented Oct 25, 2016

favreau commented Oct 25, 2016

jafyvilla commented Oct 25, 2016

		@@ -42,9 +42,13 @@ class OSPRayRenderer : public brayns::Renderer

		void setCamera( CameraPtr camera ) final;

		const std::string& getName() { return _name; }

Added support for multiple volumes #37

Added support for multiple volumes #37

Conversation

favreau commented Oct 19, 2016

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

jafyvilla commented Oct 21, 2016

favreau commented Oct 24, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafyvilla commented Oct 25, 2016

favreau commented Oct 25, 2016

jafyvilla commented Oct 25, 2016

favreau commented Oct 24, 2016 •

edited