Add SharedData- and BrickedVolume support #448

tribal-tec · 2018-07-02T13:13:33Z

use OSPRay volume and transfer function to support volume rendering
add VolumeLoader for *.raw and *.mhd volumes

rdumusc · 2018-07-02T15:35:02Z

brayns/common/scene/Scene.h

@@ -109,6 +109,22 @@ class Scene : public BaseObject

    BRAYNS_API virtual ModelPtr createModel() const = 0;

+    /**
+     * Create a volume with the given dimension, voxel spacing and data type


I think it's volume dimensions (in plural form)

rdumusc · 2018-07-02T15:38:03Z

brayns/common/volume/BrickedVolume.h

+    }
+
+    virtual void setBrick(void* data, const Vector3ui& position,
+                          const Vector3ui& size) = 0;


the size of the bricks does not have to be fixed in the constructor?

no, can be different for each brick if desired

that's interesting, I'd like to learn how Ospray handles that. I wonder what happens in case of overlapping bricks.

There's no overlap, it just overrides potential existing voxels.

OK, I see that the class documentation says the bricks are copied. Last thing, shouldn't the data* be const?

Have to const_cast it then to pass it to ospray... Though they use const data internally behind the API...

rdumusc · 2018-07-02T15:46:19Z

brayns/common/volume/SharedDataVolume.h

+     * Convenience function to use voxels from given file and pass them to
+     * setVoxels().
+     */
+    void setData(const std::string& filename);


I would suggest mapData(filename)

rdumusc · 2018-07-02T16:00:48Z

plugins/engines/ospray/OSPRayScene.h

@@ -50,32 +50,33 @@ class OSPRayScene : public Scene
    bool commitLights() final;

    void commitSimulationData();
-    void commitVolumeData();
+    bool commitVolumeData();


this return value is a bit obscure; I would imagine that it indicates a possible failure, but after checking the code that's not the case... I think a separate getter for rebuildScene is preferable

Doesn't make it better imo. That functions is private anyways, I'll do that first and we can discuss if you like.

rdumusc · 2018-07-02T16:06:44Z

plugins/engines/ospray/ispc/render/utils/AbstractRenderer.ispc

+            //                voxelColor.x = voxelColor.x * giAttenuation;
+            //                voxelColor.y = voxelColor.y * giAttenuation;
+            //                voxelColor.z = voxelColor.z * giAttenuation;
+            //            }


lots of commented code here, is there a good reason not to delete it?

@favreau has to adapt the shaders to use the ospray volumes.

rdumusc · 2018-07-02T16:21:05Z

brayns/io/VolumeLoader.cpp

+        dataRange = {std::numeric_limits<int32_t>::min() / 100,
+                     std::numeric_limits<int32_t>::max() / 100};
+        break;
+    }


this can easily be a helper function

tribal-tec · 2018-07-03T13:04:40Z

The default camera position has changed somehow, that's why the tests fail. Will look into that.

favreau · 2018-07-04T12:01:50Z

VolumeHandler h and cpp can be removed, right?

tribal-tec · 2018-07-04T12:07:19Z

Yes, I forgot the header file somehow. Thx for the hint!

rdumusc

+1, looks good enough already, although a few things are still not very clear to me.

rdumusc · 2018-07-04T11:17:10Z

brayns/common/scene/Model.h

+    const Volumes& getVolumes() const { return _volumes; }
+    bool isVolumesDirty() const { return _volumesDirty; }
+    void resetVolumesDirty() { _volumesDirty = false; }
+    void updateSizeInBytes();


rdumusc · 2018-07-04T11:17:42Z

brayns/common/scene/Model.h

@@ -324,6 +335,9 @@ class Model
    bool _sdfGeometriesDirty{false};
    bool _instancesDirty{true};

+    Volumes _volumes;
+    bool _volumesDirty{true};


all other entities are dirty by default as well.

I'm surprised that things are dirty by default but OK. Just FYI there is also bool _sdfGeometriesDirty{false};

rdumusc · 2018-07-04T11:26:09Z

brayns/io/VolumeLoader.cpp

+                      {"element-spacing", std::to_string(spacing.x()) + " " +
+                                              std::to_string(spacing.y()) +
+                                              " " +
+                                              std::to_string(spacing.z())}});


providing a std::to_string(vector) would make this a lot cleaner

rdumusc · 2018-07-04T11:49:18Z

brayns/io/VolumeLoader.cpp

