cginternals / globjects

C++ library strictly wrapping OpenGL objects.
https://globjects.org
MIT License
539 stars 59 forks source link

Questions about ref_ptr vs shared_ptr and RAII #308

Closed totalgee closed 7 years ago

totalgee commented 8 years ago

Hi there, this isn't really an "issue", more just curiosity as to why you chose to implement your own internal reference counting and ref_ptr template, vs std::shared_ptr? In the wiki you write:

Unlike std::shared_ptr a ref_ptr does not store the reference count inside of the pointer, but on the object itself. This has the advantage that it is not necessary to pass shared_ptr copies in all method signatures, but instead simple pointers can be passed and assigned to another ref_ptr later without breaking the reference count.

When you say: "necessary to pass shared_ptr copies in all method signatures"...do you mean within in the globjects API? Because there don't seem to be a lot of objects (Texture, Shader, VertexArray, etc) that take pointers to other globjects in the API... (I'm still new to your library, so maybe I've missed something.) Or maybe you mean having to pass shared_ptrs in the method signatures of an application that uses globjects? Because users of the library would be welcome to pass shared_ptrs (when appropriate) or just regular pointers (when it's known that no ownership transfer will be taking place) in their methods.

I was just wondering, you likely have other thoughts on this (or performance benchmarks) that I've not considered. Because it would be nicer not to add yet another reference counting scheme to the world (if it's not needed). Also, it's nice to be able to use something like std::unique_ptr and move semantics (managing/transferring exclusive ownership whilst incurring almost no cost) -- the object internal reference counting is not needed in such cases.

Also, do you have any thoughts of implementing RAII helpers for globjects that use "bind/unbind" or "use/release" methods? Though this is not necessary (and users can write their own), it might be nice to have RAII helpers for binding/unbinding objects, without having to do it manually. e.g.:

    // do it yourself
    glActiveTexture(GL_TEXTURE0 + m_samplerIndex);
    m_texture->bind();
    m_program->use();
    m_vao->drawArrays(GL_TRIANGLE_STRIP, 0, 4);
    m_program->release();
    m_texture->unbind();

    // use scopes and RAII to handle binding/use
    {
        glActiveTexture(GL_TEXTURE0 + m_samplerIndex);
        scoped_bind<Texture> tex(m_texture);
        scoped_bind<Program> prog(m_program);
        m_vao->drawArrays(GL_TRIANGLE_STRIP, 0, 4);
    }
scheibel commented 8 years ago

Thanks for sharing your opinion and ideas. The topic you want to discuss is a really relevant for globjects and each project depending on it. Thus, it is a main design decision which we made over two years ago and tried various times to reevaluate and change it.

Here is a list of issues and branches that covers this topic:

Somehow related: https://github.com/cginternals/globjects/issues/188

