-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[osg3.6] prevent immutability setting when textureobject is taken from orphans #934
[osg3.6] prevent immutability setting when textureobject is taken from orphans #934
Conversation
04478b5
to
4e7a67d
Compare
I just got around to actually testing this, and it does indeed fix the issue in OpenMW, as anticipated. I imagine there'll be some tweaking before this is actually merged (setted isn't a word, for example), but something based on this should be good. |
I have started looking at the PR and the underlying code. Use of _immutableSetted variable is rather awkward in terms of language. We'll need a more appropriate name if this variable is required. Looking at Texture::TextureObject there is already an _allocated member variable which sounds like there is functionality overlap with _immutableSetted. The code path contain the glTextureStorage2D does set the Allocated parameters which is possibly a bit odd. The underlying code goes many years back before glTextureStoage2D code was introduced, it might be was these new features were introduced not everything has been evolved correctly w.r.t working with the old features as well as the new. I'm coming to the topic a bit cold so don't have any fully formed opinions yet. One of my first thoughts are to explore whether the TextureObject::_allocated/setAllocated() functionality is something we can use to decided whether the glTextureStorage2D should be called or not. If so then a check against if (texturObject-isAllocated()) would be appropriate along with correct calling of setAllocated(). Thoughts? |
I think texture allocation versus immutable allocation are different concerns, even if they're related, so even if they can be combined, it'll potentially be misleading. |
On Wed, 29 Apr 2020 at 19:00, Chris Djali ***@***.***> wrote:
I think texture allocation versus immutable allocation are different
concerns, even if they're related, so even if they *can* be combined,
it'll potentially be misleading. glTexImage allocates a texture, but not
immutably, so it can be reallocated again later.
The principle behind the OSG's cache of old texture objects is that they
can be reused without reallocation, so once they are created they shouldn't
change size. So the immutable aspect should be OK. However, I can't say
for sure without reviewing all the usage of TextureObject.
My hunch is that proper use of the allocated flag and sizing of
TextureObject members should be sufficient.
|
_allocated could serve the propose if we remove _allocated=true from TextureObject::setAllocated and set it true out of TextureObject scope (in TextureXXX::apply) |
4e7a67d
to
c04daeb
Compare
@mp3butcher sometimes you perplex me: "_allocated could serve the propose if we remove _allocated=true from TextureObject::setAllocated" I can't think why you'd ever want to change a feature behavior so it doesn't behave in the most obvious way and in the way it was designed for. setAllocated should set _allocated = true, because it's the most logically sound thing to do with this naming of methods. All I'm asking here is whether the problem code block in question, the on with the glTexStorage2D call is missing a check if (textureObject->isAllocated()) and missing a corresponding setAllocated(). A little above the problem section we have a code block:
Could it simply be that the glTexStorage2D code path is missing that this type of usage? FYI, I'm not the original author of the glTexStorage2D so it might be that the original contributor of it didn't realize about the Allocated support in TextureObject and I didn't spot the problem when merging. I don't have a test case to run and see the problem happening so I'm working a bit blind on this particular bug report. |
c04daeb
to
197fc40
Compare
tested and ok for merging |
@mp3butcher Changes look good :-) |
I now have merged the changes into master.
…On Sun, 3 May 2020 at 09:37, OpenSceneGraph git repository < ***@***.***> wrote:
Merged #934 <#934>
into OpenSceneGraph-3.6.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#934 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKEGUESMX6Q2Q3QBVOXOA3RPUUOJANCNFSM4LTZEC7A>
.
|
I think there's potentially a race condition where this can still happen. I've got an APITrace (unfortunately 16 GB long because it reproduces really rarely when APITrace is attached) where
I think what's happening is possibly an incremental compile operation getting split across two frames, but I'm not entirely sure how |
It's harder to reproduce with a build made from a more recent commit, though, so either some changes have hidden it, or it's been fixed. |
a quick fix for #935 a minor problem that affect master too