cginternals / globjects

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

Explicit memory management (closes #276) #283

Closed scheibel closed 9 years ago

scheibel commented 9 years ago

Current solution to remove ref_ptr in globjects.

Open issues

What is your impression on the changes and implications on depending libraries / programs?

scheibel commented 9 years ago

E.g. during the refactoring of IncludeProcessor the functionality can be moved to gloperate.

j-o commented 9 years ago

This does not compile on MSVC because unique_ptrs can't be used with forward declared types (produces error C2338: can't delete an incomplete type).

While allowing explicit memory management might generally be a good idea, I think in the current state the price in terms of convenience loss is much too high. My primary concern here is the whole StringSource/Shader/Progam/Uniform architecture that now easily requires managing 10 or more pointers (e.g. 3 Shaders, 3 StringSources, 1 Program, 2 Uniforms) instead of a single one.

Further, I think the IncludeProcessor should remain here, as it directly emulates OpenGL functionality. The current ShadingLanguageIncludeImplementation architecture allows the transparent fallback, which would not be possible with the IncludeProcessor living elsewhere.

scheibel commented 9 years ago

This does not compile on MSVC because unique_ptrs can't be used with forward declared types (produces error C2338: can't delete an incomplete type).

Quite unfortunate, but this can be fixed.

While allowing explicit memory management might generally be a good idea, I think in the current state the price in terms of convenience loss is much too high. My primary concern here is the whole StringSource/Shader/Progam/Uniform architecture that now easily requires managing 10 or more pointers (e.g. 3 Shaders, 3 StringSources, 1 Program, 2 Uniforms) instead of a single one.

The issue with Uniforms can be addressed using UniformGroup (in gloperate) or UniformManager (probably integrated into gloperate). The issue with the shaders and their sources (as well as the IncludeProcessor restructuring) can be fixed with declaring pointer ownership of shaders over their shader sources, but with the drawback of denied sharing of sources between shaders. The same sharing argument can be applied between the sharing of shaders between programs (and uniforms and programs).

Further, I think the IncludeProcessor should remain here, as it directly emulates OpenGL functionality. The current ShadingLanguageIncludeImplementation architecture allows the transparent fallback, which would not be possible with the IncludeProcessor living elsewhere.

This decision should be based on the granularity of object oriented wrapping intended by globjects. If we intend to provide just a simple object-oriented wrapper around OpenGL, such a feature wouldn't fit in there. gloperate is designed to provide more graphics-rendering-engine-like functionalities, so emulating such complex feature would fit there just fine. Currently, globjects supports several convenience features that are not part of a strict OpenGL object oriented interface but a rendering engine. Either we should reconsider the current abstraction in globjects or try live with the consequences.

scheibel commented 9 years ago

Jenkins, retest this please.