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

reexported image crate from bevy_render as bevy::render::util::image #12724

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stinkytoe
Copy link
Contributor

Objective

Fixes #12689

Solution

Added reexport of image crate to bevy_render/src/lib.rs

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use labels Mar 26, 2024
@alice-i-cecile
Copy link
Member

I'd really like project level guidelines on what should be exported and what should not. This doesn't feel like a clear win to me.

@stinkytoe
Copy link
Contributor Author

That's fair. I hadn't read #11478 yet when I made this, so I didn't know that reexports were a discussion topic already.

My reasoning as a user is, for a large monolithic crate such as a game engine, I want to make sure I'm using the same version of the tools as you are.

What inspired my original issue was that the image crate is currently on version 0.25 while bevy is using 0.24 . I originally just added the current version of image when I discovered that I needed some of its functionality. This lead to some confusing compiler errors on my part. Easy enough to fix, I just downgraded to the version used in bevy_render's Cargo.toml . It's not that hard to do, I just thought it was easier to use a reexport like I already was for thiserror and hashbrown. It just makes my job of maintaining parity easier.

I can see the arguments the other way too, though. Also I have no idea what the ramifications on build time or binary size are for one strategy over the other.

(I'll still love y'all if you reject this!)

Copy link
Contributor

@rlidwka rlidwka left a comment

Choose a reason for hiding this comment

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

This is a good idea because Bevy already exports part of the functionality of image crate (e.g. via Image::try_into_dynamic()). But without full re-export of the crate, its functionality is only partially usable.


I've just ran into this issue, here's an example:

// this works without any dependencies
image.try_into_dynamic().unwrap().save("file");
// this doesn't, since bevy doesn't re-export ImageOutputFormat
image.try_into_dynamic().unwrap().save_with_format("file", ???::ImageOutputFormat::Png);

So as it stands now, to use save_with_format, I have to add old version of image as a dependency explicitly, which is annoying for many reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Consider re-exporting image crate
3 participants