Discussion:
[Opensg-users] OpenSG2.0 - Issues and Improvements
Michael Raab
2014-08-06 08:22:49 UTC
Permalink
Marcel Weiler
2014-08-06 11:53:42 UTC
Permalink
Hi all,

I found a bug in the function
ShaderProgramChunk::addShader(ShaderProgram * const value).
In line 177 of OSGShaderProgramChunk.cpp tessellation evaluation shaders
are added to the list of tessellation control shaders:

line 176 case GL_TESS_EVALUATION_SHADER:
line 177 this->addTessControlShader(value);
line 178 break;

This line should probably be

this->addTessEvaluationShader(value);


Also it would be nice if there were functions for reading tessellation
shader programs directly in a SimpleSHLChunk. Currently there are only
the functions 'readFragmentProgram', 'readGeometryProgram' and
'readVertexProgram'.
I would appreciate it if 'readTessControlProgram' and
readTessEvaluationProgram' were added.

Kind regards,
Marcel

---
Diese E-Mail ist frei von Viren und Malware, denn der avast! Antivirus Schutz ist aktiv.
http://www.avast.com
Carsten Neumann
2014-08-07 09:27:27 UTC
Permalink
Hello Marcel,

On 08/06/2014 01:53 PM, Marcel Weiler wrote:
> This line should probably be
>
> this->addTessEvaluationShader(value);

yup, absolutely, good catch, fixed.

> Also it would be nice if there were functions for reading tessellation
> shader programs directly in a SimpleSHLChunk. Currently there are only
> the functions 'readFragmentProgram', 'readGeometryProgram' and
> 'readVertexProgram'.
> I would appreciate it if 'readTessControlProgram' and
> readTessEvaluationProgram' were added.

done. Thanks for the bug report!

Cheers,
Carsten
Michael Raab
2014-08-06 13:08:25 UTC
Permalink
Carsten Neumann
2014-08-07 11:44:19 UTC
Permalink
Hello Michael,

On 08/06/2014 03:08 PM, Michael Raab wrote:
> quick update on that. Finally I found a laptop that has a Intel HD
> Graphics board. Indeed extension GL_ARB_imaging is not supported while
> GLversion is 4.0.0.
> I changed the hasExtOrVersion() calls back to hasExtension() and the
> crash moved ;-)

hmm, the problem with making that change is that IIUC this will break on
GL ES, since it has imaging extension and glBlendEquation is standard there.

Gerrit: any thoughts on what the best way to handle this is? Something like:

if(hasExtension(ARB_imaging)

else if(hasExtOrversion(ARB_imaging, 0x0104, 0x0200)

?

> Extensions GL_EXT_blend_minmax and GL_EXT_blend_subtract are
> supported but retrieving the function pointer for glBlendEquationExt is
> 0x00000.

hurray for driver bugs ;(

Cheers,
Carsten
Carsten Neumann
2014-08-07 09:25:59 UTC
Permalink
Hello Michael,

On 08/06/2014 10:22 AM, Michael Raab wrote:
> we have some problems, actually crashes, on client laptop that use Intel
> HD chips. The problems are linked to the usage of BlendChunk's and I
> think we have narrowed it down to the BlendEquation.
> It seems Intel HD chips and their drivers do not support GL_ARB_imaging
> extension, which is necessary for glBlendEquation. This problem arised
> with the switch from 1.8 to 2.0. I checked the BlendChunk implementation
> for differences and I've got a presumption what may be the problem. In
> 1.8 the BlendChunk uses ::hasExtension() to check for GL_ARB_imaging.
> 2.0 uses hasExtOrVersion(). So I guess that the GL version is large
> enough to get a true here even if the extension is not supported. What
> is the reason why this check was changed?

my guess would be because it is in the standard since GL version 1.4 or
thereabouts - not that that would be the first time Intel drivers
claiming some version/feature as supported and then happily ignore it ;)
Let me take a look at your follow up patch and see if we can get this to
work for both conforming and broken drivers.

> Furthermore I've improved some other things:
> 1.) TextureBuffer::processPreDeactivate(): Check if image is assigned to
> TextureObj, before accessing it..

fixed,

> 2.) OSGGeoSplitVertexArrayPumpGroup/OSGGeoVertexArrayPumpGroup: We had
> some Geometries that had at certain times no vertices. Calling
> glDrawArray with vertexCount 0 caused crashes on some graphics cards. I
> added a check here.

fixed,

> 3.) OSGImage: Added simple hash calculation to be able to compare images
> faster.

+ this->_hashCode = seed;
+ for(UInt32 i=0; i<_mfPixel.size(); ++i)
+ {
+ this->_hashCode =
+ this->_hashCode * prime + (Int32)(&(_mfPixel[i]));
+ }
+
+ this->_hashCode = seed;

err, it looks to me like there's some typos in this [1] and I'm afraid
as is this does not give me a warm fuzzy feeling...
Is the caching of the hash value important for you (given that it gets
invalidated by a change to _any_ field of the image)? Otherwise I would
slightly prefer to not make this a member function (Image already has a
hugely fat interface), but a free utility function. Also, using
OSG::SizeT for the hash code would make this play more nicely with C++11
std::hash, which uses std::size_t.

> 4.) OSGBlendChunk: In some cases glBlendEquation was used where
> glBlendEquationEXT should have be used.

-> other mail.

Thanks for the fixes!

Cheers,
Carsten


[1] hash code gets overwritten to seed,
you seem to be using the address of pixel (truncated to 32 bit) not the
content?
Michael Raab
2014-08-07 09:52:08 UTC
Permalink
<html><head></head><body><div style="font-family: Verdana;font-size: 12.0px;"><div>
<div>Hello Carsten,</div>

<div>&nbsp;</div>

<div>
<div>&gt;&gt; 3.) OSGImage: Added simple hash calculation to be able to compare images<br/>
&gt;&gt; faster.</div>

<div>&gt;+ this-&gt;_hashCode = seed;<br/>
&gt;+ for(UInt32 i=0; i&lt;_mfPixel.size(); ++i)<br/>
&gt;+ {<br/>
&gt;+ this-&gt;_hashCode =<br/>
&gt;+ this-&gt;_hashCode * prime + (Int32)(&amp;(_mfPixel[i]));<br/>
&gt;+ }<br/>
&gt;+<br/>
&gt;+ this-&gt;_hashCode = seed;</div>

<div>&gt;err, it looks to me like there&#39;s some typos in this [1] and I&#39;m afraid<br/>
&gt;as is this does not give me a warm fuzzy feeling...<br/>
&gt;Is the caching of the hash value important for you (given that it gets<br/>
&gt;invalidated by a change to _any_ field of the image)? Otherwise I would<br/>
&gt;slightly prefer to not make this a member function (Image already has a<br/>
&gt;hugely fat interface), but a free utility function. Also, using<br/>
&gt;OSG::SizeT for the hash code would make this play more nicely with C++11<br/>
&gt;std::hash, which uses std::size_t.</div>
</div>

<div>
<div>&nbsp;</div>

<div>Probably I was sleeping while implementing that..</div>

<div>Hope the following is ok..</div>

<div>&nbsp;</div>

<div>
<div>inline<br/>
OSG::SizeT Image::getHash()<br/>
{<br/>
&nbsp;&nbsp; &nbsp;if(!this-&gt;_hashCodeValid)<br/>
&nbsp;&nbsp; &nbsp;{<br/>
&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;// recalculate hash..<br/>
&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;OSG::SizeT seed = 173;<br/>
&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;OSG::SizeT prime = 37;</div>

<div>&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;this-&gt;_hashCode = seed;<br/>
&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;for(UInt32 i=0; i&lt;_mfPixel.size(); ++i)<br/>
&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;{<br/>
&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;this-&gt;_hashCode = this-&gt;_hashCode * prime + _mfPixel[i];<br/>
&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;}<br/>
&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;this-&gt;_hashCodeValid = true;<br/>
&nbsp;&nbsp; &nbsp;}</div>

<div>&nbsp;&nbsp; &nbsp;return this-&gt;_hashCode;<br/>
}</div>

<div>&nbsp;</div>

<div>Caching the hash value along with the image would be a useful feature as at certain times we do material optimizations which include the necessity to compare images quickly.</div>

<div>Computing the hash values over and over would cause a slowdown...</div>

<div>&nbsp;</div>

<div>Thanks,</div>

<div>Michael</div>
</div>

<div>&nbsp;</div>

<div name="quote" style="margin:10px 5px 5px 10px; padding: 10px 0 10px 10px; border-left:2px solid #C3D9E5; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">
<div style="margin:0 0 10px 0;"><b>Gesendet:</b>&nbsp;Donnerstag, 07. August 2014 um 11:25 Uhr<br/>
<b>Von:</b>&nbsp;&quot;Carsten Neumann&quot; &lt;***@gmail.com&gt;<br/>
<b>An:</b>&nbsp;opensg-***@lists.sourceforge.net<br/>
<b>Betreff:</b>&nbsp;Re: [Opensg-users] OpenSG2.0 - Issues and Improvements</div>

