Added the option to use metaballs for generating meshes for somas #116

favreau · 2017-02-07T14:41:17Z

No description provided.

rdumusc · 2017-02-08T14:11:31Z

brayns/io/MorphologyLoader.cpp

@@ -35,6 +36,8 @@
 #  include <brion/brion.h>
 #endif

+//#define BRAYNS_USE_BRION //// TO REMOVE ////


then remove?

rdumusc · 2017-02-08T15:08:13Z

brayns/common/scene/Scene.h

@@ -230,7 +230,7 @@ class Scene
    // Model
    PrimitivesMap _primitives;
    TrianglesMeshMap _trianglesMeshes;
-    Materials _materials;
+    MaterialsMap _materials;


I may not have the big picture in mind, but given the fact the the materials are contiguous from 0 -> nbMaterials, the vector looked more appropriate than the map

There was a mix between vectors and maps, but I decided to go for maps, mainly for access. I know that I will soon have to improve material management by being able to select then by name or ID. So maps are in the end more appropriate but I understand this is not clear in that code.

rdumusc · 2017-02-08T15:17:26Z

brayns/io/MorphologyLoader.cpp

+            // Soma
+            const brain::neuron::Soma& soma = morphology.getSoma();
+            const size_t material =
+                _material( morphologyIndex,


please rename _material() function so that it starts with an action verb: reading this I don't know what it is doing. Creating one? Getting an index..?

Renamed _material to _getMaterialFromSectionType

rdumusc · 2017-02-08T16:00:31Z

brayns/io/MorphologyLoader.cpp

+            const auto material =
+                _material( morphologyIndex, size_t( section.getType( )));
+            const auto& samples = section.getSamples();
+            if( samples.size() == 0 )


samples.empty()

rdumusc · 2017-02-08T16:05:15Z

brayns/io/MorphologyLoader.cpp

+            for( size_t i = 0; i < samplesToProcess; ++i )
+            {
+                const auto& sample = samples[i];
+                Vector3f position( sample.x(), sample.y(), sample.z());


rdumusc · 2017-02-08T16:10:06Z

brayns/io/MorphologyLoader.cpp

@@ -67,6 +188,7 @@ bool MorphologyLoader::_importMorphology(
    const size_t simulationOffset,
    float& maxDistanceToSoma)
 {
+    const bool useMetaballs = _geometryParameters.getMetaballsGridSize() != 0;


+1 for readability: make this a bool _useMetaballs() const func. and use it everywhere instead of _geometryParameters.getMetaballsGridSize() != 0

rdumusc · 2017-02-08T16:12:36Z

brayns/io/MorphologyLoader.cpp

+        if( morphologySectionTypes & MST_DENDRITE )
+            sectionTypes.push_back( brain::neuron::SectionType::dendrite );
+        if( morphologySectionTypes & MST_APICAL_DENDRITE )
+            sectionTypes.push_back( brain::neuron::SectionType::apicalDendrite );


this is a duplicate from around lines 224... try to split the code into more individual functions and reuse them whenever possible.

Done, but I had to make it a free function because the return type is from brion, and since Brion is am optional dependency, that would require an #ifdef in the header.

favreau · 2017-02-08T17:21:15Z

Retest this please

rdumusc

Good otherwise

rdumusc · 2017-02-09T07:48:55Z

brayns/common/engine/Engine.cpp

+    _scene.reset();
+    _camera.reset();
+    _renderers.clear();
+    _frameBuffer.reset();


this does not look useful, smart_ptrs are guaranteed to do it by themselves..?

Old man habits ;-) Removed

rdumusc · 2017-02-09T07:52:27Z

brayns/common/geometry/TrianglesMesh.cpp

+    _normals.clear();
+    _colors.clear();
+    _indices.clear();
+    _textureCoordinates.clear();


same here...?

rdumusc · 2017-02-09T07:53:31Z

brayns/common/scene/Scene.h

@@ -230,7 +230,7 @@ class Scene
    // Model
    PrimitivesMap _primitives;
    TrianglesMeshMap _trianglesMeshes;
-    Materials _materials;
+    MaterialsMap _materials;


rdumusc · 2017-02-09T07:56:05Z

brayns/io/MorphologyLoader.cpp

@@ -279,6 +400,17 @@ bool MorphologyLoader::importCircuit(
        {
            const auto& uri = uris[i];
            float maxDistanceToSoma = 0.f;
+
+            if( _geometryParameters.getMetaballsGridSize() != 0 )


_geometryParameters.useMetaballs()

Thanks. Done

rdumusc · 2017-02-09T07:56:21Z

brayns/io/MorphologyLoader.cpp

@@ -363,6 +495,16 @@ bool MorphologyLoader::importCircuit(
                &compartmentOffsets[i]
            };

+            if( _geometryParameters.getMetaballsGridSize() != 0 )


_geometryParameters.useMetaballs()

rdumusc · 2017-02-09T09:09:12Z

Oh, in case you maintain a changelog, there is no entry for this feature ;-)

favreau · 2017-02-09T09:35:46Z

Oops, yes, the changelog... Doing it now!

Added the option to use metaballs for generating meshes for somas

75e6675

favreau force-pushed the metaballs branch from 9c2b39b to 75e6675 Compare February 7, 2017 14:42

favreau assigned rdumusc Feb 7, 2017

rdumusc reviewed Feb 8, 2017

View reviewed changes

Cyrille Favreau added 2 commits February 8, 2017 17:46

Bug fixing in metaballs

17785f7

Fixed rdumusc comments

8d93e6c

rdumusc reviewed Feb 9, 2017

View reviewed changes

Fixed rdumusc comments BlueBrain#2

b6b491c

rdumusc approved these changes Feb 9, 2017

View reviewed changes

Updated ChangeLog and improved help documentation

509d5d6

favreau merged commit 56eba5a into BlueBrain:master Feb 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the option to use metaballs for generating meshes for somas #116

Added the option to use metaballs for generating meshes for somas #116

favreau commented Feb 7, 2017

rdumusc Feb 8, 2017

favreau Feb 8, 2017

rdumusc Feb 8, 2017

favreau Feb 8, 2017

rdumusc Feb 9, 2017

rdumusc Feb 8, 2017

favreau Feb 8, 2017

rdumusc Feb 8, 2017

favreau Feb 8, 2017

rdumusc Feb 8, 2017

favreau Feb 8, 2017

rdumusc Feb 8, 2017

favreau Feb 8, 2017

rdumusc Feb 8, 2017

favreau Feb 8, 2017

favreau commented Feb 8, 2017

rdumusc left a comment

rdumusc Feb 9, 2017

favreau Feb 9, 2017

rdumusc Feb 9, 2017

favreau Feb 9, 2017

rdumusc Feb 9, 2017

rdumusc Feb 9, 2017

favreau Feb 9, 2017

rdumusc Feb 9, 2017

favreau Feb 9, 2017

rdumusc commented Feb 9, 2017

favreau commented Feb 9, 2017

Added the option to use metaballs for generating meshes for somas #116

Added the option to use metaballs for generating meshes for somas #116

Conversation

favreau commented Feb 7, 2017

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

favreau commented Feb 8, 2017

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

rdumusc commented Feb 9, 2017

favreau commented Feb 9, 2017