Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Robert Osfield]

On 27 April 2016 at 20:20, Andy Skinner  wrote:

> So, to be clear, we'll leave it as is?
>

I am happy to merge this particular one, but if there are more, then just
how many places, ever single modification is a step up in code bloat for
dodgy warnings sake.

Robert
___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Andy Skinner]

So, to be clear, we'll leave it as is?

thanks
andy


Needing to put a using in lots of places across the OSG to just quieten 
inappropriate warnings is pretty crappy.  The more superfluous code you have in 
your code base the less easy it is to read, the easier it to break.  This makes 
me very wary of jumping through hoops to quieten dodgy warnings from the 
compilers.

Robert.



___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Robert Osfield]

Hi Andy,

On 27 April 2016 at 19:09, Andy Skinner  wrote:

> Yes, the "using" statement does quiet the warning.
>

Good to hear.


>
>
> Does that mean this is the solution, or that this just shows that there
> isn't a problem?  The reason I ask is that we've discussed the "using"
> statement for things like this before, and I don't believe you were in
> favor of it.  But using it would let us remove a bunch of warning handling
> in our use of OSG.
>
>
Needing to put a using in lots of places across the OSG to just quieten
inappropriate warnings is pretty crappy.  The more superfluous code you
have in your code base the less easy it is to read, the easier it to
break.  This makes me very wary of jumping through hoops to quieten dodgy
warnings from the compilers.

Robert.
___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Andy Skinner]

Yes, the "using" statement does quiet the warning.

Does that mean this is the solution, or that this just shows that there isn't a 
problem?  The reason I ask is that we've discussed the "using" statement for 
things like this before, and I don't believe you were in favor of it.  But 
using it would let us remove a bunch of warning handling in our use of OSG.

thanks
andy

From: osg-users [mailto:osg-users-boun...@lists.openscenegraph.org] On Behalf 
Of Robert Osfield
Sent: Wednesday, April 27, 2016 5:55 AM
To: OpenSceneGraph Users <osg-users@lists.openscenegraph.org>
Subject: Re: [osg-users] ShaderComposer::releaseGLObjects warning

Hi Andy,

On 26 April 2016 at 20:31, Andy Skinner 
<andy.skin...@mathworks.com<mailto:andy.skin...@mathworks.com>> wrote:
Thanks for that fix.  There is a remaining issue, and I'm not sure if you would 
see it as a bug or a dodgy compiler warning.  :)

We get a similar message about osgUtil::CullVisitor::clone().

NodeVisitor uses META_Object, which brings in:
virtual osg::Object* clone(const osg::CopyOp& copyop) const { return new name 
(*this,copyop); }

But CullVisitor defines clone as:
virtual CullVisitor* clone() const { return new CullVisitor(*this); }

They differ in whether they take an argument.  I believe we've discussed this 
kind of thing before when considering using "using", and it was, if I remember 
correctly, a dodgy compiler warning then.


The osg::ShaderComposer warning did highlight a bug, but this warning isn't 
highlighting an actual error.  If one calls clone(osg::CopyOp) then you'll 
still git the correct clone() method.  It does highlight what is not ideal 
coding style, and in this case it's a historical reason why there is this 
clone() convenience method.  These days a osg::clone(object) would do just as 
well without requiring the extra method, but this is a relatively modern 
addition.
Could you try doing adding:
   using NodeVisitor::clone;
To the include/osgUtil/CullVisitor and see if that quietens the warning.
Robert.


___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Robert Osfield]

Hi Andy,

On 26 April 2016 at 20:31, Andy Skinner  wrote:

> Thanks for that fix.  There is a remaining issue, and I'm not sure if you
> would see it as a bug or a dodgy compiler warning.  :)
>
>
>
> We get a similar message about osgUtil::CullVisitor::clone().
>
>
>
> NodeVisitor uses META_Object, which brings in:
>
> virtual osg::Object* clone(const osg::CopyOp& copyop) const { return new
> name (*this,copyop); }
>
>
>
> But CullVisitor defines clone as:
>
> virtual CullVisitor* clone() const { return new
> CullVisitor(*this); }
>
>
>
> They differ in whether they take an argument.  I believe we've discussed
> this kind of thing before when considering using "using", and it was, if I
> remember correctly, a dodgy compiler warning then.
>
>
The osg::ShaderComposer warning did highlight a bug, but this warning isn't
highlighting an actual error.  If one calls clone(osg::CopyOp) then you'll
still git the correct clone() method.  It does highlight what is not ideal
coding style, and in this case it's a historical reason why there is this
clone() convenience method.  These days a osg::clone(object) would do just
as well without requiring the extra method, but this is a relatively modern
addition.

