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

Change gl::ScopedTextureBind to not require explicit cast in VS2013 #545

Closed
sansumbrella opened this Issue Oct 27, 2014 · 12 comments

Comments

Projects
None yet
4 participants
@sansumbrella
Contributor

sansumbrella commented Oct 27, 2014

Only one order for the numerical parameters should be allowed, and it must be unambiguous in VS2013.

GLuint and uin8_t are ambiguous between each other.

Looking at the ctors in the latest glNext, I'm not sure what needs to change, since the first parameter is different in the cases where the integer parameters are equivalent.

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Oct 27, 2014

Collaborator

Seems this is a known problem in at least this sample, and talked about on the ImGui forum thread.

diagnostic:

1>d:\code\cinder\cinder-glnext\samples\_opengl\postprocessingaa\src\smaa\smaa.cpp(175): error C2668: 'cinder::gl::ScopedTextureBind::ScopedTextureBind' : ambiguous call to overloaded function
1>          d:\code\cinder\cinder-glnext\include\cinder\gl\gl.h(527): could be 'cinder::gl::ScopedTextureBind::ScopedTextureBind(const cinder::gl::TextureBaseRef &,uint8_t)'
1>          d:\code\cinder\cinder-glnext\include\cinder\gl\gl.h(524): or       'cinder::gl::ScopedTextureBind::ScopedTextureBind(GLenum,GLuint)'
1>          while trying to match the argument list '(cinder::gl::Texture2dRef, int)'

My understanding of this is that the compiler can't implicitly cast both arguments, and the second on is explicitly an int (not uint8_t). clang doesn't seem to care, and I'm not sure what the 'correct' thing is from the standards point of view.

Might just mean we need to add overloads for all of the gl::Texture* variants..

Collaborator

richardeakin commented Oct 27, 2014

Seems this is a known problem in at least this sample, and talked about on the ImGui forum thread.

diagnostic:

1>d:\code\cinder\cinder-glnext\samples\_opengl\postprocessingaa\src\smaa\smaa.cpp(175): error C2668: 'cinder::gl::ScopedTextureBind::ScopedTextureBind' : ambiguous call to overloaded function
1>          d:\code\cinder\cinder-glnext\include\cinder\gl\gl.h(527): could be 'cinder::gl::ScopedTextureBind::ScopedTextureBind(const cinder::gl::TextureBaseRef &,uint8_t)'
1>          d:\code\cinder\cinder-glnext\include\cinder\gl\gl.h(524): or       'cinder::gl::ScopedTextureBind::ScopedTextureBind(GLenum,GLuint)'
1>          while trying to match the argument list '(cinder::gl::Texture2dRef, int)'

My understanding of this is that the compiler can't implicitly cast both arguments, and the second on is explicitly an int (not uint8_t). clang doesn't seem to care, and I'm not sure what the 'correct' thing is from the standards point of view.

Might just mean we need to add overloads for all of the gl::Texture* variants..

@richardeakin richardeakin added the gl label Oct 27, 2014

@sansumbrella

This comment has been minimized.

Show comment
Hide comment
@sansumbrella

sansumbrella Oct 27, 2014

Contributor

Yep. I remember adding a cast in a handful of the other samples, although on the second parameter.

I made an issue when I saw the forum post so this doesn't fall through the cracks.

Contributor

sansumbrella commented Oct 27, 2014

Yep. I remember adding a cast in a handful of the other samples, although on the second parameter.

I made an issue when I saw the forum post so this doesn't fall through the cracks.

@paulhoux

This comment has been minimized.

Show comment
Hide comment
@paulhoux

paulhoux Oct 27, 2014

Collaborator

I've tried making the constructors explicit, as well as reordering the parameters (texture unit first), but that did not solve anything. What amazes me most is that a shared_ptr is implicitly cast to a GLenum.

Collaborator

paulhoux commented Oct 27, 2014

I've tried making the constructors explicit, as well as reordering the parameters (texture unit first), but that did not solve anything. What amazes me most is that a shared_ptr is implicitly cast to a GLenum.

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Oct 27, 2014

Collaborator

Yea I mean, looking at the error from our perspective, you can't help but think it is a vc++ compiler error.

What if you add more constructors that take Texture2dRef, Texture3dRef, TextureCube, etc.. annoying I know.. but at least there is only a limited number of Texture types it could be.

Collaborator

richardeakin commented Oct 27, 2014

Yea I mean, looking at the error from our perspective, you can't help but think it is a vc++ compiler error.

What if you add more constructors that take Texture2dRef, Texture3dRef, TextureCube, etc.. annoying I know.. but at least there is only a limited number of Texture types it could be.

@sansumbrella

This comment has been minimized.

Show comment
Hide comment
@sansumbrella

sansumbrella Oct 27, 2014

Contributor

What if we changed it to follow Herb Sutter's advice for parameter passing from CppCon?

That is, pass by const reference to type (not the shared_ptr). This slightly changes our calling code, but I doubt that most const Type& types will silently and ambiguously convert to ints.