<div name="quoted-content">Hello Michael,<br/>
<br/>
On 08/06/2014 10:22 AM, Michael Raab wrote:<br/>
&gt; we have some problems, actually crashes, on client laptop that use Intel<br/>
&gt; HD chips. The problems are linked to the usage of BlendChunk&#39;s and I<br/>
&gt; think we have narrowed it down to the BlendEquation.<br/>
&gt; It seems Intel HD chips and their drivers do not support GL_ARB_imaging<br/>
&gt; extension, which is necessary for glBlendEquation. This problem arised<br/>
&gt; with the switch from 1.8 to 2.0. I checked the BlendChunk implementation<br/>
&gt; for differences and I&#39;ve got a presumption what may be the problem. In<br/>
&gt; 1.8 the BlendChunk uses ::hasExtension() to check for GL_ARB_imaging.<br/>
&gt; 2.0 uses hasExtOrVersion(). So I guess that the GL version is large<br/>
&gt; enough to get a true here even if the extension is not supported. What<br/>
&gt; is the reason why this check was changed?<br/>
<br/>
my guess would be because it is in the standard since GL version 1.4 or<br/>
thereabouts - not that that would be the first time Intel drivers<br/>
claiming some version/feature as supported and then happily ignore it ;)<br/>
Let me take a look at your follow up patch and see if we can get this to<br/>
work for both conforming and broken drivers.<br/>
<br/>
&gt; Furthermore I&#39;ve improved some other things:<br/>
&gt; 1.) TextureBuffer::processPreDeactivate(): Check if image is assigned to<br/>
&gt; TextureObj, before accessing it..<br/>
<br/>
fixed,<br/>
<br/>
&gt; 2.) OSGGeoSplitVertexArrayPumpGroup/OSGGeoVertexArrayPumpGroup: We had<br/>
&gt; some Geometries that had at certain times no vertices. Calling<br/>
&gt; glDrawArray with vertexCount 0 caused crashes on some graphics cards. I<br/>
&gt; added a check here.<br/>
<br/>
fixed,<br/>
<br/>
&gt; 3.) OSGImage: Added simple hash calculation to be able to compare images<br/>
&gt; faster.<br/>
<br/>
+ this-&gt;_hashCode = seed;<br/>
+ for(UInt32 i=0; i&lt;_mfPixel.size(); ++i)<br/>
+ {<br/>
+ this-&gt;_hashCode =<br/>
+ this-&gt;_hashCode * prime + (Int32)(&amp;(_mfPixel[i]));<br/>
+ }<br/>
+<br/>
+ this-&gt;_hashCode = seed;<br/>
<br/>
err, it looks to me like there&#39;s some typos in this [1] and I&#39;m afraid<br/>
as is this does not give me a warm fuzzy feeling...<br/>
Is the caching of the hash value important for you (given that it gets<br/>
invalidated by a change to _any_ field of the image)? Otherwise I would<br/>
slightly prefer to not make this a member function (Image already has a<br/>
hugely fat interface), but a free utility function. Also, using<br/>
OSG::SizeT for the hash code would make this play more nicely with C++11<br/>
std::hash, which uses std::size_t.<br/>
<br/>
&gt; 4.) OSGBlendChunk: In some cases glBlendEquation was used where<br/>
&gt; glBlendEquationEXT should have be used.<br/>
<br/>
-&gt; other mail.<br/>
<br/>
Thanks for the fixes!<br/>
<br/>
Cheers,<br/>
Carsten<br/>
<br/>
<br/>
[1] hash code gets overwritten to seed,<br/>
you seem to be using the address of pixel (truncated to 32 bit) not the<br/>
content?<br/>
<br/>
------------------------------------------------------------------------------<br/>
Infragistics Professional<br/>
Build stunning WinForms apps today!<br/>
Reboot your WinForms applications with our WinForms controls.<br/>
Build a bridge from your legacy apps to the future.<br/>
<a href="http://pubads.g.doubleclick.net/gampad/clk?id=153845071&amp;iu=/4140/ostg.clktrk" target="_blank">http://pubads.g.doubleclick.net/gampad/clk?id=153845071&amp;iu=/4140/ostg.clktrk</a><br/>
_______________________________________________<br/>
Opensg-users mailing list<br/>
Opensg-***@lists.sourceforge.net<br/>
<a href="https://lists.sourceforge.net/lists/listinfo/opensg-users" target="_blank">https://lists.sourceforge.net/lists/listinfo/opensg-users</a></div>
</div>
</div>
</div></div></body></html>
Carsten Neumann
2014-08-07 11:44:55 UTC
Permalink
Hello Michael,

On 08/07/2014 11:52 AM, Michael Raab wrote:
> Caching the hash value along with the image would be a useful feature as
> at certain times we do material optimizations which include the
> necessity to compare images quickly.
> Computing the hash values over and over would cause a slowdown...

ok, thanks, I've added it.

Cheers,
Carsten
Loading...