Fixed simulation renderer to use OSPVolumes #484

favreau · 2018-08-01T15:27:51Z

No description provided.

tribal-tec · 2018-08-02T08:14:34Z

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

@@ -1128,9 +1128,16 @@ float getRandomValue(varying ScreenSample& sample, const int randomNumber)
    return rotate(randomDistribution[x][y], sample.sampleID.z % 8);
 }

-vec3f getRandomVector(varying ScreenSample& sample, const vec3f& normal,
-                      const int randomNumber)
+inline vec3f getRandomVector(uniform FrameBuffer* uniform fb,


just pass 'fb->size.x' instead of the entire fb?

Sure, the fb probably provides more possibilities to generate randomness, but since it's currently not the case, I guess we can limit the API to an int. Modifying it then. Thx.

karjonas

Looks good. If it is possible maybe a unit test would be nice too ;)

karjonas · 2018-08-02T08:18:52Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

-    //        }
-    //    }
-    return shadowIntensity;
+    int x = max(1, (int)(1.f / volume->samplingRate));


Give proper variable name, is it samplingRate?

karjonas · 2018-08-02T08:19:56Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

-    //    }
-    return shadowIntensity;
+    int x = max(1, (int)(1.f / volume->samplingRate));
+    int c = 0;


Rename to sampleCounter or something

karjonas · 2018-08-02T08:21:51Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

+         self->abstract.lights && i < self->abstract.numLights; ++i)
+    {
+        const uniform Light* uniform light = self->abstract.lights[i];
+        const varying vec2f s = make_vec2f(1.f / self->randomNumber);


Rename variable to randomSample?

karjonas · 2018-08-02T08:22:47Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

-    t0 -= random;
-    t1 -= random;
+    const float epsilon = 1.f;
+    int x = max(1, (int)(1.f / volume->samplingRate));


Rename x to samplingRate

karjonas · 2018-08-02T08:23:47Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

-    t1 -= random;
+    const float epsilon = 1.f;
+    int x = max(1, (int)(1.f / volume->samplingRate));
+    int c = 0;


Rename to sampleCounter or something

karjonas · 2018-08-02T08:27:39Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

+                // Shading according to computed normal
+                const vec2f s =
+                    make_vec2f(0.0f); // sample center of area lights
+                const Light_SampleRes light =


Since this is only taking the first light what happens if you have several lights?

karjonas · 2018-08-02T08:28:04Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

@@ -626,7 +600,7 @@ inline void setGeometryShadingAttributes(
            attributes.opacity *= diffuseColorFromMap.w;
        }

-        // Specular color
+        // Specular coloboundingBoxr


karjonas · 2018-08-02T08:28:56Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

@@ -710,34 +685,42 @@ inline void processSimulationValue(ShadingAttributes& attributes,
 }

 /**
-  The purpose of the processSimulationContribution function is to retrieve the
-  value of the simulation from a secondary model attached to the OSPRay scene.
+  The purpose of the processSimulationContribution function is to retrieve


These paragraphs have strange line breaks.

karjonas · 2018-08-02T08:32:32Z

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

 {
+    RandomTEA rng_state;
+    varying RandomTEA* const uniform rng = &rng_state;
+    RandomTEA__Constructor(rng,


It seems weird to me that you have to construct the RandomTEA object every time you get a random vector. Is this how it is done elsewhere?

karjonas · 2018-08-02T08:37:07Z

For some reason I code reviewed outdated code so ignore those :(

karjonas · 2018-08-02T08:40:10Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

+                const vec3f reflectedNormal = normalize(
+                    ray.dir - 2.f * dot(ray.dir, gradient) * gradient);
+                const float angle =
+                    powf(max(0.f, dot(light.dir, reflectedNormal)), 20.f);


Magic number 20.f should maybe be a constant somewhere

Made it a attribute of the renderer (volumeSpecularExponent)

karjonas · 2018-08-02T08:43:16Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

+                const vec2f s = make_vec2f(0.5f);
+                const Light_SampleRes light =
+                    self->abstract.lights[0]->sample(self->abstract.lights[0],
+                                                     dg, s);


Maybe just use make_vec2f(0.5f) and remove variable s.

I cannot, the sample function needs a reference, so there needs to be a variable there :(

karjonas · 2018-08-02T08:47:00Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

+        // Look up the opacity associated with the volume sample.
+        sampleOpacity = volume->transferFunction->getOpacityForValue(
+            volume->transferFunction, volumeSample);
+        if (samplingCounter == 0 && sampleOpacity < 0.01f)


Make magic number 0.01f a constant somewhere?

I am now using the threshold attribute that I renamed samplingThreshold.

karjonas · 2018-08-02T08:52:47Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

        }
+        composite(make_vec4f(volumeSampleColor, sampleOpacity), pathColor,
+                  2.0f);


Maybe this magic number should also be a constant

DEFAULT_COMPOSITE_ALPHA_CORRECTION

karjonas · 2018-08-02T08:58:28Z

plugins/engines/ospray/ispc/render/SimulationRenderer.ispc

+        if (shadowRay.geomID == -1)
+            break;
+
+        DifferentialGeometry dg2;


Is there a better name than dg2?

Yep, shadowingGeometry is the correct term :)

favreau · 2018-08-02T09:56:13Z

Hold on, just realized that the volume alpha is wrong in the frame buffer. Fixing it now...

tribal-tec · 2018-08-02T14:20:01Z

plugins/engines/ospray/OSPRayEngine.cpp

+                                    {0.01f, 1.f}});
+            properties.setProperty({"volumeSpecularExponent",
+                                    "Volume specular exponent",
+                                    1.f,


default value is 20 in osp?

yes, 'ns' attribute in ospray/volume/volume.cpp

? here it's 1.f, but here it's 20: https://github.com/BlueBrain/Brayns/pull/484/files#diff-cdd02215d26908726f81514dace27955R63

tribal-tec · 2018-08-02T14:20:47Z

plugins/engines/ospray/ispc/render/utils/RandomGenerator.ih

@@ -29,14 +29,16 @@ float getRandomValue(varying ScreenSample& sample, const int randomNumber);
    Returns a random direction based on location in the frame buffer, the normal
   to the surface and rotation parameters for the precomputed hard-coded random
   distribution.
+    @param frameBufferWidth Width ot the frame buffer


Cyrille Pierre Henri Favreau added 2 commits August 1, 2018 17:28

Fixed simulation renderer to use OSPVolumes

e7e4146

Making progress

ac0c787

favreau force-pushed the master branch from e9dfc3f to 02325e6 Compare August 1, 2018 15:28

favreau requested a review from karjonas August 1, 2018 15:29

Volumes shadowing geomtry

8c74e1d

favreau force-pushed the master branch from 02325e6 to 8c74e1d Compare August 1, 2018 18:14

tribal-tec reviewed Aug 2, 2018

View reviewed changes

karjonas reviewed Aug 2, 2018

View reviewed changes

PR Comments

e797ca3

Fixed volume alpha in frame buffer

03b3a67

tribal-tec reviewed Aug 2, 2018

View reviewed changes

PR comments

00a563f

favreau force-pushed the master branch from ab0dea8 to 00a563f Compare August 2, 2018 14:35

Merge branch 'master' into master

018d62e

tribal-tec approved these changes Aug 2, 2018

View reviewed changes

favreau merged commit 161165a into BlueBrain:master Aug 2, 2018

tribal-tec added the bug label Sep 4, 2018

tribal-tec changed the title ~~Fixed simulation renderer to use OSPVolumes~~ Fixed simulation renderer to use OSPVolumes Sep 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed simulation renderer to use OSPVolumes #484

Fixed simulation renderer to use OSPVolumes #484

favreau commented Aug 1, 2018

tribal-tec Aug 2, 2018

favreau Aug 2, 2018

karjonas left a comment

karjonas Aug 2, 2018

karjonas Aug 2, 2018

karjonas Aug 2, 2018

karjonas Aug 2, 2018

karjonas Aug 2, 2018

karjonas Aug 2, 2018

karjonas Aug 2, 2018

karjonas Aug 2, 2018

karjonas Aug 2, 2018

karjonas commented Aug 2, 2018

karjonas Aug 2, 2018

favreau Aug 2, 2018

karjonas Aug 2, 2018

favreau Aug 2, 2018

karjonas Aug 2, 2018

favreau Aug 2, 2018 •

edited

Loading

karjonas Aug 2, 2018

favreau Aug 2, 2018

karjonas Aug 2, 2018

favreau Aug 2, 2018

favreau commented Aug 2, 2018

tribal-tec Aug 2, 2018

favreau Aug 2, 2018

tribal-tec Aug 2, 2018

tribal-tec Aug 2, 2018

favreau Aug 2, 2018

Fixed simulation renderer to use OSPVolumes #484

Fixed simulation renderer to use OSPVolumes #484

Conversation

favreau commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karjonas 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

karjonas commented Aug 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

favreau Aug 2, 2018 • edited Loading

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 Aug 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

favreau Aug 2, 2018 •

edited

Loading