// Constructor definition
ScopedTextureBind( const TextureBase &texture, uint8_t textureUnit );
// Calling code
TextureRef tex;
gl::ScopedTextureBind texture( *tex, 1 );

Part of the reason for preferring const ref to type arguments is it makes it clear that the function will not participate in the lifetime management of the argument. That is, it is clear that you aren't copying the shared_ptr, since shared_ptr isn't anywhere to be seen in the definition.

This of course would result in reconsidering how we pass textures to the Cinder GL stack in general, but that could be a good thing.

Contributor

sansumbrella commented Oct 27, 2014

What if we changed it to follow Herb Sutter's advice for parameter passing from CppCon?

That is, pass by const reference to type (not the shared_ptr). This slightly changes our calling code, but I doubt that most const Type& types will silently and ambiguously convert to ints.

// Constructor definition
ScopedTextureBind( const TextureBase &texture, uint8_t textureUnit );
// Calling code
TextureRef tex;
gl::ScopedTextureBind texture( *tex, 1 );

Part of the reason for preferring const ref to type arguments is it makes it clear that the function will not participate in the lifetime management of the argument. That is, it is clear that you aren't copying the shared_ptr, since shared_ptr isn't anywhere to be seen in the definition.

This of course would result in reconsidering how we pass textures to the Cinder GL stack in general, but that could be a good thing.

@sansumbrella

This comment has been minimized.

Show comment
Hide comment
@sansumbrella

sansumbrella Oct 27, 2014

Contributor

Herb's advice is also consolidated on gotw

Contributor

sansumbrella commented Oct 27, 2014

Herb's advice is also consolidated on gotw

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Oct 27, 2014

Collaborator

Well not only how we pass Textures (by const& or const shared_ptr&), but GlslProg's, BufferObj's, etc. It would either create an inconsistency, or we're going to have to dereference shared_ptr's all over the place. One of the selling points in a shared_ptr based design is that as a user you can treat them, for the most part, as regular objects.

While I think what Herb wrote there has some merit, the most practical variant is const shared_ptr&. Otherwise it is going to get annoying, trying to remember what type to pass for gl data types..

Collaborator

richardeakin commented Oct 27, 2014

Well not only how we pass Textures (by const& or const shared_ptr&), but GlslProg's, BufferObj's, etc. It would either create an inconsistency, or we're going to have to dereference shared_ptr's all over the place. One of the selling points in a shared_ptr based design is that as a user you can treat them, for the most part, as regular objects.

While I think what Herb wrote there has some merit, the most practical variant is const shared_ptr&. Otherwise it is going to get annoying, trying to remember what type to pass for gl data types..

@sansumbrella

This comment has been minimized.

Show comment
Hide comment
@sansumbrella

sansumbrella Oct 28, 2014

Contributor

Yes, I think it is worth considering the change to parameter passing throughout.

I tend to think of shared_ptr's as regular pointers most of the time, for a couple of reasons:

  • They implicitly convert to bool
  • You need to dereference them to do anything with the pointed-to object (using the -> operator or * if you want to use operators rather than methods)
  • In VS2013, they implicitly convert to other numeric types, apparently

Passing const shared_ptr& almost certainly makes it easier for beginners to feel comfortable with the framework. It's also nice that it doesn't feel like you need to "modify" the type you are storing to use most of Cinder's functions. Beginners have often asked on the forums about how to use the ip:: methods because they were confused about how to pass a pointer to them.

On the other hand, we're passing shared_ptr to these methods while ignoring the fact that they are shared_ptr in the implementation and not caring about how they are actually scoped.

To try this out, we can add const& versions of the free functions that are called by the const shared_ptr& versions, so both approaches work. The shared_ptr interface would essentially be convenience for people who aren't comfortable or otherwise don't want to explicitly dereference the shared_ptr.

// const& function definition will do the actual work.
void gl::draw( const Texture &tex );

// shared_ptr overload calls the const& function.
inline void gl::draw( const shared_ptr<Texture> &tex ) { gl::draw( *tex ); }

If there wasn't an issue of pointers being implicitly convertible to numbers, I would happily continue passing const shared_ptr& around. Since the issue exists, I think it's less confusing to pass const&, and worth trying out to see how it feels in practice.

Contributor

sansumbrella commented Oct 28, 2014

Yes, I think it is worth considering the change to parameter passing throughout.

I tend to think of shared_ptr's as regular pointers most of the time, for a couple of reasons:

  • They implicitly convert to bool
  • You need to dereference them to do anything with the pointed-to object (using the -> operator or * if you want to use operators rather than methods)
  • In VS2013, they implicitly convert to other numeric types, apparently

Passing const shared_ptr& almost certainly makes it easier for beginners to feel comfortable with the framework. It's also nice that it doesn't feel like you need to "modify" the type you are storing to use most of Cinder's functions. Beginners have often asked on the forums about how to use the ip:: methods because they were confused about how to pass a pointer to them.

On the other hand, we're passing shared_ptr to these methods while ignoring the fact that they are shared_ptr in the implementation and not caring about how they are actually scoped.

