Hi Miha, Excellent work - both on reproducing the problem in a really coherent way and providing a what looks to be a fully backwards compatible fix. I have merged the changes and checked them into svn/trunk.
The only tweak I had to make was to move the StringList and split defintions from ObjectWrapper to the Serializer header, and add in replace _name usage to ParentType::_name to get things to compile with my g++4.9.1 compiler. Cheers, Robert. On 2 March 2015 at 23:14, Miha Ravšelj <[email protected]> wrote: > Hi Robert, > > finally I managed to find some time to reply to your previous mail. > > Regarding previous submission it was only partial solution. After further > testing I found similar bug also in ClearNode serializer. > > //GLbitfield mask = GL_COLOR_BUFFER_BIT|GL_DEPTH_BUFFER_BIT; > This line was problematic since it produced incorrect result when let's > say COLOR flag is serialized > it should be null as in Camera serializer or in a proposed > BitFlagsSerializer > > > This line of code caused that whenever only GL_COLOR_BUFFER_BIT bit was > written and on value read GL_COLOR_BUFFER_BIT|GL_DEPTH_BUFFER_BIT was > restored instead of GL_COLOR_BUFFER_BIT only. > > //GLbitfield mask = 0; //this resolves the issue same as in camera > Also same bit-wise comparison bug was also present in write method. > > ------------------------------------------------------------------------------------- > > As you can see there are total 3 bit mask serializers in OSG and all 3 had > bugs so I decided to add ADD_BITFLAGS_SERIALIZER and replace USER > serializers in osg::Camera, osg::ClearNode and osgText::TextBase. I have > made sure that bitflags serializer does not break backwards-compatibility > since it uses same code as user serializer does in all 3 cases. (see > tester.cpp on how compatibility test was performed) > > Attached patch file is against current trunk( 14749 ). > > > Cheers, > Miha > > On Mon, Feb 23, 2015 at 12:46 PM, Robert Osfield <[email protected] > > wrote: > >> Hi Miha, >> >> Thanks for spotting and resolving this bug. I have done a quick review >> of the proposed serializer but at this point haven't done a close enough >> review to figure out the consequences for backwards compatibility of this >> change. Any thoughts on this? >> >> To resolve the bug I have merged your other changes with a small tweak >> and comparing the bit mask result against !=0 rather than the repeating the >> enum. This little tweak reduces the amount of code and should make it less >> likely a copy and paste style bug might creep in at a later date. >> >> The changes that I went for are: >> >> $ svn diff >> Index: src/osgWrappers/serializers/osg/Camera.cpp >> =================================================================== >> --- src/osgWrappers/serializers/osg/Camera.cpp (revision 14710) >> +++ src/osgWrappers/serializers/osg/Camera.cpp (working copy) >> @@ -143,10 +143,10 @@ >> else >> { >> std::string maskString; >> - if ( mask==GL_COLOR_BUFFER_BIT ) maskString += >> std::string("COLOR|"); >> - if ( mask==GL_DEPTH_BUFFER_BIT ) maskString += >> std::string("DEPTH|"); >> - if ( mask==GL_ACCUM_BUFFER_BIT ) maskString += >> std::string("ACCUM|"); >> - if ( mask==GL_STENCIL_BUFFER_BIT ) maskString += >> std::string("STENCIL|"); >> + if ( (mask & GL_COLOR_BUFFER_BIT)!=0 ) maskString += >> std::string("COLOR|"); >> + if ( (mask & GL_DEPTH_BUFFER_BIT)!=0 ) maskString += >> std::string("DEPTH|"); >> + if ( (mask & GL_ACCUM_BUFFER_BIT)!=0 ) maskString += >> std::string("ACCUM|"); >> + if ( (mask & GL_STENCIL_BUFFER_BIT)!=0 ) maskString += >> std::string("STENCIL|"); >> if ( !maskString.size() ) maskString = std::string("NONE|"); >> os << maskString.substr(0, maskString.size()-1) << std::endl; >> } >> Index: src/osgWrappers/serializers/osgText/TextBase.cpp >> =================================================================== >> --- src/osgWrappers/serializers/osgText/TextBase.cpp (revision 14710) >> +++ src/osgWrappers/serializers/osgText/TextBase.cpp (working copy) >> @@ -159,10 +159,10 @@ >> else >> { >> std::string maskString; >> - if ( mask==osgText::TextBase::TEXT ) maskString += >> std::string("TEXT|"); >> - if ( mask==osgText::TextBase::BOUNDINGBOX ) maskString += >> std::string("BOUND|"); >> - if ( mask==osgText::TextBase::FILLEDBOUNDINGBOX ) maskString += >> std::string("FILLED|"); >> - if ( mask==osgText::TextBase::ALIGNMENT ) maskString += >> std::string("ALIGNMENT|"); >> + if ( (mask&osgText::TextBase::TEXT)!=0 ) maskString += >> std::string("TEXT|"); >> + if ( (mask&osgText::TextBase::BOUNDINGBOX)!=0 ) maskString += >> std::string("BOUND|"); >> + if ( (mask&osgText::TextBase::FILLEDBOUNDINGBOX)!=0 ) maskString >> += std::string("FILLED|"); >> + if ( (mask&osgText::TextBase::ALIGNMENT)!=0 ) maskString += >> std::string("ALIGNMENT|"); >> if ( !maskString.size() ) maskString = std::string("NONE|"); >> os << maskString.substr(0, maskString.size()-1) << std::endl; >> } >> >> >> Cheers, >> Robert. >> >> >> On 22 February 2015 at 21:53, Miha Ravšelj <[email protected]> >> wrote: >> >>> Hello, >>> >>> I have been testing I/O operation on osgText::Text and I discovered that >>> writing to osgt format does not work correctly due to invalid bit-wise >>> comparison. >>> >>> I checked all the serializers for similar user serializers and found >>> that also osg::Camera has same bug in writeClearMask. I didn't found any >>> other serializer but if there are any that you are aware of (beside >>> TextBase and Camera) they should be checked as well. >>> >>> The attached code in openscenegraph.zip resolves this issue in both >>> serializers. >>> >>> I have also attached BitFlagsSerializer.h as a proposal for serializing >>> bit flags . If there are multiple bitflags that are serialized inside OSG >>> then I believe it is better if system serializer is added and used instead >>> of producing same or similar code in user serializer(s). >>> >>> Miha >>> >>> _______________________________________________ >>> 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
