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 _______________________________________________ osg-users mailing list osg-users@lists.openscenegraph.org http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org