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

[Robert Osfield]

Hi julien,

On Thu, 16 Aug 2018 at 17:45, Julien Valentin
<julienvalentin51 at gmail.com> 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.