Reworked proximity renderer #518
Conversation
| auto& testSuite = boost::unit_test::framework::master_test_suite(); | ||
|
|
||
| const char* app = testSuite.argv[0]; | ||
| const char* argv[] = {app, "--renderer", |
There was a problem hiding this comment.
The args are formatted weird. Why not:
const char* argv[] = {app, "demo",
"--renderer", "proximity",
"--samples-per-pixel", "16",
"--synchronous-mode", "true"};
There was a problem hiding this comment.
If you move "demo" as the first argument then the other arguments and values might line up better
There was a problem hiding this comment.
you can also // clang-format off it's too messy ;-)
There was a problem hiding this comment.
The format applied here is what we agreed on when we decided to use clang-format. I would not see the point of making an exception here. And I don't see any issue with the new formatting, it's clear and not messy at all. Am I missing something?
favreau
force-pushed
the
master
branch
2 times, most recently
from
f4d5594
to
d07fddf
Compare
|
What's different/new/fixed in 'Reworked'? Hard to see from the code. |
|
@tribal-tec. Just added a description to the PR. Let me know if it's still unclear. |
|
Thx, much better. Please put that to the commit message also when you rebase/merge. |
favreau
force-pushed
the
master
branch
2 times, most recently
from
f20cdb4
to
c0950af
Compare
tests/braynsTestData.cpp
Outdated
| @@ -222,6 +222,31 @@ BOOST_AUTO_TEST_CASE(render_circuit_with_particle_renderer) | |||
| brayns.getEngine().getFrameBuffer())); | |||
| } | |||
|
|
|||
| BOOST_AUTO_TEST_CASE(render_circuit_with_proximity_renderer) | |||
There was a problem hiding this comment.
This test doesn't use a circuit here. We should move all the tests that do not need BBPTestDaa to a separate file.
There was a problem hiding this comment.
I agree, but will do a separate PR for that.
tests/braynsTestData.cpp
Outdated
| camera.setPosition(camPos - (rotCenter - camPos)); | ||
|
|
||
| brayns.render(); | ||
| BOOST_CHECK(compareTestImage("testdataallmini50proximity.png", |
There was a problem hiding this comment.
testproximity.png only?
There was a problem hiding this comment.
Ok. testDemoProximity.png
| if (self->detectionDistance > 0.f && | ||
| ray.t < self->detectionDistance * 1000.f) | ||
| bool continueWithSurfaceShading = true; | ||
| if (self->detectionDistance) |
There was a problem hiding this comment.
Why not write detectionDistance > 0.f, it looks a bit weird to treat a float as a bool.
There was a problem hiding this comment.
Hum... it's not a boolean, it really is a test on the value. We do the same for shadow intensity, and ambient occlusion strength for example. I don't think we need to introduce a new boolean variable there. Ok with you?
There was a problem hiding this comment.
As discussed, forget my comment.
| postIntersect(self->super.super.model, dg, ao_ray, | ||
| DG_MATERIALID); | ||
|
|
||
| varying bool test = true; | ||
| bool test = true; |
There was a problem hiding this comment.
Rename to doDetectionTest
| postIntersect(self->super.super.model, dg, ao_ray, | ||
| DG_MATERIALID); | ||
|
|
||
| varying bool test = true; |
There was a problem hiding this comment.
I think it is clearer to write like this:
if (self->detectionOnDifferentMaterial && material == dg.material)
test = false;
or make one expression like:
bool doDetectionTest = !(self->detectionOnDifferenetMaterial && material == dg.material);
| color = self->nearColor * opacity / ao_ray.t; | ||
| const float a = ao_ray.t / self->detectionDistance; | ||
| const vec4f touchColor = | ||
| make_vec4f(a > 0.2f ? self->nearColor : self->farColor, |
There was a problem hiding this comment.
0.2f is a magic number so it should be a constant.
There was a problem hiding this comment.
Ok. created a nearFarThreshold constant.
favreau
force-pushed
the
master
branch
2 times, most recently
from
5e3ed75
to
f72e912
Compare
tests/braynsTestData.cpp
Outdated
| camera.setTarget(rotCenter); | ||
| camera.setPosition(camPos - (rotCenter - camPos)); | ||
|
|
||
| brayns.render(); |
There was a problem hiding this comment.
After my previous PR, you have to call commitAndRender() here now to fix the test error in CI.
There was a problem hiding this comment.
hum... fails in debug.
Test is aborted Leaving test module "braynsAddModelFromBlob"; testing time: 2179609us LeakSanitizer: bad pointer 0x000000000056
…he geometry without impacting the rendering of the touches. New rendering parameters are: - Surface shading: Activates or deactivates the rendering of the surfaces - Alpha correction: Used to blend colors as the ray goes through intersections
|
Retest this please |
The idea is to be able to show or hide the geometry without impacting the rendering of the touches. New rendering parameters are: