Skip to content

Change Texture2D::fill to be non-mutable#549

Merged
asny merged 1 commit intoasny:masterfrom
maxime-tournier:maxime-tournier/texture-fill-mut
Jun 14, 2025
Merged

Change Texture2D::fill to be non-mutable#549
asny merged 1 commit intoasny:masterfrom
maxime-tournier:maxime-tournier/texture-fill-mut

Conversation

@maxime-tournier
Copy link
Contributor

This PR changes Texture2D::fill to be non-mutable, so that one can change a texture's contents through a material Texture2DRef (which is behind an Arc). I'm using this to display my webcam stream on a textured quad.

It's unclear to me why the method currently takes a mutable ref to self since all the side-effects are on the GPU (just like generate_mip_maps, which is non-mutable), so feel free to enlighten me on this point :)

Best regards,

@asny
Copy link
Owner

asny commented Jun 14, 2025

Sorry for the really really late reply 😬 I think I was just being a bit idealistic and wanted functions that mutated something to be mutable so you don't accidentally edit something and it follows the Rust ideology. I think it's a nice idea, but it's hard to do in practice. For example, the render targets are usually used for writing, hence it's mutable to create a render target, but you can also use them for reading back to the CPU, so in that case it should be non-mutable. And your example with generating mip maps should also take a mutable reference, but since it's not enforced by the compiler, I didn't see it.

So I think I'm going to change all cases of &mut self to &self in the core to be consistent and not limit anybody. Finally, I think the real problem is the TextureRef, it should not need an Arc, but that's another problem 🙂

@asny asny merged commit 3d967a6 into asny:master Jun 14, 2025
4 checks passed
@asny
Copy link
Owner

asny commented Jun 14, 2025

Changed the rest of the texture functions here

@maxime-tournier
Copy link
Contributor Author

Thanks a lot for the detailed explanations 👍

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.

2 participants