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

[Julien Valentin]

..But my question was
If I'd make a pr with this fix for all Textures

Code:
  if( useTexStorrage)
        {
            //ensure _internalFormat is sized
            GLenum sized_internalFormat = _internalFormat;
            if(!isSizedInternalFormat( sized_internalFormat))
                    sized_internalFormat =  assumeSizedInternalFormat( sized_internalFormat, _sourceType==0 ? _sourceType : GL_UNSIGNED_BYTE);
            if(!sized_internalFormat)
                OSG_WARN<<"Texture2D : can't generate TextureStorage for  internalFormat: "<<_internalFormat<<" and  SourceType: "<<_sourceType==0 ? _sourceType : GL_UNSIGNED_BYTE<<std::endl;
            extensions->glTexStorage2D( GL_TEXTURE_2D, (_numMipmapLevels >0)?_numMipmapLevels:1, sized_internalFormat, _textureWidth, _textureHeight);
        }; 



Would you accept it?


mp3butcher wrote:
> 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
> 

Code:




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

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