[osg-users] BindImageTexture Crash

[D. Christopher Fennell]

Hello,

The latest submission for BindImageTexture will crash. It's expecting a buffer object, which is new. The osgcomputeshaders example, which uses BindImageTexture, will also crash with this latest change.

Thank you,

DC

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





_______________________________________________
osg-users mailing list
osg-...@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

[Julien Valentin]

Yes, I'm not surprised
my last pr was merged a bit too soon and may introduce a bug...
I mentionned it wasn't clean but Robert merge it anyway
My fault: I take pr more as a channel of communication than real candidates to merge

dcfennell wrote:
> Hello,
>
> The latest submission for BindImageTexture will crash. It's expecting a buffer object, which is new. The osgcomputeshaders example, which uses BindImageTexture, will also crash with this latest change.
>
> Thank you,
>
> DC


------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=72901#72901

[D. Christopher Fennell]

OK. Thank you. I have a local workaround until a fix is in place.

DC

------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=72902#72902

[Julien Valentin]

This bug is related to how unrefaffterapply is implemented
@Robert
Do you think this feature could be modified to release only Image data and not the whole Image (make Image and Imagedataproxy)?
This mod could lead to
-generalization BufferObject to TextureObject
-serialization homogenization (release only data and keep filename to load it if required)

Hope i'm clear...

mp3butcher wrote:
> Yes, I'm not surprised
> my last pr was merged a bit too soon and may introduce a bug...
> I mentionned it wasn't clean but Robert merge it anyway
> My fault: I take pr more as a channel of communication than real candidates to merge
>
>
> dcfennell wrote:

> > Hello,
> >
> > The latest submission for BindImageTexture will crash. It's expecting a buffer object, which is new. The osgcomputeshaders example, which uses BindImageTexture, will also crash with this latest change.
> >
> > Thank you,
> >
> > DC
>


------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=72904#72904

[D. Christopher Fennell]

FYI, I did setUnRefImageDataAfterApply to false. The image data becomes valid, but the buffer object is NULL. One OSG example uses a textureBufferObject with BindTextureImage, which is OK. But if you use a texture, there is no buffer object.

Perhaps just a simple check for valid 'getBufferData' and then a check for 'getBufferObject'??

Or even better, if 'to' is NULL, shouldn't just the _target be applied?

Whether image data is NULL or not, the texture object should be valid after _target->apply if it's setup correctly and have a valid ID to bind (with glBindImageTexture).

------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=72905#72905

[Julien Valentin]

The bufferobject is null because of the design used for unRefImageDataAfterApply feature
Texture owns textureobjects in order to deref bufferdata

What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new classTextureObject)->GraphicObject intact

dcfennell wrote:
> FYI, I did setUnRefImageDataAfterApply to false. The image data becomes valid, but the buffer object is NULL. One OSG example uses a textureBufferObject with BindTextureImage, which is OK. But if you use a texture, there is no buffer object.
>
> Perhaps just a simple check for valid 'getBufferData' and then a check for 'getBufferObject'??
>
> Or even better, if 'to' is NULL, shouldn't just the _target be applied?
>
> Whether image data is NULL or not, the texture object should be valid after _target->apply if it's setup correctly and have a valid ID to bind (with glBindImageTexture).

------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=72906#72906

[Robert Osfield]

On 1 February 2018 at 20:46, Julien Valentin <julienva...@gmail.com> wrote:
> The bufferobject is null because of the design used for unRefImageDataAfterApply feature
> Texture owns textureobjects in order to deref bufferdata
>
> What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new classTextureObject)->GraphicObject intact

Umm.... lets change something that's been working fine for well over a
decade to fix an issue recently introduce by an ill-considered
submission.

I will need to look deeper into the issue and decide what is best to
do. I don't have the time to do this right away. Might have time
next week.

Robert.

[Robert Osfield]

I have reverted the PR, this resolved the osgcomputeshaders bug.

[Julien Valentin]

https://github.com/openscenegraph/OpenSceneGraph/pull/465

Here's what I'm thinking of....sorry for the huge commit
ChangeLog (in absence of commit history)

introduce a TextureGraphicObject ( buffered_object PCTOs -arg why can't I rename Texture::TextureObject to Texture::GLTextureObject, TextureGraphicObject is a so ugly name?!! - )
inject a base class for PerContextGraphicObject (base of BufferObject, TextureGraphicObject (bufferedPCTOs), ProgramsObject ..)[/list]
deprecate Image less Texture by adding an Image with NULL data (may require further data==0 tests to ensure retrocomp) and make unrefimageafterapply just deallocate Image
remove buffered_object from Texture to use the TextureGraphicObject of its Image[/list]

I'll try to clean the PR if community is everyone happy with it

robertosfield wrote:
> I have reverted the PR, this resolved the osgcomputeshaders bug.
>

> On 2 February 2018 at 09:17, Robert Osfield <> wrote:

>
> > On 1 February 2018 at 20:46, Julien Valentin <> wrote:
> >
> > > The bufferobject is null because of the design used for unRefImageDataAfterApply feature
> > > Texture owns textureobjects in order to deref bufferdata
> > >
> > > What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new classTextureObject)->GraphicObject intact
> > >
> >
> > Umm.... lets change something that's been working fine for well over a
> > decade to fix an issue recently introduce by an ill-considered
> > submission.
> >
> > I will need to look deeper into the issue and decide what is best to
> > do. I don't have the time to do this right away. Might have time
> > next week.
> >
> > Robert.
> >
> _______________________________________________
> osg-users mailing list
>

