OpenSceneGraph Forum Forum Index OpenSceneGraph Forum
Official forum which mirrors the existent OSG mailing lists. Messages posted here are forwarded to the mailing list and vice versa.
 
   FAQFAQ    SearchSearch    MemberlistMemberlist    RulesRules    UsergroupsUsergroups    RegisterRegister 
 Mail2Forum SettingsMail2Forum Settings  ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 
   AlbumAlbum  OpenSceneGraph IRC ChatOpenSceneGraph IRC Chat   SmartFeedSmartFeed 

serializer bug in osgText::TextBase and osg::Camera


 
Post new topic   Reply to topic    OpenSceneGraph Forum Forum Index -> Submission
View previous topic :: View next topic  
Author Message
locan
Newbie


Joined: 10 Aug 2011
Posts: 7

PostPosted: Sun Feb 22, 2015 10:00 pm    Post subject:
serializer bug in osgText::TextBase and osg::Camera
Reply with quote

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

------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
robertosfield
OSG Project Lead


Joined: 18 Mar 2009
Posts: 12206

PostPosted: Mon Feb 23, 2015 11:54 am    Post subject:
serializer bug in osgText::TextBase and osg::Camera
Reply with quote

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 < (
Only registered users can see emails on this board!
Get registred or enter the forums!
)> wrote:
Quote:
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
(
Only registered users can see emails on this board!
Get registred or enter the forums!
)
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org



------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
locan
Newbie


Joined: 10 Aug 2011
Posts: 7

PostPosted: Mon Mar 02, 2015 11:21 pm    Post subject:
serializer bug in osgText::TextBase and osg::Camera
Reply with quote

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 < (
Only registered users can see emails on this board!
Get registred or enter the forums!
)> wrote:
Quote:
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 < (
Only registered users can see emails on this board!
Get registred or enter the forums!
)> wrote:


Quote:
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
(
Only registered users can see emails on this board!
Get registred or enter the forums!
)
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org





_______________________________________________
osg-submissions mailing list
(
Only registered users can see emails on this board!
Get registred or enter the forums!
)
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org



------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
robertosfield
OSG Project Lead


Joined: 18 Mar 2009
Posts: 12206

PostPosted: Tue Mar 03, 2015 11:16 am    Post subject:
serializer bug in osgText::TextBase and osg::Camera
Reply with quote

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 < (
Only registered users can see emails on this board!
Get registred or enter the forums!
)> wrote:
Quote:
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 < (
Only registered users can see emails on this board!
Get registred or enter the forums!
)> wrote:
Quote:
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 < (
Only registered users can see emails on this board!
Get registred or enter the forums!
)> wrote:


Quote:
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
(
Only registered users can see emails on this board!
Get registred or enter the forums!
)
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org





_______________________________________________
osg-submissions mailing list
(
Only registered users can see emails on this board!
Get registred or enter the forums!
)
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org







_______________________________________________
osg-submissions mailing list
(
Only registered users can see emails on this board!
Get registred or enter the forums!
)
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org



------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
Display posts from previous:   
Post new topic   Reply to topic    OpenSceneGraph Forum Forum Index -> Submission All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
You cannot attach files in this forum
You cannot download files in this forum

Similar Topics
Topic Author Forum Replies Posted
No new posts How to change camera's yaw and pitch,... Rodrigo General [3rdparty] 3 Thu Jan 31, 2019 12:43 am View latest post
No new posts setViewMatrixAsLookAt not working to ... Rodrigo General 4 Tue Jan 08, 2019 2:48 am View latest post
No new posts Scale object based on camera distance Moohasha General 3 Fri Jan 04, 2019 4:57 pm View latest post
No new posts Attach TextureBuffer to camera? robs General 0 Fri Dec 28, 2018 9:50 pm View latest post
No new posts osgText crash with multiple Viewers ravidavi General 3 Tue Dec 18, 2018 8:50 am View latest post


Board Security Anti Bot Question MOD - phpBB MOD against Spam Bots
Powered by phpBB © 2001, 2005 phpBB Group
Protected by Anti-Spam ACP