Wireframe shading for SceneShapes, use Cs instead of vertexCs.

[cedriclaunay]

Instead of create a new shader wireframe(), who will be the same as the constant(), in IECoreGL/Shader.cpp. I guess, use the uniform argument "bool vertexCsActive" in defaultVertexSource() is more clear.

Because Shader::Setup is const in IECoreGL/Primitive.cpp, I can't add a uniform Parameter at "constantSetup" ( line:204). So I have choose to add a new argument into Primitive::addPrimitiveVariablesToShaderSetup(), "bool vertexCsActive".

[johnhaddon]

Changing the signature of this virtual method will break binary compatibility. Also, it will prevent the CurvesPrimitive and PointsPrimitive overrides from being picked up (because the signatures no longer match).

[johnhaddon]

I do like the idea of reusing the existing constant shader instead of introducing another one that's almost the same. But in addition to my comments above, I don't think adding the extra arguments to shaderSetup() and addPrimitiveVariablesToShaderSetup() is a good move - those arguments are only of use to the internal Primitive implementation, so we're leaking internal implementation details into a more public API.

[johnhaddon]

This search is now broken - it cannot know whether the setup it finds was created with vertexCsActive == true or not, and can return the wrong one.

[johnhaddon]

Since I can't think of a better alternative, I think we do need to introduce a new shader. I don't think it should be called Shader::wireframe() though, since really it has nothing to do with wireframe - it's just a really cut down constant shader which is only of use to the Primitive class. I suggest just making the shader privately in Primitive.cpp, as we do with constant2().

[cedriclaunay]

Add new wireframe() shader in IECoreGL/Primitive.cpp

[johnhaddon]

I think the name wireframe() is misleading, since we also use it for drawing outlines and points. It also seems that the vertex shader calculates a lot of useless information that the fragment shader doesn't use. Could you fix that please?

[cedriclaunay]

Something like constant3(), seems ok ?

[johnhaddon]

I actually think the constant2() name is rubbish too (it's one of mine) so let's not copy that. How about flatConstant() or uniformConstant(), so that it's a bit more obvious that the difference is that it ignores vertex colours?

[cedriclaunay]

Yes, flatConstant() seems the more clear for me.

[cedriclaunay]

Fixed shader name and shader contents.

[andrewkaufman]

Actually, I merged this already, but one of the tests is failing, which I guess is saying that the "gl:primitive:wireframeColor" state isn't effecting wireframe renders anymore...

======================================================================
FAIL: testWireframeShading (__main__.TestPointsPrimitive)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/IECoreGL/PointsPrimitive.py", line 533, in testWireframeShading
    self.assertEqual( result.floatPrimVar( e.A() ), 1 )
AssertionError: 0.0 != 1

----------------------------------------------------------------------

[andrewkaufman]

Whats strange is that loading up a scene in Gaffer, wireframeColor still seems to work as expected...

Also, replacing the shader construction inside flatConstant() with this lets the test pass:
static ShaderPtr s = new Shader( "", Shader::constantFragmentSource() );

but replacing it with this still fails:
static ShaderPtr s = new Shader( Shader::defaultVertexSource(), Shader::constantFragmentSource() );

and I was under the impression those two statements do the same thing...