Could you try doing adding:

   using NodeVisitor::clone;

To the include/osgUtil/CullVisitor and see if that quietens the warning.

Robert.
___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Andy Skinner]

Thanks for that fix.  There is a remaining issue, and I'm not sure if you would 
see it as a bug or a dodgy compiler warning.  :)

We get a similar message about osgUtil::CullVisitor::clone().

NodeVisitor uses META_Object, which brings in:
virtual osg::Object* clone(const osg::CopyOp& copyop) const { return new name 
(*this,copyop); }

But CullVisitor defines clone as:
virtual CullVisitor* clone() const { return new CullVisitor(*this); }

They differ in whether they take an argument.  I believe we've discussed this 
kind of thing before when considering using "using", and it was, if I remember 
correctly, a dodgy compiler warning then.

thanks
andy


From: osg-users [mailto:osg-users-boun...@lists.openscenegraph.org] On Behalf 
Of Robert Osfield
Sent: Monday, April 18, 2016 4:05 PM
To: OpenSceneGraph Users <osg-users@lists.openscenegraph.org>
Subject: Re: [osg-users] ShaderComposer::releaseGLObjects warning

On 18 April 2016 at 20:30, Robert Osfield 
<robert.osfi...@gmail.com<mailto:robert.osfi...@gmail.com>> wrote:
Hi Jannik and Ulrich,
You are right it's a bug, the missing const is an error, rather than the 
missing virtual.  I've fixed this and rewritten the implementation of 
ShaderCompose::releaseGLObjects() const.
I need to do a build and test before checking it in.

Now checked into OpenSceneGraph-3.4 and master.
I am bit concerned that this fix breaks the ABI of 3.4 though...  Would 
changing const affect the ABI?  I'm thinking yes, but it's too late in the 
evening for me to go and search for an answer...
Robert.

___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Robert Osfield]

Hi Alberto,

On 25 April 2016 at 19:22, Alberto Luaces  wrote:

> Indeed it does, for example on my system the signature of the method
> changes from
>
> _ZN3osg14ShaderComposer16releaseGLObjectsEPNS_5StateE
>
> to
>
> _ZNK3osg14ShaderComposer16releaseGLObjectsEPNS_5StateE
>

What's the best way to check the signature?



> therefore old programs will not run with this change.  I suggest to
> additionally keep also the old, non-const method, since they can
> co-exist.  Old binaries will expose the bug, but at least they will
> be able to execute.
>


To the 3.4 branch I've just made the change:

 $ git diff
diff --git a/include/osg/ShaderComposer b/include/osg/ShaderComposer
index 68c6d6c..f39d092 100644
--- a/include/osg/ShaderComposer
+++ b/include/osg/ShaderComposer
@@ -39,6 +39,9 @@ class OSG_EXPORT ShaderComposer : public osg::Object
 virtual osg::Shader* composeMain(const Shaders& shaders);
 virtual void addShaderToProgram(Program* program, const Shaders&
shaders);

+/// kept for backward compatibility
+void releaseGLObjects(osg::State* state) { static_cast(this)->releaseGLObjects(state); }
+
 virtual void releaseGLObjects(osg::State* state) const;

 protected:


Do you think this will be sufficient?

Robert.
___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Alberto Luaces]

Robert Osfield writes:

> On 18 April 2016 at 20:30, Robert Osfield  wrote:
>
>> Hi Jannik and Ulrich,
>>
>> You are right it's a bug, the missing const is an error, rather than the
>> missing virtual.  I've fixed this and rewritten the implementation of
>> ShaderCompose::releaseGLObjects() const.
>>
>> I need to do a build and test before checking it in.
>>
>
> Now checked into OpenSceneGraph-3.4 and master.
>
> I am bit concerned that this fix breaks the ABI of 3.4 though...  Would
> changing const affect the ABI?  I'm thinking yes, but it's too late in the
> evening for me to go and search for an answer...

Indeed it does, for example on my system the signature of the method
changes from

_ZN3osg14ShaderComposer16releaseGLObjectsEPNS_5StateE

to

_ZNK3osg14ShaderComposer16releaseGLObjectsEPNS_5StateE

therefore old programs will not run with this change.  I suggest to
additionally keep also the old, non-const method, since they can
co-exist.  Old binaries will expose the bug, but at least they will
be able to execute.

-- 
Alberto

___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Robert Osfield]

On 18 April 2016 at 20:30, Robert Osfield  wrote:

