Repair VAS design in 3.6 [please review]

[mp3butcher]

fix propal for #674

Some of the original discussion is on #587
The original discussion on the leak was covered in the thread https://www.mail-archive.com/osg-users@lists.openscenegraph.org/msg77211.html "VAO Resource Leak in OSG 3.6.2" starting 2 Aug 2018.

commit engaged eee5d54
@robertosfield edited:
problems adressed in this pr are:

• bufferobject "overfree" bug (releaseGLObject at Drawable destruction cf issue delete of shared GLObjects when Drawable is released #674) (fix: revert eee5d54 to previous design using dirtyGLObject in ~geometry -vas leak at end of program prevented with following fix-)
• threading issue with VAO (vao released after context destruction cf issue delete of shared GLObjects when Drawable is released #674) (fix:record each created vas in VASmanager and delete them on context destruction..vas can be released a second time after context destruction so i added a guard in VASmanager::flushAll)
• vas life cycle (no way to override vas destruction)(fix add a callback and mod callback interface)
• and several dirtyGLObject releasGLObject misses in Drawable based (crash at prog end with vao path using geometrypool and particlesystem)

[openscenegraph]

I have done a first pass review and it doesn't jump out at my as an actual fix, more of shuffling cards to hide an issue. The changes affect the ABI as well.

If you have a crash and it's related, could you create an full example that illustrates the problem. When I look into the sharing issue I might be able spot the problem with the crash you are seeing.

[mp3butcher]

I modified the issue #674 sample code with the setting of the ThreadingModel.Does it require a new issue?

[mp3butcher]

@robertosfield
I changed my propal

[robertosfield]

On my return from holiday's I'll do a proper review on the PR and the underlying issue. As a general note, a PR that changes 14 files to address an issue raises a red flag - either the underlying class design was originally broken or that the PR mis-understands the original design, or somewhere in between - both are broken :-)

Once I've spent a bit of time reproducing the bug and thinking about the failure mechanism I should be able to spot a way forward. For changes to the 3.6 branch I'll be aiming to minimze changes to the public interface and ideally not change the ABI between 3.6.3 and the the 3.6 branch.

[mp3butcher]

All the VAS design was broken and a lot of stuff was missing
I have not even test if my programs works with my pr, but it definitly can't wihtout
And why do you want to keep abi of a bugged release candidate?
i just add funcs to the API and clean the VASdesign not a disaster....

[robertosfield]

On Thu, 27 Dec 2018 at 12:42, Julien Valentin ***@***.***> wrote: all was broken that's why i cahnged wo much file

What specifically do you think was faulty? Robert.

…

[mp3butcher]

sharing same bos(vbo and ebo) among several geometries with vas_impl=vao gived bad results (wrong vas setup)

I just confirmed this bug is not one.... my fault(error in my test code)...sorry...not so broken after all but lot of missing releaseGLobject and dirtyGLobject

[robertosfield]

Hi @mp3butcher, I can't work out exactly what you mean by the above statement. Range of possibilities are there isn;t an OSG issue at all, to there being a few missing implementations somewhere.

[mp3butcher]

@robertosfield no issue mea culpa forget my post.
The "only" problems adressed in this pr are:

• bufferobject "overfree" bug (cf issue) (fix: revert to previous design using dirtyGLObject in ~geometry -vas leak prevented with following fix-)
• threading issue with VAS (cf issue) (fix:record each created vas in VASmanager)
• vas life cycle (no way to override vas destruction)(fix add a callback and mod callback interface)
• and several dirtyGLObject releasGLObject misses in Drawable based
  PR presentation updated to explain more in details

[openscenegraph]

Copied below text from Issue message closing this Issue, here for completeness:

I have resolved the issue by refactoring the Geometry destructor so that is just resets the local array and primitives ref_ptr<>'s + arrays and relies upon the Array/PrimitiveSet/BufferData destructors for doing release of GL objects from within their own destructors. This means that the shared arrays/primitives don't get released prematurely.

337f240

[mp3butcher]

Do you read my post sometime?....There are other issues...
I'll be back to harcell... sorry

[robertosfield]

On Mon, 31 Dec 2018 at 12:07, Julien Valentin ***@***.***> wrote: Do you read my post sometime....There are other issues...

Which post, posted where?

[mp3butcher]

in this pr description

[openscenegraph]

If there are specific issues that need to be addressed the right way to resolve them is to tackle them individually and in each case outline how the issue appears. Throwing everything together in a single PR makes it impossible to work out what change has been done for what reason. The "issues" may also be just usage problems rather problems with the underlying implementation. again throwing everything together make it impractical to track what is really required as a code change and what needs to be done at the application level.