SDF geometries support #391

karjonas · 2018-05-18T14:35:57Z

A new geometry type called ExtendedSDFGeometries is added. This PR also includes a thickness smoothing algorithm used when loading morphologies. It can be activated by adding '--dampen-branch-thickness-changerate true' when running Brayns. To create SDF geometries when loading a morphology you add '--use-sdf-geometries true'

rdumusc

Look pretty good in general! Some unit tests would be nice too :-)

rdumusc · 2018-05-22T09:42:49Z

brayns/common/CMakeLists.txt

@@ -42,6 +42,7 @@ set(BRAYNSCOMMON_PUBLIC_HEADERS
  geometry/Cylinder.h
  geometry/Sphere.h
  geometry/TrianglesMesh.h
+  geometry/SDFGeometry.h


Alphabetical order

rdumusc · 2018-05-22T09:54:44Z

brayns/io/MorphologyLoader.cpp

@@ -66,7 +81,7 @@ class MorphologyLoader::Impl
                                              model.getCylinders(),
                                              model.getCones(),
                                              model.getTrianglesMeshes(),
-                                              model.getBounds());
+                                              model.getBounds(), model);


hmmm, looks like it should only take model as a parameter then. But maybe there was a reason for not passing the model before..?

rdumusc · 2018-05-22T09:55:30Z

brayns/io/MorphologyLoader.cpp

-     * @param compartmentReport Compartment report to map to the morphology
-     * @param scene Scene to which the morphology should be loaded into
-     * @return True if the loading was successful, false otherwise
+     * @brief _connectSDFSomaChildren Creates an SDF soma by adding and


rm "@brief _connectSDFSomaChildren"

rdumusc · 2018-05-22T10:04:15Z

brayns/common/geometry/SDFGeometry.h

+inline SDFGeometry createSDFConePill(const Vector3f& p0, const Vector3f& p1,
+                                     const float radiusBottom,
+                                     const float radiusTip,
+                                     const float sigmoid = false)


Since ConePill and ConePillSigmoid are two separate SDFTypes, it would probably be more consistent to have a createSDFConePillSigmoid() function.

In general, bool parameters are not ideal because they are not self-describing when reading the calling code.
In:
createSDFConePill(somaPosition, sample, somaRadius * 0.5f, radiusEnd, true);
it is not obvious what true means.

rdumusc · 2018-05-22T10:05:52Z

brayns/io/MorphologyLoader.cpp

+    }
+
+    /**
+     * @brief _finalizeSDFGeometries Calculates all neighbours and adds the


rm "@brief _finalizeSDFGeometries"

rdumusc · 2018-05-22T11:27:17Z

plugins/engines/ospray/ispc/geometry/ExtendedSDFGeometries.ispc

+    vec2f t = make_vec2f(max(tmin.x, tmin.y), max(tmin.x, tmin.z));
+    t0 = max(t.x, t.y);
+    t = make_vec2f(min(tmax.x, tmax.y), min(tmax.x, tmax.z));
+    t1 = min(t.x, t.y);


rdumusc · 2018-05-22T11:36:30Z

brayns/common/scene/Model.h

+    std::vector<size_t> _SDFNeighboursFlat;
+    std::vector<std::vector<size_t>> _SDFNeighbours;
+    std::vector<SDFGeometry> _SDFGeometries;
+    bool _SDFGeometriesDirty{false};


Not sure, but that's a lot of additions to the Model class, could maybe go into a separate SDF class instanced here..?

I was wondering in general if this should be in the core at all or rather be a 'real' plugin? Maybe not now, but latest when the circuit loader and friends are moved to a plugin, this shall move as well.

rdumusc · 2018-05-22T11:39:24Z

brayns/common/scene/Model.h

+                                  const std::vector<size_t>& neighbourIndices);
+
+    /** Builds the flat list of neighbours for the SDF geometries */
+    void buildSDFGeometryNeighboursFlat();


Does this have to be public? Who should call it, why and when?

rdumusc · 2018-05-22T11:41:40Z

brayns/common/scene/Model.h

+      @param neighbourIndices Indices of the neighbours
+      */
+    void setSDFGeometryNeighbours(size_t geometryIdx,
+                                  const std::vector<size_t>& neighbourIndices);


How about renaming this function to updateSDFGeometryNeighbours? They are already set in addSDFGeometry() if I'm correct.

Also, this may be a stupid question, but where do I find the geometryIdx to pass to this function?

rdumusc · 2018-05-22T11:46:13Z

brayns/common/scene/Model.h

+      Adds a SDFGeometry to the scene
+      @param materialId Material of the geometry
+      @param geom Geometry to add
+      @param neighbours List of global indices of the geometry neighbours


For people like me who are not familiar with SDF terminology, it would great if you could document what neighbours are used for and how I should compute them.

karjonas · 2018-05-24T12:03:31Z

All issues should be fixed now

tribal-tec

Good for me otherwise. Maybe trying to use the texture coordinates to support simulation mapping.

tribal-tec · 2018-05-24T12:02:28Z

brayns/common/scene/Model.cpp

+        _bounds.merge(geom.center + Vector3f(geom.radius));
+        break;
+    }
+    case brayns::SDFType::Pill:


use default: instead?

I added a throw on the default case so that you do not forget to set the bounds if you add a new geometry type.

favreau · 2018-05-24T16:06:28Z

Ultra super cool PR! :)

A new geometry type called ExtendedSDFGeometries is added. This PR also includes a thickness smoothing algorithm used when loading morphologies. It can be activated by adding '--dampen-branch-thickness-changerate true' when running Brayns. To create SDF geometries when loading a morphology you add '--use-sdf-geometries true'

…ing it

karjonas · 2018-05-25T07:17:06Z

I also added the textures and timestamp variables to the SDF geometries now so we can get the proper simulation colors.

rdumusc

+1 for me, just a few last comments :-)

rdumusc · 2018-05-25T09:26:13Z

brayns/common/scene/Model.cpp

+void Model::updateSDFGeometryNeighbours(
+    size_t geometryIdx, const std::vector<size_t>& neighbourIndices)
+{
+    _sdfGeometryData.neighbours[geometryIdx] = neighbourIndices;


doesn't this make the geometry dirty?

rdumusc · 2018-05-25T09:31:59Z

brayns/common/scene/Model.cpp

+    }
+    default:
+        throw std::runtime_error("No bounds found for SDF type.");
+    }


I would refactor this a bit, moving the logic to get the min/max to the geometry class and keep here only something like:
_bounds.merge(geom.getMin());
_bounds.merge(geom.getMax());
As an added benefit, it lets you unit test independently the geom.getMin/Max() code :-)

Yes, I created a function that returns the bounding box.

rdumusc · 2018-05-25T09:34:49Z

brayns/common/scene/Model.cpp

+           _sdfGeometryData.neighbours.size());
+
+    _sdfGeometryData.neighbours.push_back(neighbourIndices);
+    _sdfGeometryData.geometries.push_back(geom);


To limit indirection and make the code more readable, the above could moved to a function of sdfGeometryData and have here:
_sdfGeometryData.add(materialId, geom, neighbourIndices);

I renamed the variable to _sdf. Do you think it is readable enough then?

hmm sure, although my point was not exactly about the variable name. What I was trying to say is more that since those lines of code operate exclusively on the struct members, it could make sense to put them a member function of the struct which is called it from here... but that's fine as well.

rdumusc · 2018-05-25T09:36:49Z

brayns/common/scene/Model.h

+
+        std::vector<std::vector<size_t>> neighbours;
+        std::vector<size_t> neighboursFlat;
+        bool isDirty{false};


the dirty flag for other types of data is kept separately, maybe do the same for consistency..?

Yes, fixed.

karjonas requested review from tribal-tec and rdumusc May 18, 2018 14:36

rdumusc reviewed May 22, 2018

View reviewed changes

karjonas force-pushed the sdf_geometries branch 2 times, most recently from 677dbe6 to 7682fbe Compare May 24, 2018 10:07

tribal-tec reviewed May 24, 2018

View reviewed changes

Jonas Karlsson added 22 commits May 25, 2018 08:22

Fix ospray 1.4 compilation

da5bfa5

Do not calculate tree structure unnecessarily

952cf08

Alphabetize

eb9bf06

Add createSDFConePillSigmoid function

258c268

Make Parallel model loading work for SDF geometries

13ebc36

Fix documentation

b1f962f

Improve comments

5182165

Fix documentation

abe1c5a

Clarify variable name

7e781a0

Clarify using variable

154f704

Simplify

cdb9aa8

Prepend morphology to flag for clarity

359919e

Constify

a3d4923

Move SDF flat neigbhours list to OSPRayModel since that is the one us…

4255d71

…ing it

Rename function and improve documentation

1fdf5be

Rename command line flag

5817219

Add test case using SDF geometries

908ac60

Add missing rename

0e46cc7

Move adding of geometries into separate methods

b4cf1fa

Move SDF-related variables into own struct

5f70c56

Update test data

0ef86fe

Jonas Karlsson added 5 commits May 25, 2018 08:22

Fix materials bug

e58cd0d

Add timestamp and texture coordinates to SDF geometries

41f242a

Throw exception if SDF type is unsupported

bcab1b6

Fix texture lookup bug

880b9f8

Update test data

fc3307f

karjonas force-pushed the sdf_geometries branch from 189352a to fc3307f Compare May 25, 2018 06:50

rdumusc reviewed May 25, 2018

View reviewed changes

Jonas Karlsson added 7 commits May 25, 2018 11:54

Rename variable

cb6a0c9

Move bounds calculation

3bdb88b

Set geometry dirty if neighbours updated

bdda760

Rename variable for readability

2ace44e

Simplify code

25188e2

Simplify code

5b58fd7

Add unit tests for SDF bounding box

47e7ecd

rdumusc approved these changes May 25, 2018

View reviewed changes

karjonas merged commit 5d97ca3 into BlueBrain:master May 25, 2018

tribal-tec added the enhancement label May 30, 2018

karjonas deleted the sdf_geometries branch June 5, 2018 15:20

tribal-tec mentioned this pull request Jun 8, 2018

Use signed distance fields for approximating organic shapes (synapses, soma) #367

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDF geometries support #391

SDF geometries support #391

karjonas commented May 18, 2018

rdumusc left a comment

rdumusc May 22, 2018

rdumusc May 22, 2018

rdumusc May 22, 2018

rdumusc May 22, 2018

rdumusc May 22, 2018

rdumusc May 22, 2018

rdumusc May 22, 2018

tribal-tec May 22, 2018

rdumusc May 22, 2018

rdumusc May 22, 2018

rdumusc May 22, 2018

karjonas commented May 24, 2018

tribal-tec left a comment

tribal-tec May 24, 2018

karjonas May 25, 2018

favreau commented May 24, 2018

karjonas commented May 25, 2018

rdumusc left a comment

rdumusc May 25, 2018

karjonas May 25, 2018

rdumusc May 25, 2018

karjonas May 25, 2018

rdumusc May 25, 2018

karjonas May 25, 2018

rdumusc May 25, 2018

rdumusc May 25, 2018

karjonas May 25, 2018

SDF geometries support #391

SDF geometries support #391

Conversation

karjonas commented May 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karjonas commented May 24, 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

favreau commented May 24, 2018

karjonas commented May 25, 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