Hi Ralf, Sorry for the slow review. Slowly clearing the backlog now that I have some free time.
I've done a review of the osgUtil::ShaderGen changes and broadly they are in keeping with the existing ShaderGen but it really jumps out that ShaderGen really would be much easier to develop and improve it was re-implemented with #pragma(tic) shader composition. What the code should have is a single vertex and fragment program that uses #ifdef's to toggle on/off the various features in the shader, then have the ShaderGen class just set the #define's via StateSet::setDefine() etc. This approach would be much cleaner and easier to maintain, and would also work more seamlessly with the final shader_pipeline work that I've been experimenting with. At this point I don't think it makes sense to add more complexity to ShaderGen as it'll just make it harder to rewrite to use #pragma(tic) shader composition. The right approach is to convert it to #psc and then add additional features. I don't know how much work this might be but I'll do a code review of ShaderGen in OSG master now and see how easy it would be to adapt it. If I don't get to this refactor of ShaderGen right away then it would be helpful if others could pitch in. Once it's refactored then we can look at again at extending support for more than just very basic fixed function pipeline like in your proposal. Cheers, Robert. Robert. On 23 October 2017 at 14:03, Ralf Habacker <[email protected]> wrote: > Am 17.10.2017 um 18:06 schrieb Robert Osfield: >> With ShaderGen it's there as stop gap to generate some shaders for a >> subset of the fixed function pipeline that is common in a subset of the >> OSG loaders. It is just a means of getting "something" on the screen on >> platforms like GL core profile and GLESL 2+, it's not an all purpose >> fully functioning solution. > > The appended patch adds build in support of diffuse and specular > textures for obj by extending ShaderGen and the obj plugin. Adding > shaders is disabled by default and needs to be enabled with a plugin > option: > > osgviewer -O useBuildInShader plane-with-uv-map.obj > > I added the changed files and the related git patches which contains > explanations about how this has been implemented. A test obj file has > also be added. The used images are not added but downloadable from > http://www.rastertek.com/dx10tut21.html because I did not find any > related copyright info. > >>With the shader_pipeline work one would need to have a top level shader >>that is able to handle the diffuse and specular texture maps in an >>appropriate way. The current top level shader I've worked on so far is >>OpenSceneGraph-Data/shaders/shaderpipeline.vert and >>shaderpipeline.frag, this is just early days though, but might give you >>an idea where this functionality is going and how one might add support >>for different texturing and lighting approaches. > > Looking at the patch I'm currently not sure if all ShaderGen state mask > combinations are working correctly and the question here is if it would > make more sense to refactor ShaderGen to use define based shaders > instead of adding all the remaining combinations to the recent > implementation. > > All the more because there are already 'define' based shaders available > for osg (see https://github.com/OpenMW/openmw/tree/master/files/shaders) > > Regards > Ralf > > > > > > > > > > _______________________________________________ > osg-submissions mailing list > [email protected] > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org > _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
