Hi Aurelien, I have just done my first pass review and as things stand I don't feel it's appropriate to merge all the changes.
The BufferTemplate class is a simple and useful class that stands on it own, although I am wondering whether there might be an opportunity to create a templated uniform class that wraps. On it's own the BufferTemplate doesn't yet have a direct role so some kind of templated uniform would provide this. I haven't really thought long enough about this yet though, it's jut your example uses the BufferTemplate and UniformBufferBinding together and it felt like perhaps the two could be even more closely coupled. The Pipeline class it's self and it's integration with osg::State and other places in the OSG feels awkward to me, with a CPU overhead for all applications not just ones that actually use it. Given this the hurdle for it getting into the core OSG is much higher - it has to solve a problem a wide range of problems for the majority of users to justify such an overhead. Reviewing the code it felt like Pipeline is almost like osg::State itself in that it is there to track and respond to state changes. This overlap struck me a bit like subclassing of osg::State might be what you are kinda working at, albeit in a side loaded sort of way. My insinct if you want to do this then working out which methods in osg::State need to be made virtual would be appropriate. It might be that full subclassing is actually inappropriate or overkill and that refactoring some of the core functionality of osg::State into a helper class that provided a default implementation and with subclasses to override specific behaviour, this is kinda of like what you have done with osg::Pipeline but you've gone for an approach that adds an extra call to this osg::Pipeline rather than replacing existing behaviours in osg::State with the custom behaviour. Methods like State::applyMode(GLmode) and State::applyAttribute(StateAttribute) and apply(StateSet) seem to be candidates for being made virtual and being allowed to be overriden. Using subclassing would avoid all the if () do_this() type coding that is inefficient on the CPU and awkward to read. There is also the ShaderComposer functionality already built into the OSG that is broader in scope and capabilities, but alas not finished, the osg::Pipeline approach seems quite a distinctly different direction to solving similar problems and would not take as another step nearer to shader composition but add something entirely different which will make it more awkward to reconcile and keep coherent as develop the OSG. It's a while since I was able to work on shader composition so unfortunately I don't have all the issues fresh in my head. I really need to get back into the topic but right now I'm need to complete some work for a client project so can't do this right away. One area that strikes me that may be something interesting to look into is using the BuffertTemplate/UniformBufferBinding in combination with existing fixed function StateAttribute that we want to wrap up to be used by shaders i.e. wrapping up glMaterial or glLight data into. It could also be possible to tweak ShaderComposer to do what you want with osg::Pipeline. I'll need to come back to your submission and look at ShaderComposer functionality another day, often it's good just to sleep on topics. Perhaps others can do a review and look for an opportunities of allowing your to do what you need to do without such intrusive changes to the core OSG and keep us moving towards fully functioning shader composition. Cheers, Robert. _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
