Axis drawing is working #346

chevtche · 2016-10-18T08:35:27Z

No description provided.

eile · 2016-10-18T09:08:46Z

Better PR and commit message would be: Implement axis and legend for full volume box

'is working' is assumed at this point :P

eile · 2016-10-18T09:53:53Z

livre/core/render/Frustum.h

@@ -80,6 +80,11 @@ class Frustum : public Frustumf
    LIVRECORE_API Matrix4f getMVPMatrix() const;

    /**
+     * @return The normal matrix.


Please don't state the obvious. What's the normal matrix?

The normal matrix is for normals what modelview matrix is for vertices. I don't know a better name.
Everyone call that normal matrix. In old OpenGL it was called gl_NormalMatrix for example.

The function name is fine, but repeating it in doxygen is not. What about @return the inverse transposed model-view matrix?

eile · 2016-10-18T09:54:11Z

livre/eq/EventHandler.cpp

@@ -294,6 +294,11 @@ bool EventHandler< C >::handleEvent( const eq::EventType type,
        frameSettings.toggleStatistics();
        return true;

+    case 'a':
+    case 'A':
+        _impl->config.getFrameData().getRenderSettings().toogleDrawAxis();


typo toggle

eile · 2016-10-18T09:55:23Z

livre/eq/render/RayCastRenderer.cpp

+            _axis.draw();
+
+            glUseProgram( 0 );
+        }


refactor if scope into method

eile · 2016-10-18T09:56:55Z

livre/eq/render/RayCastRenderer.cpp

@@ -156,6 +168,7 @@ struct RayCastRenderer::Impl
        _nSamplesPerRay = frameData.getVRParameters().getSamplesPerRay();
        _computedSamplesPerRay = _nSamplesPerRay;
        _nSamplesPerPixel = frameData.getVRParameters().getSamplesPerPixel();
+        _drawAxis = frameData.getRenderSettings().getDrawAxis( );


Just a comment, i.e., don't change: This is useless dynamic state. Better design would be to pass the frameData state to the method in need of it, instead of duplicating it here.

I think the motivation for this duplication was to isolate the renderer from the Equalizer related classes.

eile · 2016-10-18T09:57:56Z

livre/lib/data/BoundingAxis.cpp

+    {
+        for( int32_t i = 0; i < 3; ++i )
+        {
+            for( int32_t j = 0; j < 4; ++j )


k-i-j loop nesting?

does it really matter ?

eile · 2016-10-18T09:58:09Z

livre/lib/data/BoundingAxis.cpp

+        }
+    }
+
+    for( uint32_t i = 0; i < vertices.size(); ++i)


eile · 2016-10-19T12:20:27Z

Erm, I just wanted to unblock PR, not approve. Still learning the new UI here... Somebody else please approve.

hernando

I will continue tomorrow

hernando · 2016-10-20T12:13:29Z

livre/eq/render/RayCastRenderer.cpp

@@ -156,6 +168,7 @@ struct RayCastRenderer::Impl
        _nSamplesPerRay = frameData.getVRParameters().getSamplesPerRay();
        _computedSamplesPerRay = _nSamplesPerRay;
        _nSamplesPerPixel = frameData.getVRParameters().getSamplesPerPixel();
+        _drawAxis = frameData.getRenderSettings().getDrawAxis( );


I think the motivation for this duplication was to isolate the renderer from the Equalizer related classes.

hernando · 2016-10-20T12:15:34Z

livre/eq/render/RayCastRenderer.cpp

+        LBASSERT( program );
+        glUseProgram( program );
+
+        GLint tParamNameGL = glGetUniformLocation( program, "renderTexture" );


tParamNameGL -> location?
Unless it's inconsistent with the style all around.

It's everywhere like that... Unless I change it everywhere in Livre I will keep it like that

hernando · 2016-10-20T12:19:22Z

livre/eq/render/shaders/fragAxis.glsl

+flat in uint type;
+
+// http://www.opengl.org/wiki/Compute_eye_space_from_window_space.
+vec4 calcPositionInEyeSpaceFromWindowSpace( vec4 windowSpace )


Why don't you pass the eye position from the vertex shader? I know that increasing the number of variables passed between shader limits the scalability, but that's not an issue for the axis rendering.

It's the fragment position in eye coordinate, not the camera position.

I understood that, my point is that in the vertex shader you can multiply the vertex coordinate by the modelview and pass that to the fragment shader. Then you don't need to unproject here.

I really don't get it... I need to multiply vertex coordinate by modelViewProjectionMatrix otherwise my fragments positions will be wrong.

hernando · 2016-10-20T12:20:16Z

livre/eq/render/shaders/fragAxis.glsl

+
+    vec4 dst = imageLoad( renderTexture, ivec2( gl_FragCoord.xy ));
+
+    if( type == 1 )


What is type 1?

Done. I put a #define in the shader AXIS = 1 so it's like the enum in c++ code.

hernando · 2016-10-20T12:20:52Z

livre/eq/render/shaders/fragAxis.glsl

+    if( type == 1 )
+    {
+        if(!((( dot( pixelEyeSpacePos, normal ) <= 0.0 ) && ( dot( pixelEyeSpacePos, normal2 ) <= 0.0 )) ||
+             (( dot( pixelEyeSpacePos, normal ) >= 0.0 ) && ( dot( pixelEyeSpacePos, normal2 ) >= 0.0 ))))


A comment that explains what all these operations are checking would be helpful.

Move the dot products out of the if and assign them to a const variable. You have the same operation 3 times. This is not about efficiency as much as about readability.

Also I prefer avoiding expressions like !(.....) if they can be rewritten without the !.
In this example you have:

!(( A <= 0 && B <= 0 ) || ( A >=0 && B >= 0))
!( A <= 0 && B <= 0 ) && !( A >= 0 && B >= 0)
( A >= 0 || B >= 0) && ( A <= 0 || B <= 0)

As you can see, there are also unnecessary parenthesis around the comparison operations, which I think don't help making the expression clearer.

A <= 0 and A >= 0 are one the negation of the other, do you need both comparisons to test for equality to 0? Could you replace both with a single variable a A >= 0 (normalPointsForward, or something similar) and use A and !A instead?

I don't get the "Could you replace both with a single variable a A >= 0 (normalPointsForward, or something similar)"

The code looks like that now:
if(( dotNormal > 0.0 ) && ( dotNormal2 < 0.0 ) ||
( dotNormal < 0.0 ) && ( dotNormal2 > 0.0 ))

hernando · 2016-10-20T12:55:51Z

livre/lib/data/BoundingAxis.cpp

+    float tickSize = 0;
+
+    const float rangeLenght = range.y() - range.x();
+    const float rawTickLenght = rangeLenght / maxTickNumber;


I'm a bit lost with these names. This is the distance between tickets in volume coordinates, why do you call it rawTickLength?

No. This is the distance between ticks if you don't do the 1 2 5 fitting magic... it's why I called it "raw". It's an abstract intermediate mathematical result... I am not sure a better name can be found.

tickDistanceHint

hernando · 2016-10-20T12:57:40Z

livre/lib/data/BoundingAxis.cpp

+    const float z = volInfo.worldSize.z() * 0.5f;
+
+    _createLocalTicks( volInfo, { -x, -y, z }, { x, -y, z }, { 0, 1, 0 }, { 0, 0, -1 },
+                       false, tickSize, factor, vertices, normals, normals2, colors, types );


Create a struct for all the parameters that do not change (including volInfo).

Why ? Maybe I don't undestand the comment because for me if I add a struct it will only increase the size of code.

hernando · 2016-10-20T12:58:49Z

livre/lib/data/BoundingAxis.cpp

+        Vector3f posOnAxis = start + axisDir * ( worldSpaceOffset + worldSpaceTickSize * i );
+        vertices.push_back( posOnAxis.x( ));
+        vertices.push_back( posOnAxis.y( ));
+        vertices.push_back( posOnAxis.z( ));


write a helper funtion to write: append(vertices, posOnAxis)

hernando · 2016-10-20T13:01:59Z

livre/lib/data/BoundingAxis.cpp

+                       false, tickSize, factor, vertices, normals, normals2, colors, types );
+    _createLocalTicks( volInfo, { -x, -y, z }, { x, -y, z }, { 0, 0, -1 }, { 0, 1, 0 },
+                       true, tickSize, factor, vertices, normals, normals2, colors, types );
+


Write a comment before each block to know which face are you adding.

hernando · 2016-10-20T13:06:41Z

livre/lib/data/BoundingAxis.cpp

+    glGenBuffers( 1, &_typeBufferId );
+    glBindBuffer( GL_ARRAY_BUFFER, _typeBufferId );
+    glBufferData( GL_ARRAY_BUFFER, sizeof(GLubyte) * types.size(),
+                  &types[0], GL_STATIC_DRAW );


Normally, a single VBO is used for all the data so you don't have to rebind a new buffer to set each vertex attrib pointer. You can use glBufferSubData to upload the pieces one by one.

Approved by mistake

hernando · 2016-10-20T13:10:32Z

Strange, I thought that dismissing the review would put the PR in a non-mergeable state, but that's not the case (at least I can merge it, I don't know if @chevtche can)

chevtche · 2016-10-27T13:04:32Z

Retest this please

hernando · 2016-11-01T16:12:26Z

livre/eq/render/RayCastRenderer.cpp

+        glUniformMatrix4fv( tParamNameGL, 1, false, frustum.getNormalMatrix( ).array );
+
+        tParamNameGL = glGetUniformLocation( program, "invProjectionMatrix" );
+        glUniformMatrix4fv( tParamNameGL, 1, false, frustum.getInvProjMatrix( ).array );


After removing calcPositionInEyeSpaceFromWindowSpace you don't need this, do you?

Yes I need it in the shader.
"vec3 pixelWorldSpacePos = vec3(( invModelViewMatrix * pixelEyeSpacePos ).xyz );"

But not for the axis, right?

True, done.

hernando · 2016-11-01T16:16:13Z

livre/lib/data/BoundingAxis.cpp

+    }
+
+    // There is a possible optimization here, it is possible to append directly
+    // in the code lines [ 50 - 89 ].


Why aren't you doing it? It seems trivial and you reduce the verbosity of the code. You can do it simply by:

std::vector< Vector3f >& vertices = bbAxisData.vertices; ....

This technique will not work for bbAxisData.normals, bbAxisData.normals2, bbAxisData.color, bbAxisData.types. There is a way to do it for everything in the loop but it will take more time and the code will be even more complicated. Here you are only copying 48 vertexes...

I just put a comment for future work.

Remove the comment then, the optimization is quite negligible.

hernando · 2016-11-01T16:18:31Z

You should have done a new commit for the CR changes, now it's difficult to keep track of what you changed.

hernando · 2016-11-01T16:22:18Z

livre/lib/data/BoundingAxis.cpp

+        currentAxis = Vector3f( end - start ).find_min_index();
+
+    const Vector2f range = _computeRange( bbAxisData.volInfo, currentAxis );
+    const float rangeLenght = range.y() - range.x();


Still same typo: Lenght -> Length

hernando · 2016-11-01T16:22:34Z

livre/lib/data/BoundingAxis.cpp

+    const Vector2f range = _computeRange( bbAxisData.volInfo, currentAxis );
+    const float rangeLenght = range.y() - range.x();
+
+    const float worldSpaceLenght = start.distance( end );


And also here.

hernando · 2016-11-01T16:24:32Z

livre/lib/data/BoundingAxis.cpp

+        if( flipTick )
+            sign = -(( 0.0f < cross[ tickAxis ]) - ( cross[ tickAxis ] < 0.0f ));
+        else
+            sign = ( 0.0f < cross[ tickAxis ]) - ( cross[ tickAxis ] < 0.0f );


Replace lines 312-325 by:

int sign = ( 0.0f < cross[ tickAxis ]) - ( cross[ tickAxis ] < 0.0f ); if( flipTick ) sign = -sign;

hernando · 2016-11-01T16:25:50Z

livre/lib/data/BoundingAxis.cpp

+    container.push_back( vector.x( ));
+    container.push_back( vector.y( ));
+    container.push_back( vector.z( ));
+    container.push_back( vector.w( ));


Make the _appendXdVector functions free functions inside an anonymous namespace, it doesn't make sense to make them member functions.

Why cannot it be a member function ?

It's not that it can't.
When a "member" function doesn't need any state of the object to which it belongs it's better to make it a free function to keep the class interface clean. In this case the member function is even private, so making it a free function contained in the .cpp allows you to change it in the future without touching the .h file.

hernando · 2016-11-01T16:28:12Z

livre/lib/data/BoundingAxis.cpp

+    const Vector2f range = _computeRange( bbAxisData.volInfo, longestBBAxis );
+
+    const uint32_t maxTickNumber = 50;
+    const float tickSizes[4] = { 1, 2, 5, 10 };


This should be called tickDistances for consistency, shouldn't it? And the same goes for bbAxisData.tickSize.

Changed everything to distance.

hernando · 2016-11-02T11:52:28Z

Retest this please

chevtche · 2016-11-02T12:06:03Z

Merge or not merge ?

eile suggested changes Oct 18, 2016

View reviewed changes

chevtche assigned hernando Oct 18, 2016

eile previously approved these changes Oct 19, 2016

View reviewed changes

chevtche force-pushed the master branch from 0745280 to cbb8057 Compare October 19, 2016 13:03

hernando reviewed Oct 20, 2016

View reviewed changes

chevtche force-pushed the master branch from cbb8057 to 9e91748 Compare October 27, 2016 12:28

hernando reviewed Nov 1, 2016

View reviewed changes

chevtche added a commit to chevtche/Livre that referenced this pull request Nov 2, 2016

CR BlueBrain#346

f5582a4

chevtche force-pushed the master branch from f5582a4 to 208eace Compare November 2, 2016 10:40

chevtche added a commit to chevtche/Livre that referenced this pull request Nov 2, 2016

CR BlueBrain#346

208eace

chevtche force-pushed the master branch from 208eace to 9181499 Compare November 2, 2016 11:03

chevtche added a commit to chevtche/Livre that referenced this pull request Nov 2, 2016

CR BlueBrain#346

9181499

hernando pushed a commit to hernando/Livre that referenced this pull request Nov 2, 2016

CR BlueBrain#346

4a4a575

hernando approved these changes Nov 2, 2016

View reviewed changes

chevtche added 2 commits November 2, 2016 14:26

Implement axis and legend for full volume box

07ea075

CR BlueBrain#346

786ebde

chevtche force-pushed the master branch from ef57b98 to 786ebde Compare November 2, 2016 13:26

hernando merged commit fd7ad22 into BlueBrain:master Nov 2, 2016


		vec4 dst = imageLoad( renderTexture, ivec2( gl_FragCoord.xy ));

		if( type == 1 )

Axis drawing is working #346

Axis drawing is working #346

Conversation

chevtche commented Oct 18, 2016

eile commented Oct 18, 2016

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

eile commented Oct 19, 2016

hernando 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

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

hernando commented Oct 20, 2016

chevtche commented Oct 27, 2016

Choose a reason for hiding this comment

chevtche Nov 1, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hernando commented Nov 1, 2016

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

hernando commented Nov 2, 2016

chevtche commented Nov 2, 2016

chevtche Nov 1, 2016 •

edited

Loading