Allow Manually Created Textures in IECoreGL::Shader::Setup

[danieldresser-ie]

I've left the V1 commit in the history, even though we agreed that we won't be using that approach.

Thinking about trying to actually use Texture to store the textures, adding API to Shader::Setup that takes a separate target felt very wrong, since the target is so clearly part of the Texture - in particular, there wouldn't be any nice path forward for code written using this weird API.

What I've ended up with here is a subclass RawTexture that adds the target type.
Pros: No modification needed to Shader::Setup API. Decent path forward in the long run - once we can change the API, RawTexture becomes a synonym/shim for Texture.
Cons: Some weird friend declarations needed to mess around with private stuff in the meantime. Shader::ScopedBinding doesn't work with RawTexture ( because ~ScopedBinding can't have any way to access the Texture or its type without breaking ABI ). I was feeling pretty proud of this plan until I realized that spanner in the works, but at least I can error clearly on that case, so maybe it's still OK?

Or maybe this was all a waste of time and I should have just gone with implementing the weird API on Shader::Setup::addUniformParameterWithOverriddenTextureTarget?

[johnhaddon]

adding API to Shader::Setup that takes a separate target felt very wrong, since the target is so clearly part of the Texture - in particular, there wouldn't be any nice path forward for code written using this weird API.

The path forward is pretty simple : one day you pass the type to the Texture constructor, and remove the type from the addUniformParameter() call.

Cons: Shader::ScopedBinding doesn't work with RawTexture

For me, this is a pretty big one. Why introduce an API feature if we can't support it properly, and will delete it later anyway?

Or maybe this was all a waste of time and I should have just gone with implementing the weird API

I'm afraid I'm inclined to think that. I'd rather have a minimal localised workaround than introduce a temporary class that doesn't fulfil its promise. The approach we originally agreed is a tiny addition to the API surface area, and I really don't think it's that weird :

/// As above, but specifying the texture target explicitly. This is a temporary workaround
/// because it is not currently possible to query the target from the texture itself.
/// \todo Store target as a member of Texture, and remove this overload.
addUniformParameter( const string &name, ConstTexturePtr &value, GLenum target );

[johnhaddon]

Alternative thought : we do know the type of the uniform parameter we're setting, so can we just derive the texture target from that? GL_TEXTURE_2D for GL_*SAMPLER_2D etc....

[danieldresser-ie]

Switched to deducing texture target from the sampler, and everything appears to still be working.

[johnhaddon]

LGTM. I assume we want it on RB-10.4 though?

[johnhaddon]

Why wouldn't we just provide a public query for the target type?

[danieldresser-ie]

I'm really confused by this comment and the next one ... since you decided we wouldn't go with the version of the code that uses RawTexture. I was freaked out that I may have uploaded the wrong version, but looking at this PR now, I only see one commit, and it doesn't add RawTexture ... I guess you were looking at something out of date?

[johnhaddon]

Oh, yeah, that is freaky. Those are comments I made during my first review - I definitely didn't intentionally repeat them for this one, as there was only that one commit to look at. I guess we put it down to a GitHub bug...

Just to be completely clear, this LGTM, so feel free to merge once you've retargeted it to RB-10.4.

[johnhaddon]

Since RawTexture is RunTimeTyped, I think this should be runTimeCast?

[johnhaddon]

At some point I think we might as well add a public accessor for the texture ID anyway.