Skip to content
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

Also implement AsUniformValue for textures directly #1956

Merged
merged 2 commits into from Aug 12, 2021

Conversation

strohel
Copy link
Contributor

@strohel strohel commented Aug 10, 2021

In addition to implementing if for references, i.e.
impl<'a> AsUniformValue for &'a {TextureType}.

The method as_uniform_value(&self) takes self by reference, so there
should be no need to require the second-level reference.

This allows a longer lifetime to be automatically inferred. Fixes
compilation of the texture_2d_as_uniform_value_lifetime() test from the
previous commit.

This commit doesn't remove the original impl above in order not to introduce
breaking changes. Removing would mean every texture passing in uniform! {}
would have to change.

Should fix #1400. We should also be able to close #1054 (resolved in a slightly different manner).
Related closed issues/PRs: #1629, #1632.

Also related: #1850 - not fixed in this PR, but a similar change to impl for Sampler could fix it.
CC @slightknack.

Currently this fails to compile with:
```
error[E0515]: cannot return value referencing function parameter `texture`
   --> tests/texture_creation.rs:103:9
    |
103 |         texture.as_uniform_value()
    |         -------^^^^^^^^^^^^^^^^^^^
    |         |
    |         returns a value referencing data owned by the current function
    |         `texture` is borrowed here

error: aborting due to previous error

For more information about this error, try `rustc --explain E0515`.
```
In addition to implementing if for references, i.e.
`impl<'a> AsUniformValue for &'a {myname}`.

The method `as_uniform_value(&self)` takes self by reference, so there
should be no need to require the second-level reference.

This allows a longer lifetime to be automatically inferred. Fixes
compilation of the texture_2d_as_uniform_value_lifetime() test from the
previous commit.

This commit doesn't remove the original impl above in order not to introduce
breaking changes. Removing would mean every texture passing in `uniform! {}`
would have to change.
@slightknack
Copy link

Thank you @strohel! We're currently using this fork for a portion our pipeline, this change makes it easier to work with uniforms in an idiomatic Rust manner. Let's get this merged!

@est31
Copy link
Collaborator

est31 commented Aug 10, 2021

This commit doesn't remove the original impl above in order not to introduce
breaking changes. Removing would mean every texture passing in uniform! {}
would have to change.

Why would it need a change? As far as I'm aware, when a Trait is implemented for T, then it's also implemented for &'a T, no?

@strohel
Copy link
Contributor Author

strohel commented Aug 12, 2021

Why would it need a change? As far as I'm aware, when a Trait is implemented for T, then it's also implemented for &'a T, no?

Yes when used with a dot operator (method call resolution), but not when used as other function argument, per https://doc.rust-lang.org/nightly/nomicon/coercions.html:

Note that we do not perform coercions when matching traits (except for receivers, see the next page). If there is an impl for some type U and T coerces to U, that does not constitute an implementation for T. For example, the following will not type check, even though it is OK to coerce t to &T and there is an impl for &T:
(...)

The culprit here is impl<'n, T> UniformsStorage<'n, T, EmptyUniforms> where T: AsUniformValue { pub fn new(name: &'n str, value: T) } used from uniform!{}. If I remove the AsUniformValue impl for references, I get:

error[E0277]: the trait bound `&glium::texture::CompressedSrgbTexture2d: AsUniformValue` is not satisfied
   --> examples/image.rs:148:18
    |
148 |             tex: &opengl_texture
    |                  -^^^^^^^^^^^^^^
    |                  |
    |                  the trait `AsUniformValue` is not implemented for `&glium::texture::CompressedSrgbTexture2d`
    |                  help: consider removing the leading `&`-reference
    |
    = help: the following implementations were found:
              <glium::texture::CompressedSrgbTexture2d as AsUniformValue>

Passing textures by value might not be an option when the caller does not want to lose ownership.


If anyone has ideas how to resolve this without keeping the 2 similar AsUniformValue implementations, I'll happily try them out.

@est31
Copy link
Collaborator

est31 commented Aug 12, 2021

@strohel fair enough.

Copy link
Collaborator

@est31 est31 left a comment

Choose a reason for hiding this comment

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

Thanks!

@est31 est31 merged commit 710dba0 into glium:master Aug 12, 2021
@strohel
Copy link
Contributor Author

strohel commented Aug 12, 2021

Thanks, @est31, that was quick!

@strohel strohel deleted the asuniformvalue-for-textures-directly branch August 12, 2021 14:35
@strohel
Copy link
Contributor Author

strohel commented Sep 6, 2021

@est31, can we please request cutting a patch release with this fix? It would allow us to publish a package that depends on it.

@est31
Copy link
Collaborator

est31 commented Sep 6, 2021

@strohel sure, will do!

@est31 est31 mentioned this pull request Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsUniformType lifetime issue Make AsUniformValue::as_uniform_value() take self by value instead of reference
3 participants