> http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
>
> ------------------
> Post generated by Mail2Forum
[list=][/list]

------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=72911#72911

[Julien Valentin]

Okay

I gone totally mad with this PR...(chainsaw night fever)
Mea Culpa

So to sum up the problem:
- BindImageTexture stateattribute is not a textureattribute (_imageunit != _textureunit) but it sometimes have to bind texture object on a textureunit in case data of the associated texture has gone dirty

We then need a way to know if texture have to be applied:

What I've propose :
- clarify semantic given to textures::_modifiedcount to be the textureobjectmodifiedcount. Son as tos belong to Texture i putted modifiedcount in Texture and removed it from daughter classes
-in LayeredTextures (cubemap, texture2Darray) i changed modifiedcount to layermodifiedcount these flags doesn't have the same purpose as the textureobjectmodifiedcount as it's just a inner mecanism not t upload unchanged layer image (not related to the to)

https://github.com/openscenegraph/OpenSceneGraph/pull/467
My pr has been rejected so i ask community reviews and insights because I can see a better compromise to finish BindImageTexture

Thanks in advance

mp3butcher wrote:
> https://github.com/openscenegraph/OpenSceneGraph/pull/465
>
> Here's what I'm thinking of....sorry for the huge commit
> ChangeLog (in absence of commit history)
>

> 1) introduce a TextureGraphicObject ( buffered_object PCTOs )
> 2) inject a base class PerContextGraphicObject (base of BufferObject, TextureGraphicObject (bufferedPCTOs), ProgramsObject ..)
> 3) deprecate Image less Texture by adding an Image with NULL data and make unrefimageafterapply just deallocate Image (may require further data==0 tests to ensure retrocomp)
> 4) remove buffered_object from Texture to use the TextureGraphicObject of its Image instead
>
> I'll try to clean the PR if community is happy with it

>
>
>
> robertosfield wrote:
> > I have reverted the PR, this resolved the osgcomputeshaders bug.
> >
> > On 2 February 2018 at 09:17, Robert Osfield <> wrote:
> >
> > > On 1 February 2018 at 20:46, Julien Valentin <> wrote:
> > >
> > > > The bufferobject is null because of the design used for unRefImageDataAfterApply feature
> > > > Texture owns textureobjects in order to deref bufferdata
> > > >
> > > > What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new classTextureObject)->GraphicObject intact
> > > >
> > >
> > > Umm.... lets change something that's been working fine for well over a
> > > decade to fix an issue recently introduce by an ill-considered
> > > submission.
> > >
> > > I will need to look deeper into the issue and decide what is best to
> > > do. I don't have the time to do this right away. Might have time
> > > next week.
> > >
> > > Robert.
> > >
> > _______________________________________________
> > osg-users mailing list
> >
> > http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
> >
> > ------------------
> > Post generated by Mail2Forum
> [list=][/list]

------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=72946#72946

[Robert Osfield]

Hi Juilien,

On 7 February 2018 at 18:59, Julien Valentin <julienva...@gmail.com> wrote:
> So to sum up the problem:
> - BindImageTexture stateattribute is not a textureattribute (_imageunit != _textureunit) but it sometimes have to bind texture object on a textureunit in case data of the associated texture has gone dirty

It is a form of texture attribute, it relates directly to texture, the
only thing that decouples BindImageTexture from being a traditional
OSG style texture attribute is that you don't always need to apply the
texture unit when applying it as the interface provides this. This
duality of concepts/implementation is down to OpenGL 4.x introducing
APIs that don't work like previous OpenGL APIs worked.

The only reason I can see for BindImageTexture attribute needing to
bind a particular texture unit being applying the texture would be to
avoid osg::State lazy state updating for not apply a texture for a
texture unit.

In normal OSG usage There are several things going on when you apply a
Texture::apply(), the create a texture object is not already created,
any data is uploaded to this texture object if required and the
texture object is made current to currently set up texture unit
(osg::State handles this part). Once a texture is created doing a
Texture::apply() only really does the binding of the texture object to
the texture unit.

For BindImageTexture my guess is that if you do need to respond to the
dirty Texture by calling apply() is to just apply it on the current
texture unit and then may sure osg::State's lazy state updating knows
about it. The osg::State::applyTextureAttribute(unsigned int unit,
const StateAttribute* attribute) method is the tool for the job. The
actual texture unit is probably irrelevant as the OSG should
automatically reset if required so you could using unit=0 just fine,
or the value of osg::State::getActiveTextureUnit() if you want to
avoid changing as much state as possible,

> We then need a way to know if texture have to be applied:
>
> What I've propose :
> - clarify semantic given to textures::_modifiedcount to be the textureobjectmodifiedcount. Son as tos belong to Texture i putted modifiedcount in Texture and removed it from daughter classes

That's a dreadful idea given it directly conflicts with the
array/cubmemap version of texture subclasses, that alone should have
raised a red flag with your implementation not being appropriate.

Perhaps a virtual bool Texture::isDirty(const unsigned int contextID)
would be the way to implement a generic mechanism. This is the
standard object orientation way of decoupling interface from
implementation.

> -in LayeredTextures (cubemap, texture2Darray) i changed modifiedcount to layermodifiedcount these flags doesn't have the same purpose as the textureobjectmodifiedcount as it's just a inner mecanism not t upload unchanged layer image (not related to the to)

modifiedCount implementation has the same function in all Texture
subclasses, of you tracking whether image data and associated texture
objects are in sync or not.