Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

minimum changes to fix BindImageTexture #467

Closed
wants to merge 4 commits into from

Conversation

mp3butcher
Copy link
Contributor

@mp3butcher mp3butcher commented Feb 7, 2018

re-add Texture changes of
#466
and fix issue
http://forum.openscenegraph.org/viewtopic.php?t=17102
by making _modifiedcount Texture attribute (see commit log)

…f texture objects)

and remove it from its sons (except in layered type where a PerLayerPCcounter is used to avoid uploading unchanged layers)
-readd the virtual Texture::getBufferData removed by the rollback differencing Texture from TextureBuffer
… to be applied

and fix case when BD of a Texture is dirty
@mp3butcher mp3butcher changed the title Imagebinding2 minum changes proposal for BindImageTexture Feb 7, 2018
@mp3butcher mp3butcher changed the title minum changes proposal for BindImageTexture minimum changes proposal for BindImageTexture Feb 7, 2018
@mp3butcher mp3butcher changed the title minimum changes proposal for BindImageTexture minimum changes to fix BindImageTexture Feb 7, 2018
@openscenegraph
Copy link
Owner

Making _modifiedCount part of the base class is broken because you end up having to duplicate the data structure for the array version of texture.

Also I can grasp how 12 changed files could be at all a minimal change set. You've merged three set of changes into one commit.

Adding the texture unit as into BindImageTexture just feels like this class is getting more a more tightly coupled state that breaks the orthogonal design that the OSG and OpenGL try to maintain, the fact you are having to do this tight coupling suggests that approach is far from idea and may need revising at a fundamental level.

I'm closing this PR as it just contains too many different problems.

@mp3butcher
Copy link
Contributor Author

mp3butcher commented Feb 7, 2018

Why do you close a pr without discussing it?
Try to respect my insights as i respect yours

As i mentionned in the commit the semantic of _modifiedCount is not clear:

  • in unlayered Textures it refers to texture object dirtiness
  • in layered Textures it's used as a hint not to upload unchanged image layers (there no per context per layer gl object..)

Make _modifiedcount a Texture attrib make clear to everyone that _modifiedCount represents "texture object modified count" and not a context-unrelated-inner-tricks-not-to-upload-image-already-there
Given this semantic there's no data duplication....

Concerning how I designed BindImageTexture, we already have that discussion it's not a textureatrribute imageunit is not textureunit!!
But in the case texture target is dirty we need a tu otherwise we hack the current active textureunit and it can lead to problems..
the solution of a "virtual-texture-attribute" seams the wisest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants