[osg-users] minor change: move assumeSizedInternallFormat from Texture to Image

[Julien Valentin]

testing enum as bool is sure bad practice but the first error was to set 0 as default for _sourceFormat...


robertosfield wrote:
> Hi julien,
> 
> On Thu, 16 Aug 2018 at 17:45, Julien Valentin
> <> wrote:
> 
> > Ho I haven't realized the root of existence of this function was the previous contribution to the implementation of glTexStorage.
> > So Now I can see where the contributor (sure it's Pawel Ksiezopolski) wanted to do.
> > 
> > I don't know why there's so many comments and misses in sizedInternalFormats but it's same to add them at the end
> > 
> > So my proposal is a 2 way process:
> > -add missing  sizedInternalFormats (ex:I found internalformat GL_RGBA16 never mentionned)
> > -Fix the image less glTexStorage use case with
> > 
> > 
> > Code:
> > if( useTexStorrage)
> > {
> > //ensure _internalFormat is sized
> > GLenum sized_internalFormat = _internalFormat;
> > if(!isSizedInternalFormat( sized_internalFormat))
> > sized_internalFormat =  assumeSizedInternalFormat( sized_internalFormat, _sourceType ? _sourceType : GL_UNSIGNED_BYTE);
> > if(!sized_internalFormat)
> > OSG_FATAL<<"Texture2D : can't generate TextureStorage setup fails: "<<std::endl;
> > extensions->glTexStorage2D( GL_TEXTURE_2D, (_numMipmapLevels >0)?_numMipmapLevels:1, sized_internalFormat, _textureWidth, _textureHeight);
> > };
> > 
> 
> I did a quick look through Texture.cpp, and there is this entry:
> 
> if(useTexStorrage)
> {
> if(extensions->isTexStorage2DSupported() && _borderWidth == 0)
> {
> //calculate sized internal format
> if(!compressed_image)
> {
> if(isSizedInternalFormat(_internalFormat))
> {
> sizedInternalFormat = _internalFormat;
> }
> else
> {
> sizedInternalFormat =
> assumeSizedInternalFormat((GLenum)image->getInternalTextureFormat(),
> (GLenum)image->getDataType());
> }
> }
> else
> {
> 
> if(isCompressedInternalFormatSupportedByTexStorrage(_internalFormat))
> {
> sizedInternalFormat = _internalFormat;
> }
> }
> }
> 
> useTexStorrage &= sizedInternalFormat != 0;
> }
> 
> if(useTexStorrage)
> {
> 
> It's handling the case of having an osg::Image but does illustrate
> some of previous PR's introduced (I'm not the author of this
> particular set of code so I'm learning by reading...)   What it does
> have is attempt to select an appropriate sizeInternalFormat, but if
> this fails then switch off use of glTexSorage.
> 
> It would be good to avoid having different code paths having different
> combinations of ways of setting up the sizeInternalFormat and
> different ways of falling back when it's not supported,  My
> inclination would be to have a selectSizeInernalFormat(Image*) method
> that can take an optional image pointer, then have this have it's own
> with Image and witout image internal logical to do the mapping, and
> then have the code be something like:
> 
> 
> GLenum useTexStorageWithWithSizedInternalFormat =
> extensions->isTextureStorageEnabled ?
> selectSizedInternalFormat(optional_image) : 0;
> if (useTexStorageWithWithSizedInternalFormat)
> {
> // glTexStorage code path
> }
> else
> {
> // glTexImage code path
> }
> 
> I'm not 100% sure conflating the bool and internal format in this way
> is perfect coding practice but suggest it as just having lots of code
> and different variables in play just complicates the code.  The code
> we are talking about is already overloaded with different code paths
> so it's really important to avoid this code becoming even more
> spaghetti like, opportunities to untangle the existing spaghetti is
> worth a few compromises like reusing an enum as the bool and the size.
> 
> Robert.
> _______________________________________________
> osg-users mailing list
> 
> http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
> 
>  ------------------
> Post generated by Mail2Forum


------------------------
Twirling twirling twirling toward freedom

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