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

[Julien Valentin]

Forgot to mention:
The code posing problem with ImageLess Texture is not in Texture.cpp but it's in derivated TextureXXX.cpp


mp3butcher wrote:
> ..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=74554#74554