Regarding your second proposal: I know the pattern you suggest and I can think of a more general concept to apply this to globjects (including global OpenGL states and maybe other things). Four example seems to be too small to show such a feature is required. Further, one thing we thought about managing bound objects is a cache (https://github.com/cginternals/globjects/issues/235), although this breaks one basic assumption of globjects: arbitrary OpenGL calls may be made without using globjects. The only remaining assumption for this feature to work is that only glbinding may be used to issue arbitrary OpenGL calls without using globjects.

Best regards,

Willy

totalgee commented 8 years ago

What I'm not clear on, from reading all these threads (thanks!) and seeing the branches, is what is the current state of things? Have memory-management decisions been made? Those "remove_ref_ptr" and "explicit_memory_management" branches haven't been touched in nearly a year... does this mean the decision has been made to abandon them? Just wondering if the current ref_ptr and Referenced base class stuff, then, is "final"...? (just would be good to know, one way or another)

Regarding my RAII example, I know it was a contrived example, but it was just to illustrate an example usage. It can be useful to have these helpers around, people may choose to use them or not (like std::lock_guard). But it's obviously not as critical (pardon the pun) as releasing mutexes... Also, at times (and for performance reasons) you don't want to unbind a GL resource shortly after binding it, when you know that the same context will be required soon (e.g. drawing multiple objects with the same shader program and textures). So it's not necessary to unbind it immediately.

Thanks for listening, Glen.

andrew-kennedy commented 7 years ago

I have been heavily investigating using globjects myself as well, as a replacement for my own primitive openGL shader wrapper objects, but I too am confused by the memory management semantics of using this library. Ideally I'd like to use globjects with make_shared, is this possible as of now?

scheibel commented 7 years ago

I started with a std::shared_ptr variant for memory management that can be tested using the remove_ref_ptr branch. However, a straight-forward replacement of ref_ptr with shared_ptr yielded a strong limitation of the shared_ptr: we would rely on virtual method calls in our constructors and the shared reference counting is not fully initialized during this stage of object creation. We could use workarounds (and I started doing so in this branch) but I vision we would end up with a general wrapper for each globjects::Object that hides memory management completely. Another alternative may be that memory management has to be handled by the users of globjects but that would impede current use cases. We somewhat enjoy the current interface of globjects and it is surprisingly stable (seems like it fits for our uses cases pretty well).

I think we need more insights into integration and usage in projects of external developers to provide an improved object management.

scheibel commented 7 years ago

We decided to put the user of globjects in charge of managing shareable objects. Only objects with exclusive ownership are maintained by globjects. This behavior is integrated in the current master and breaks the compatibility to our latest release (1.1.0).

andrew-kennedy commented 7 years ago

What does this functionally mean for a user moving from 1.0 to 1.1.0? What code changes are required?

scheibel commented 7 years ago

The release 1.1.0 doesn't contain these changes and it was intentionally created using the commit before the memory management changes. So moving from 1.0.0 to 1.1.0 shouldn't include any work. You just have a small list of additional features (pixelStore, buffer data upload overloads) and fixes (memory leaks). However, the change to the current master is more drastic. I think a good starting point is to look at the changes we applied to the qt example.

To summarize the changes between the two memory management systems: the current globjects interface won't create memory internally if it may not have exclusive ownership. You have to handle pointer lifetime by yourself. This heavily affects shaders, programs and uniforms. Where you may have stored a program reference-pointer you now have to store a program pointer, pointers to each attached shader and most likely one or even more pointer per shader to its sources. Hopefully we'll have a migration guide when globjects 2.0 gets released.

andrew-kennedy commented 7 years ago

Wow, reading the changes you applied in your example it seems like using globjects suddenly requires a lot more work on the side of the user. What motivated this change as opposed to leaving the current interface the way it was? I know this whole thread was prompted by people wanting to avoid ref_ptrs but this change actually seems like it makes usage more difficult. I'm not super experienced though so feel free to let me know if I'm wrong. 😊

scheibel commented 7 years ago

Yeah I'm also not happy with loading so much work onto the user and we had really long discussions about this internally. However, this issue popped up regularly in the last two years and we thought if there were so many mixed feelings about reference counting, we should eliminate it. I think the main rational reasons are:

However, globjects main goal is to provide an object-oriented wrapper around OpenGL and to provide a fallback-enabling interface for modern graphics programming. If you want to write hacky tech-demos, plain OpenGL is just fine (and we use it regularly). If you want to write high-level rendering code, you may want to use a higher level of abstraction. We use globjects to teach the OpenGL API and for tools where we want a basis for cross-compatibility (using modern features where applicable and fallback to basic OpenGL 3.x where required).

A further note on our main use case: the rendering system gloperate. Here, globjects is intended to be the middle layer between low-level API calls and full rendering engine. For us, it makes writing the engine easier and as engine-writers, we are able to stem the additional amount of ownership handling which is then abstracted for the actual use cases.

andrew-kennedy commented 7 years ago

Can I ask why you use raw pointers instead of smart pointers? It seems weird that you have to call get() to get a raw pointer every time you want to pass pointers to globjects functions. Seems to kinda defeat the purpose of smart pointers if I suddenly "give up" my ownership semantics once I pass my pointer to a globjects function, but maybe I'm misunderstanding smart pointers.

scheibel commented 7 years ago

With the switch from ref_ptr to smart pointers of the standard library we chose the unique_ptr as designated replacement. Of course, we could have passed around const std::unique_ptr<T> & in all interfaces but the general message of such interface is:

You're getting a pointer where you may not derive ownership from. You must not control the lifetime of the object. You have to rely on the caller that the object lifetime exceed your last use of the pointer.

It would be the same if we would use shared_ptr (although that may lead to the conclusion that we store std::weak_ptr in our objects and thus, we would restrict the pointer type for users again).

So basically, it is a non-owning pointer. And we use a simpler depiction of such non-owning pointer: the non-owning raw pointer.

andrew-kennedy commented 7 years ago

I guess what's unfortunate about non owning raw pointers is it's impossible for calling code to know the called function is using the pointer in a non-owning way without reading the implementation or having documentation; there's no intent expressed in code. Do you think globjects would eventually move to a non-owning "observing" pointer type should it be introduced in a future C++ standard?

Also, even ignoring the possibility of a eventual standard non-owning type, is there a reason a custom type (as suggested in the blog post you linked to) was avoided?

If you require some reference semantics that are not provided by any existing type (and raw pointers are too broad, as usual), why not implement it yourself? A simple type that clearly expresses your intent can go a long way towards improving your code.

Sorry if I'm asking too many questions, I'm also trying to learn from globjects as an example of interface design I suppose 😊

scheibel commented 7 years ago

I'm just trying to use globjects as not just a cool wrapper library but also to learn about interface design!

Yeah we figured that a few questions ago :smile:

I'm actually looking forward to the std::observer_ptr but I think globjects is bound to be implemented in C++11 for the next couple of years. What we may do is writing our own observer pointer type and using it as a typedef for platforms with upcoming std::observer_ptr support. But then again, you quote suggests we write the lacking reference counting pointer ourselves (which we did) but we perceived half of the community using globjects were not content with the self-implemented solution.

andrew-kennedy commented 7 years ago

Ohh I see, I thought the quote (from the blog post) was implying that you merely should write the observer pointer type yourselves, as you mentioned. That seems like a pretty good compromise.

Either way, thanks for indulging my curiosity!