Hi Paul, I'm afraid I find your approach for huge blocks of separate code paths awful, it is really is very bad for maintenance as you just can't spot divergence in code, and the multiple code paths will ensure less people will be tested each of the code paths. As for computational efficiency - there is just an extra if statement per function pointer, and for code that is only called one per graphics context during initialization so it's absolutely negligible.
The code absolutely needs to reduce the deviation between the platforms, it the element of the port to these new platforms that will avoid the OSG becoming a spaghetti code base that is next to impossible to maintenance in a reliable way. Robert. On Tue, Nov 10, 2009 at 2:59 PM, Paul Martz <[email protected]> wrote: > Hi Robert -- Thanks for reviewing. > > Before submitting, I did consider that you might reject these changes, as I > had seen your approach and I knew that mine was different. > > I don't see the same value in sharing code paths as you. If the underlying > implementation is different, then the shared code path will _execute_ > differently. In such a case, the shared code path might make things more > confusing to developers who come along later, rather than more clear. > > Your approach follows this pattern: > > gl3corefeature = isGLExtensionSupported("extension that doesn't exist in > GL3"); > getGLExtensionFuncPtr( funcptr, "core name", "arb name", "ext name" ); > #ifdef GL3 > gl3corefeature = true; > #endif > > This is confusing, because the shared code path - used by GL3 - is > unnecessarily querying for the presence of a feature as an extension, when > it is known to exist in the core. Someone casually glancing at the code > might even miss the platform-specific code path at the bottom of the > function, which corrects for this incorrect query. Furthermore, ARB and EXT > entry point names are unnecessarily present, even though, as you admit, they > won't come into play in a correctly implemented GL3. > > Mine breaks this out into two code paths: > > #ifdef GL3 > gl3corefeature = true; > getGLExtensionFuncPtr( funcptr, "core name" ); > #else > gl3corefeature = isGLExtensionSupported("extension that doesn't exist in > GL3"); > getGLExtensionFuncPtr( funcptr, "core name", "arb name", "ext name" ); > #endif > > In my approach, it is easy to see at a glance exactly how the GL3 code path > works. The GL3 path does what's right for GL3, and other paths can be added > as needed for other platforms. > > Both your approach and mine will produce the same results on a correctly > implemented GL3, but yours is actually less efficient, given the unnecessary > checks for extension presence. > > I'd like you to please reconsider my submission and commit it to core as > submitted, if you agree it is a better approach. But of course you have the > final say. > > Paul Martz > Skew Matrix Software LLC > _http://www.skew-matrix.com_ <http://www.skew-matrix.com/> > +1 303 859 9466 > > > > Robert Osfield wrote: >> >> Hi Paul, >> >> I've just reviewed your two sets of GL3 changes and have had to, for >> now, reject most of them as they will just make the code less >> maintainable by duplicating code paths unnecessarily. My approach >> with the GLES support has to been to minimize the number of separate >> code paths between all OpenGL targets as to avoid issues of code >> quality, testing and maintenance. This does mean that one sometimes >> has to look harder for opportunities to share code, or to localize the >> target specific code paths to just a couple of lines, but it is >> possible and in the long run will lead to a better OSG. >> >> For instance, you have often replaced code like: >> >> >> setGLExtensionFuncPtr(_glCompressedTexImage2D,"glCompressedTexImage2D","glCompressedTexImage2DARB"); >> >> With an GL3 specific path that reads: >> >> >> setGLExtensionFuncPtr(_glCompressedTexImage2D,"glCompressedTexImage2D"); >> >> Where this should be totally unnecessary as the original code will >> have done the right things already, as it'll check for the first >> function name and succeded and then never both with the second ARB >> version of the function name. This three parameters version of >> setGLExtensionFuncPtr exist purely to help avoid the need for separate >> code paths. >> >> The changes I was expecting was to adopt the three parameter >> setGLExentionFuncPtr more widely with the advert of GL3 and GLES3 have >> these functions in the core. I was also expecting just to have to set >> the enabling of the feature on by default in very local code segments. >> I've counted 13 files in your submissions that didn't take these >> opportunities, which is frustrating as it leaves us both with more >> work to do to get things in a good enough form to merge. >> >> At my end I'll see where I can easily change the code in svn/trunk to >> enable things for GLES 2 and GL3 where it isn't right now, and use >> your modified files as a guide to where you found problems. Once I've >> done this I'll check the changes in, but as I only have GL2 and GLES2 >> + GLES1.1 available to me right now I won't be able to test the GL3 >> path. Given that I don't have GL3 available I will hang back from >> trying to make all the possible changes required for GL3, so once I've >> checked my changes in you'll need to check the latest in svn/trunk out >> and then review your original changes against svn/trunk and make >> changes to the svn/trunk version that seek to minimize the number of >> lines of code that differ across GL targets. >> >> Cheers, >> Robert. >> >> >> On Mon, Nov 9, 2009 at 12:56 AM, Paul Martz <[email protected]> >> wrote: >>> >>> Hi Robert -- Here's part 2 of this submission, containing the more >>> complex >>> changes to Texture.cpp, State.cpp, and Drawable.cpp. Also a mod to >>> BufferObject.cpp that I previously missed. >>> >>> Again, the basic idea is that, for any feature known to be part of core >>> GL3, >>> don't bother querying for the extension, and be sure to load only the >>> core >>> entry point address. >>> >>> Texture.cpp supersedes the same file in the previous submission. >>> -- >>> Paul Martz >>> Skew Matrix Software LLC >>> _http://www.skew-matrix.com_ <http://www.skew-matrix.com/> >>> +1 303 859 9466 >>> >>> >>> _______________________________________________ >>> 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 >> >> > _______________________________________________ > 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