+    else
+    {
+        dimensions = _volumeParameters.getDimensions();
+        spacing = _volumeParameters.getElementSpacing();


Here only these 2 properties from VolumeParameters are used (which are the same as passed to the Volume constructor(s)). But I'm not sure they should be in the VolumeParameters class, because if you load an mhd then the dimensions of the Volume won't match with the VolumeParameters anymore (so they only act as default values, but that's kind of confusing).
I have the impression that {dimensions+spacing+type} could go into a separate class/struct (named VolumeParameters) that can be used here (stored as _defaultVolumeParameters) and maybe also used in Volume(s), while the rest of the class VolumeParameters is renamed to VolumeRenderingParameters.
Otherwise, if we want to keep the VolumeParameters (including dimensions) as a whole, then it should be updated from here to keep matching the size of what was loaded...

the parameters are the fallback if you don't have an mhd. They should be updated at best today if an mhd was provided.

rdumusc · 2018-07-04T12:08:28Z

plugins/engines/ospray/OSPRayScene.cpp

@@ -78,12 +78,12 @@ OSPRayScene::~OSPRayScene()
 void OSPRayScene::commit()
 {
    const bool rebuildScene = isModified();
+    const bool addRemoveVolumes = _commitVolumeData();


Without knowing too much, I still find the logic here convoluted. I don't know why the isModified() is checked before _commitSimulationData and commitTransferFunctionData and what's so different with _commitVolumeData... Couldn't they just set the isModified() flag as needed, and then we would "ideally" have a commit() function that looks like this:

void OSPRayScene::commit() { _commitVolumeData(); _commitSimulationData(); _commitTransferFunctionData(); if (isModified()) _rebuildScene(); }

the modified state is also evaluated outside after this function to clear the framebuffer for instance. This happens if for instance the transfer function has changed, hence _commitTransferFunctionData() updates the modified state in the scene. But it does not require a rebuild of the scene, so the state is stored explicitly before.

- use OSPRay volume and transfer function to support volume rendering - add VolumeLoader for *.raw and *.mhd volumes

tribal-tec requested review from favreau and rdumusc July 2, 2018 13:13

rdumusc reviewed Jul 2, 2018

View reviewed changes

tribal-tec force-pushed the volume_final branch 2 times, most recently from c912666 to 8aa2a29 Compare July 3, 2018 08:35

tribal-tec force-pushed the volume_final branch 2 times, most recently from bd65bf7 to a512d10 Compare July 4, 2018 09:26

tribal-tec force-pushed the volume_final branch from a512d10 to ee0fc8c Compare July 4, 2018 12:08

rdumusc approved these changes Jul 4, 2018

View reviewed changes

Add SharedData- and BrickedVolume support

26b8d4d

- use OSPRay volume and transfer function to support volume rendering - add VolumeLoader for *.raw and *.mhd volumes

tribal-tec force-pushed the volume_final branch from ee0fc8c to 26b8d4d Compare July 10, 2018 09:33

tribal-tec merged commit 93a9ea3 into BlueBrain:master Jul 10, 2018

tribal-tec deleted the volume_final branch July 10, 2018 09:53

tribal-tec added the enhancement label Sep 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SharedData- and BrickedVolume support #448

Add SharedData- and BrickedVolume support #448

tribal-tec commented Jul 2, 2018

rdumusc Jul 2, 2018

rdumusc Jul 2, 2018

tribal-tec Jul 3, 2018

rdumusc Jul 3, 2018

tribal-tec Jul 3, 2018

rdumusc Jul 3, 2018

tribal-tec Jul 3, 2018

rdumusc Jul 2, 2018

rdumusc Jul 2, 2018

tribal-tec Jul 3, 2018

rdumusc Jul 2, 2018

tribal-tec Jul 3, 2018

rdumusc Jul 2, 2018

tribal-tec commented Jul 3, 2018

favreau commented Jul 4, 2018

tribal-tec commented Jul 4, 2018

rdumusc left a comment

rdumusc Jul 4, 2018

rdumusc Jul 4, 2018

tribal-tec Jul 10, 2018

rdumusc Jul 10, 2018

rdumusc Jul 4, 2018

rdumusc Jul 4, 2018

tribal-tec Jul 10, 2018

rdumusc Jul 4, 2018

tribal-tec Jul 10, 2018

Add SharedData- and BrickedVolume support #448

Add SharedData- and BrickedVolume support #448

Conversation

tribal-tec commented Jul 2, 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

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

tribal-tec commented Jul 3, 2018

favreau commented Jul 4, 2018

tribal-tec commented Jul 4, 2018

rdumusc 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