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

Better dispose() support for painter and data objects (like Textures, buffers) #27

Open
ulmangt opened this issue Jul 2, 2013 · 10 comments
Assignees

Comments

@ulmangt
Copy link
Member

ulmangt commented Jul 2, 2013

Glimpse Painters, Textures, Buffers should be able to be painlessly disposed without an active GLContext.

The actual disposal should be managed behind the scenes the next time that the context in question is active.

@ghost ghost assigned ulmangt Jul 2, 2013
ulmangt added a commit that referenced this issue Sep 24, 2013
@ghost
Copy link

ghost commented Apr 24, 2014

You can call GLAutoDrawable.invoke() to put your runnable into the queue. Then, you don't need a current OpenGL context at the time of the call. Keep in mind that there are several aspects to handle when "releasing" a buffer, there are the OpenGL identifier(s) and the storage on the CPU (especially the direct NIO buffers, not managed by the garbage collector).

@ulmangt
Copy link
Member Author

ulmangt commented Apr 25, 2014

GLAutoDrawable.invoke() is definitely good to know about, and probably part of the solution.

GlimpsePainter is setup similar to WorldWind's Layer interface. It's passed an object encapsulating a GLContext when it needs to be painted. So it doesn't have a reference to a GLAutoDrawable or GLContext outside of the paintTo() callback.

I was hoping that WorldWind's setup would be of some guidance, but many of their Layers don't seem to actually do anything when their dispose() method is called (plus their dipose() doesn't provide a GLAutoDrawable or GLContext, so it doesn't seem like it could immediately cleanup OpenGL resources anyway). I need to look closer in case there's insight there that I'm missing, since their Layer setup appears similar to ours.

Also, because GlimpsePainters (think WorldWind Layers) can be attached to multiple GlimpseCanvases (each wrapping a GLAutoDrawable with shared GLContexts), we don't really want to have a GlimpseCanvas dispose of all of its attached GlimpsePainters when it is disposed (since others might still be using the Painter). I believe either some more sophisticated bookeeping on our end is necessary, or the burden of properly disposing individual painters needs to be left to the end user of the API...

Anyway, thanks for pointing out GLAutoDrawable.invoke(). It got me to think through this issue again, which I had tabled a while back to concentrate on other issues -- but definitely deserved to be revisited.

@ghost
Copy link

ghost commented Apr 25, 2014

Why do you think that it should be possible to call dispose() when no OpenGL context is current?

@ulmangt
Copy link
Member Author

ulmangt commented Apr 28, 2014

Sorry, I think I just wasn't being clear with my terminology.

We have high level containers which wrap a GLAutoDrawable and need functionality similar to GLAutoDrawable.destory( ) -- which does not need to be called with a current context, correct? (Although of course behind the scenes it will need to make a context current in order to call dispose( ) for all registered GLEventListeners).

A low level dispose( ) which is repsonsible for actually freeing OpenGL memory via glDeleteBuffers( ) certainly needs an active GLContext.

@ghost
Copy link

ghost commented Apr 29, 2014

Yes glDeleteBuffers obviously requires a current GLContext. It seems tricky to get a GLAutoDrawable within a painter. Please use the updated JogAmp documentation instead of the obsolete Oracle documentation when talking about JOGL 2.

@ulmangt
Copy link
Member Author

ulmangt commented Apr 29, 2014

Agreed. In our case, GlimpsePainter (and GlimpseLayout) are intended to be shared by multiple GlimpseCanvas/GLAutoDrawable. So you only have access to the GLDrawable when it's passed in during a call to GlimpsePainter.paintTo( GlimpseContext ).

Of course, this sharing of painters means that the current GlimpseCanvas behavior of disposing all its attached painters when its GLAutoDrawable receives dispose(..) is incorrect because those resources could be attached to multiple GlimpseCanvas.

It's actually also the cause of this problem from the forums: https://groups.google.com/forum/?fromgroups=#!topic/metsci-glimpse/Dca1GQc7q8c. Moving the canvas around inside a docking framework can cause dispose(..) to be called, which breaks because of the above behavior.

Simple fix is to not dispose painter resources automatically when a canvas is disposed and provide a means for users to manually call queue disposal of GlimpsePainter resources when appropriate. I was trying to do too much automatically...

@ghost
Copy link

ghost commented Apr 30, 2014

Maybe you can use weak references to handle that; when a painter is no longer used (no strong reference), you dispose its resources but then you depend on the garbage collection. The manual way is less dangerous.

Did you mean destroy() instead of dispose()? There is no dispose() method in the very latest version of JOGL.

@ulmangt
Copy link
Member Author

ulmangt commented Jun 5, 2014

Version 2.1.3, released today, implements the fix from my previous comment (calling GlimpseCanvas.destroy() no longer automatically disposes of GL resources of attached GlimpsePainters).

This fixes the bugs that is automatic disposal caused, but still puts the burden on the user to handle proper disposal of painter resources. Weak references are an interesting option, although I'm currently leaning towards just providing a nicer dispose() method in the GlimpsePainter interface (it currently requires a reference to a GlimpseContext, which means its really only conveniently callable from inside GlimpsePainter.paintTo().

@ghost
Copy link

ghost commented Jun 6, 2014

I have to look more carefully at your changes. I agree with your suggestion, I prefer a dispose() method, AWT already does something similar. How should this method be used? Disposing the GL resources requires a current OpenGL context.

@ulmangt
Copy link
Member Author

ulmangt commented Jun 6, 2014

Right now using GlimpsePainter.dispose( GlimpseCanvas ) is definitely not convenient. As you say, since it needs to be called with a current OpenGL context you need to write something like:

GlimpseCanvas.getGLDrawable( ).invoke( false, new GLRunnable( )
{
  public boolean run( GLAutoDrawable drawable )
  {
    painter.dispose( new GlimpseContextImpl( drawable ) );
    return false;
  }
});

Not ideal for a library which is trying to abstract away OpenGL details (at least in the common case).

Fortunately, GlimpseCanvas (our wrapper around the native GL drawing surface / GLAutoDrawable) provides a convenient place to hide this detail:

GlimpseCanvas.disposePainter( GlimpsePainter )

This is most likely how I'll handle it in the next release. Doesn't break any existing code and fits cleanly into how GlimpseCanvas is intended to be used (as a container which handles lifecycle for painters).

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

No branches or pull requests

1 participant