HI Alok, I've now merged your map implementation into svn/trunk. I've renamed GLStaticLibrary_map.cpp to GLStaticLibrary.cpp and added the .h and .cpp into the src/osg/CMakeLists.txt. I don't have a static version of GLES2 to test against so I'll have to defer to you on this count.
One thing I feel that we could improve the code further would be to use a macro for each of that static function mappings. It's well in the evening here so I'll not attempt to code change the code right now, next week I'll have a bash at illustrating what I mean. As things stand if code works for you then great - we can polish it later. Cheers, Robert. On Fri, May 14, 2010 at 2:34 PM, Robert Osfield <[email protected]> wrote: > Hi Alok, > > Sorry for not getting onto reviewing/merging this submission sooner. > I've been away, then bug fix most of this week. I'm about to head out > for few hours, but this evening or tomorrow should have an window of > opportunity to review your submission. My expectation is that one of > combinations should be fine for merging, and if not I'll do the tweaks > at this end to get it right. > > Cheers, > Robert. > > On Fri, May 14, 2010 at 2:27 PM, Alok Priyadarshi <[email protected]> wrote: >> Hi Robert, >> Just wanted to send you a reminder about this submission. Our group >> works against the OSG svn head so it will make it easier for us if >> this is submitted. Please let me know if you want me to make further >> changes. >> >> Thanks, >> -Alok >> >> On Thu, Apr 29, 2010 at 4:30 PM, Alok Priyadarshi <[email protected]> wrote: >>> Hi Robert, >>> >>> I am attaching another patch to add a helper class GLStaticLibrary. I >>> have also moved GLStaticLibrary.h into src/osg as you suggested. >>> >>> I am providing two implementations of GLStaticLibrary - >>> GLStaticLibrary_map.cpp and GLStaticLibrary_table.cpp. Please rename >>> whichever one you like to GLStaticLibrary.cpp and submit. If you want >>> me to make additional changes let me know. >>> >>> GLStaticLibrary_map.cpp uses an std::map<functionName, >>> functionPointer> that gets initialized once and queried for each >>> functionName. >>> Pros: No need to keep function-table sorted manually >>> Cons: Needs explicit thread-safety. Inefficient O(n^2 log^2n) on the >>> assumption that you typically search for a function only once. Putting >>> a function in a map to be searched only once is an overkill. >>> >>> GLStaticLibrary_table.cpp uses a sorted static array of >>> std::pair<functionName, functionPointer> that gets initialized at >>> startup and queried by std::lower_bound for each functionName. >>> Pros: Inherently thread-safe. Efficient O(nlogn) >>> Cons: The function table has to be sorted. I do not think this is a >>> big deal because you will not be changing this table everyday. In fact >>> it is already fixed for old versions of GL. I auto-generated the table >>> using a script to make sure it is sorted. We can check-in the script >>> if you want. If you do not like the sorted requirement, we can change >>> use a linear search algorithm (std::find_if) which will still be >>> faster O(n^2) than the map - again on the assumption that you >>> typically search for a function only once. >>> >>> I personally like the table implementation but I am fine with either one. >>> >>> On Wed, Apr 28, 2010 at 3:32 PM, Robert Osfield >>> <[email protected]> wrote: >>>> Hi Alok, >>>> >>>> Thanks fort the changes. I've dived in a done a review of the changes >>>> and have gone ahead and merged the APIENTTRY to GL_APIENTRY changes. >>>> These are now checked into svn/trunk. >>>> >>>> I don't feel the GLES2StaticLibrary class is quite right as it stands. >>>> First up I see this as an internal implementation details rather than >>>> an public interface so there shouldn't be able class or functions in >>>> the include/osg directory, all the implementation should be kept >>>> private in the src/osg implementation directory. I'm reasonably >>>> comfortable with the general approach of having a helper class, but >>>> it's just a case of getting the implementation right. >>>> >>>> I also don't see the static GL library as just a GLES2 issue, so >>>> rather than merge the CMake changes that just reference GLES2 I've >>>> checked in a general OSG_GL_LIBRARY_STATIC variable and associated >>>> #define, this leaves the door open to static versions of any of the GL >>>> libs. >>>> >>>> I think the next step would be to implement the GLESStaticLibrary >>>> function mapping as an std::map<> as it will make the implement more >>>> straight forward and easier to maintain, and we can also easily extend >>>> it so the contents of the std::map<> are provided for different static >>>> GL/GLES targets. One thing we'll need to be mindful of is the thread >>>> safety of initialization the std::map. >>>> >>>> Cheers, >>>> Robert. >>>> >>>> On Wed, Apr 28, 2010 at 8:55 PM, Alok Priyadarshi <[email protected]> >>>> wrote: >>>>> Hi Robert, >>>>> >>>>> I have attached my changes to statically link gles2 lib. There are two >>>>> main changes: >>>>> 1. Replaced APIENTRY to GL_APIENTRY which is used by OpenGL ES >>>>> headers. For desktop GL GL_APIENTRY has been defined as APIENTRY. >>>>> 2. Added GLES2StaticLibrary class that provides proc-address lookup >>>>> given a proc name. >>>>> >>>>> Not perfect, but this is the least intrusive option I could think of. >>>>> >>>>> Thanks, >>>>> -Alok >>>>> >>>>> _______________________________________________ >>>>> 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