> Hi Jannik and Ulrich,
>
> You are right it's a bug, the missing const is an error, rather than the
> missing virtual.  I've fixed this and rewritten the implementation of
> ShaderCompose::releaseGLObjects() const.
>
> I need to do a build and test before checking it in.
>

Now checked into OpenSceneGraph-3.4 and master.

I am bit concerned that this fix breaks the ABI of 3.4 though...  Would
changing const affect the ABI?  I'm thinking yes, but it's too late in the
evening for me to go and search for an answer...

Robert.
___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Robert Osfield]

Hi Jannik and Ulrich,

You are right it's a bug, the missing const is an error, rather than the
missing virtual.  I've fixed this and rewritten the implementation of
ShaderCompose::releaseGLObjects() const.

I need to do a build and test before checking it in.

Thankfully ShaderComposer is a not widely used, and deprecated thanks to
#pragmatic shader composition so this error shouldn't effect too many users.

Robert.

On 18 April 2016 at 20:02, Ulrich Hertlein  wrote:

> Hi guys,
>
> On 18/04/2016 20:22, Robert Osfield wrote:
> > On 18 April 2016 at 17:39, Andy Skinner  > > wrote:
> >
> > We are getting a warning for ShaderComposer::releaseGLObjects: 'void
> > osg::ShaderComposer::releaseGLObjects(osg::State *)' : member
> function does not
> > override any base class virtual member function
>  osg::Object::releaseGLObjects is
> > virtual, but osg::ShaderComposer::releaseGLObjects is not.  Is there
> a reason why?
> >
> > Sounds like a dodgy compiler warning.
>
> I think not, it looks like an actual bug:
>
> osg::Object
> virtual void releaseGLObjects(osg::State* = 0) const {}
>
> osg::ShaderComposer
> void releaseGLObjects(osg::State* state);
>
> The ShaderComposer method is non-const and therefore does not override the
> osg::Object method!
>
> Can we use the 'override' keyword yet?
>
> Cheers,
> /ulrich
> ___
> osg-users mailing list
> osg-users@lists.openscenegraph.org
> http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
>
___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Ulrich Hertlein]

Hi guys,

On 18/04/2016 20:22, Robert Osfield wrote:
> On 18 April 2016 at 17:39, Andy Skinner  > wrote:
>  
> We are getting a warning for ShaderComposer::releaseGLObjects: 'void
> osg::ShaderComposer::releaseGLObjects(osg::State *)' : member function 
> does not
> override any base class virtual member function   
> osg::Object::releaseGLObjects is
> virtual, but osg::ShaderComposer::releaseGLObjects is not.  Is there a 
> reason why?
> 
> Sounds like a dodgy compiler warning.

I think not, it looks like an actual bug:

osg::Object
virtual void releaseGLObjects(osg::State* = 0) const {}

osg::ShaderComposer
void releaseGLObjects(osg::State* state);

The ShaderComposer method is non-const and therefore does not override the 
osg::Object method!

Can we use the 'override' keyword yet?

Cheers,
/ulrich
___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Jannik Heller]

Hi Robert

Actually there is a bug here. The ShaderComposer::releaseGLObjects does not 
override because it is declared without the const specifier. 
Object::releaseGLObjects is declared const.


> osg::Object::releaseGLObjects is virtual, but 
> osg::ShaderComposer::releaseGLObjects is not. Is there a reason why? 

The virtual keyword doesn't really have a meaning in overridden functions. 
Still wouldn't hurt to add it though.

Cheers
Jannik

--
Read this topic online here:
http://forum.openscenegraph.org/viewtopic.php?p=66874#66874





___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

Re: [osg-users] ShaderComposer::releaseGLObjects warning

[Robert Osfield]

On 18 April 2016 at 17:39, Andy Skinner  wrote:


> We are getting a warning for ShaderComposer::releaseGLObjects: 'void
> osg::ShaderComposer::releaseGLObjects(osg::State *)' : member function does
> not override any base class virtual member function
> osg::Object::releaseGLObjects is virtual, but
> osg::ShaderComposer::releaseGLObjects is not.  Is there a reason why?
>

Sounds like a dodgy compiler warning.
___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

[osg-users] ShaderComposer::releaseGLObjects warning

[Andy Skinner]

We are getting a warning for ShaderComposer::releaseGLObjects:
'void osg::ShaderComposer::releaseGLObjects(osg::State *)' : member function 
does not override any base class virtual member function

osg::Object::releaseGLObjects is virtual, but 
osg::ShaderComposer::releaseGLObjects is not.  Is there a reason why?

thanks
andy

___
osg-users mailing list
osg-users@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org