To try this out, we can add const& versions of the free functions that are called by the const shared_ptr& versions, so both approaches work. The shared_ptr interface would essentially be convenience for people who aren't comfortable or otherwise don't want to explicitly dereference the shared_ptr.

// const& function definition will do the actual work.
void gl::draw( const Texture &tex );

// shared_ptr overload calls the const& function.
inline void gl::draw( const shared_ptr<Texture> &tex ) { gl::draw( *tex ); }

If there wasn't an issue of pointers being implicitly convertible to numbers, I would happily continue passing const shared_ptr& around. Since the issue exists, I think it's less confusing to pass const&, and worth trying out to see how it feels in practice.

@andrewfb

This comment has been minimized.

Show comment
Hide comment
@andrewfb

andrewfb Oct 28, 2014

Collaborator

I weighed all of this at the start of glNext but I'm fairly convinced the average Cinder user would be at least annoyed and probably confused by requiring a deeper understanding of shared_ptr et al. Because the types in question (in the gl:: family) cannot exist as anything but *Ref, my feeling has been that it makes sense to shape the API accordingly. This immediate case is a compiler bug (and it's fixed in VC 2014) so I would definitely not make a sweeping API change in response to it. However it's something I've weighed since before Cinder was public. If it were only advanced C++ users I would agree. And there is an argument to be had that we're doing a bit of a disservice protecting new C++ users from the realities of pointers. To some extent this will come to a head as I refactor Surface to no longer have an implicit shared_ptr - it will support both Surface and SurfaceRef constructions.

I don't love replicating the methods to take both T& and shared_ptr as it seems a bit heavy-handed to protect a user from having to learn how pointers work, and it makes ambiguous what the "right" thing to do is. From a normal C++ perspective receiving a const T& is the right thing and a more advanced user would potentially prefer that. For the time being I'm undecided on this, and have been for about 4 years now.

Collaborator

andrewfb commented Oct 28, 2014

I weighed all of this at the start of glNext but I'm fairly convinced the average Cinder user would be at least annoyed and probably confused by requiring a deeper understanding of shared_ptr et al. Because the types in question (in the gl:: family) cannot exist as anything but *Ref, my feeling has been that it makes sense to shape the API accordingly. This immediate case is a compiler bug (and it's fixed in VC 2014) so I would definitely not make a sweeping API change in response to it. However it's something I've weighed since before Cinder was public. If it were only advanced C++ users I would agree. And there is an argument to be had that we're doing a bit of a disservice protecting new C++ users from the realities of pointers. To some extent this will come to a head as I refactor Surface to no longer have an implicit shared_ptr - it will support both Surface and SurfaceRef constructions.

I don't love replicating the methods to take both T& and shared_ptr as it seems a bit heavy-handed to protect a user from having to learn how pointers work, and it makes ambiguous what the "right" thing to do is. From a normal C++ perspective receiving a const T& is the right thing and a more advanced user would potentially prefer that. For the time being I'm undecided on this, and have been for about 4 years now.

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Nov 15, 2014

Collaborator

As a quick solution, I propose to add overloads to ScopedTextureBind that take Texture2d, Texture3d, and TextureCubeMap as arguments, which should resolve the ambiguities in vs2013 (and could eventually be removed). If this is acceptable with others, I'll make a PR.

Collaborator

richardeakin commented Nov 15, 2014

As a quick solution, I propose to add overloads to ScopedTextureBind that take Texture2d, Texture3d, and TextureCubeMap as arguments, which should resolve the ambiguities in vs2013 (and could eventually be removed). If this is acceptable with others, I'll make a PR.

@sansumbrella

This comment has been minimized.

Show comment
Hide comment
@sansumbrella

sansumbrella Nov 15, 2014

Contributor

I am in favor of the quick solution.

In my application code, I am enjoying following the c++ parameter-passing advice, but I agree that for most people (and given the constraints of all the gl:: objects), it would be annoying to need to explicitly dereference at every gl:: function call.

When Surface comes around to being an explicitly shared object, though, I think it will be better to pass by const Surface & rather than const SurfaceRef &. Because there's no reason you need to dynamically allocate a temporary surface that just gets passed directly to a texture.

Contributor

sansumbrella commented Nov 15, 2014

I am in favor of the quick solution.

In my application code, I am enjoying following the c++ parameter-passing advice, but I agree that for most people (and given the constraints of all the gl:: objects), it would be annoying to need to explicitly dereference at every gl:: function call.

When Surface comes around to being an explicitly shared object, though, I think it will be better to pass by const Surface & rather than const SurfaceRef &. Because there's no reason you need to dynamically allocate a temporary surface that just gets passed directly to a texture.

richardeakin added a commit to richardeakin/Cinder that referenced this issue Nov 18, 2014

@richardeakin richardeakin modified the milestone: 0.9.0 Nov 20, 2014

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Nov 21, 2014

Collaborator

Fixed with #595.

Collaborator

richardeakin commented Nov 21, 2014

Fixed with #595.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment