-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed simulation renderer to use OSPVolumes #484
Conversation
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just pass 'fb->size.x' instead of the entire fb?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. If it is possible maybe a unit test would be nice too ;)
// } | ||
// } | ||
return shadowIntensity; | ||
int x = max(1, (int)(1.f / volume->samplingRate)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give proper variable name, is it samplingRate?
// } | ||
return shadowIntensity; | ||
int x = max(1, (int)(1.f / volume->samplingRate)); | ||
int c = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to sampleCounter or something
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename variable to randomSample?
t0 -= random; | ||
t1 -= random; | ||
const float epsilon = 1.f; | ||
int x = max(1, (int)(1.f / volume->samplingRate)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename x to samplingRate
t1 -= random; | ||
const float epsilon = 1.f; | ||
int x = max(1, (int)(1.f / volume->samplingRate)); | ||
int c = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to sampleCounter or something
// Shading according to computed normal | ||
const vec2f s = | ||
make_vec2f(0.0f); // sample center of area lights | ||
const Light_SampleRes light = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only taking the first light what happens if you have several lights?
@@ -626,7 +600,7 @@ inline void setGeometryShadingAttributes( | |||
attributes.opacity *= diffuseColorFromMap.w; | |||
} | |||
|
|||
// Specular color | |||
// Specular coloboundingBoxr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These paragraphs have strange line breaks.
{ | ||
RandomTEA rng_state; | ||
varying RandomTEA* const uniform rng = &rng_state; | ||
RandomTEA__Constructor(rng, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
For some reason I code reviewed outdated code so ignore those :( |
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 20.f should maybe be a constant somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it a attribute of the renderer (volumeSpecularExponent)
const vec2f s = make_vec2f(0.5f); | ||
const Light_SampleRes light = | ||
self->abstract.lights[0]->sample(self->abstract.lights[0], | ||
dg, s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just use make_vec2f(0.5f) and remove variable s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot, the sample function needs a reference, so there needs to be a variable there :(
// Look up the opacity associated with the volume sample. | ||
sampleOpacity = volume->transferFunction->getOpacityForValue( | ||
volume->transferFunction, volumeSample); | ||
if (samplingCounter == 0 && sampleOpacity < 0.01f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make magic number 0.01f a constant somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am now using the threshold attribute that I renamed samplingThreshold.
} | ||
composite(make_vec4f(volumeSampleColor, sampleOpacity), pathColor, | ||
2.0f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this magic number should also be a constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEFAULT_COMPOSITE_ALPHA_CORRECTION
if (shadowRay.geomID == -1) | ||
break; | ||
|
||
DifferentialGeometry dg2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better name than dg2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, shadowingGeometry is the correct term :)
Hold on, just realized that the volume alpha is wrong in the frame buffer. Fixing it now... |
{0.01f, 1.f}}); | ||
properties.setProperty({"volumeSpecularExponent", | ||
"Volume specular exponent", | ||
1.f, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value is 20 in osp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, 'ns' attribute in ospray/volume/volume.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? here it's 1.f, but here it's 20: https://github.com/BlueBrain/Brayns/pull/484/files#diff-cdd02215d26908726f81514dace27955R63
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo 'ot'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx :)
No description provided.