Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Use unique_resource for GL objects #5172

Merged
merged 5 commits into from
Jun 1, 2016

Conversation

brunoabinader
Copy link
Member

Source: https://github.com/okdshin/unique_resource

These replace the complexity of manually handling moveable-RAII objects with a type specific for that purpose.

As suggested in #5141 (comment).

👀 @jfirebaugh @kkaefer @tmpsantos @1ec5

@brunoabinader brunoabinader added refactor Core The cross-platform C++ core, aka mbgl labels May 27, 2016
namespace mbgl {
namespace gl {

constexpr GLsizei TextureMax = 64;

using UniqueObject = std_experimental::unique_resource<GLuint, std::function<void(GLuint)>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using std::function polymorphism, create a unique *Deleter functor and Unique* typedef for each resource type as sketched in #5141 (comment). This adds additional type safety: mistaken use of the wrong resource type will be a compile time error.

@brunoabinader brunoabinader force-pushed the brunoabinader-globjects-refactor branch from d59f361 to 1dfa2db Compare May 31, 2016 00:10
@brunoabinader
Copy link
Member Author

Changes since last review:

  • s/gl::GLObjectStore/gl::ObjectStore/
  • Using *Deleter functors and Unique* aliases for unique_resource usage.
  • Dealt with absence of copy assignment in gl::ObjectStore reference inside all *Deleter's using std::reference_wrapper
  • Dealt with absence of default constructor in unique_resource with mbgl::optional where needed (e.g. in places where the GL object is lazily created).

@brunoabinader brunoabinader force-pushed the brunoabinader-globjects-refactor branch 2 times, most recently from 4d59236 to 37ff9e6 Compare May 31, 2016 09:45
@brunoabinader brunoabinader force-pushed the brunoabinader-globjects-refactor branch 2 times, most recently from 69bf354 to 927c00c Compare May 31, 2016 17:02
if (buffer.created()) {
MBGL_CHECK_ERROR(glBindBuffer(bufferType, getID()));
if (buffer) {
MBGL_CHECK_ERROR(glBindBuffer(bufferType, (*buffer).get()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't *buffer work, using the unique_resource implicit conversion?

Copy link
Member Author

@brunoabinader brunoabinader May 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept .get() to keep in sync with the other Unique* usage elsewhere, but your suggestion looks better, thanks!

@brunoabinader brunoabinader force-pushed the brunoabinader-globjects-refactor branch 2 times, most recently from 3298927 to c7e697f Compare May 31, 2016 19:06
@tmpsantos
Copy link
Contributor

@brunoabinader this is looking really nice. Do you think it would be possible to make utests for these holders? (not just that, but also the other wrappers we have for GL stuff)

@brunoabinader brunoabinader force-pushed the brunoabinader-globjects-refactor branch from c7e697f to 3175358 Compare June 1, 2016 12:30
@tmpsantos
Copy link
Contributor

Neat, thanks for adding the tests.

@brunoabinader brunoabinader merged commit 3175358 into master Jun 1, 2016
@brunoabinader brunoabinader deleted the brunoabinader-globjects-refactor branch June 1, 2016 12:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants