Skip to content

Optional explicit compressed image format support #19190

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

Merged

Conversation

DGriffin91
Copy link
Contributor

@DGriffin91 DGriffin91 commented May 12, 2025

Objective

  • Allow compressed image formats to be used with ImagePlugin and GltfPlugin in cases where there is no RenderDevice resource. (For example, when using a custom render backend)

Solution

  • Define a CompressedImageFormatSupport component that allows the user to explicitly determine which formats are supported.

Not sure if this is the best solution. Alternatively, I considered initializing CompressedImageFormatSupport from render device features separately, it would need to run after the render device is initialized but before ImagePlugin and GltfPlugin finish. Not sure where the best place for that to happen would be.

Update: decided on going with @greeble-dev solution: defining the CompressedImageFormatSupport resource in bevy_image, but letting bevy_render register the resource value.

@greeble-dev
Copy link
Contributor

greeble-dev commented May 13, 2025

I've sketched out an alternative solution: define the CompressedImageFormatSupport resource in bevy_image, but let bevy_render register the resource value. This has the advantage that bevy_gltf and custom renderers wouldn't need to depend on bevy_render. Might be worth considering?

main...greeble-dev:bevy:gltf-compressed-image-format-support

It feels awfully fragile to rely on plugin initialisation order... if anything goes wrong then it silently fails. But I think that's kinda true of the current situation and this PR as well, so at least it's not a step back?

@greeble-dev greeble-dev added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 13, 2025
@DGriffin91
Copy link
Contributor Author

@greeble-dev I think I like your alternative design better. It's similar to the alternative I mentioned, and now seeing it I think I like it better than what I initially had. The plugin order initialization issue is unfortunate, but it does seem like bevy was already dependent on the initialization order that this requires, so agreed that it doesn't seem like a step back.

One thing to note, your example is missing the extra check needed here:

let supported_compressed_formats = match app.world().get_resource::<RenderDevice>() {
But I tested adding it there and it seemed to work as expected.

@greeble-dev How would you like me to get your changes into the PR? I want to make sure you get credit on the commit. I'm fine adding the check in bevy_render and the needed docs. Thanks!

In case it's helpful for testing here's a version of the FlightHelmet using BCn textures: FlightHelmetBCn.zip

And a minimal usage example:

use bevy::prelude::*;
use bevy_image::{CompressedImageFormatSupport, CompressedImageFormats};
use bevy_render::{settings::WgpuSettings, RenderPlugin};

fn main() {
    App::new()
        .insert_resource(CompressedImageFormatSupport(CompressedImageFormats::BC))
        .add_plugins(
            DefaultPlugins.set(RenderPlugin {
                render_creation: WgpuSettings {
                    backends: None,
                    ..default()
                }
                .into(),
                ..default()
            }),
        )
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(SceneRoot(asset_server.load(
        GltfAssetLabel::Scene(0).from_asset("models/FlightHelmetBCn/FlightHelmet.gltf"),
    )));
}

@greeble-dev
Copy link
Contributor

@DGriffin91, no credit necessary :]. I'm fine if you want to copy across whatever works for you.

Re the missing check, that got me thinking that this PR unlocks the possibility of moving the registration of ImageLoader entirely into bevy_image? Which is a scarier change and too big a step for this PR, but seems to align with the general direction of moving stuff out of bevy_render (#15812).

@DGriffin91
Copy link
Contributor Author

CI appears to be failing for reasons unrelated to the PR

@@ -46,6 +46,7 @@ bevy_color = { path = "../bevy_color", version = "0.16.0-dev", features = [
"serialize",
"wgpu-types",
] }
bevy_ecs = { path = "../bevy_ecs", version = "0.16.0-dev", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

For the benefit of other reviewers - this is not adding a dependency, just making it explicit. Previously was implicit bevy_image -> bevy_app -> bevy_ecs.

Copy link
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

This seems sketchy but not your place to refactor plugin initialization. I guess it's okay for now!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 18, 2025
@alice-i-cecile
Copy link
Member

Marking down as something to consider for #69.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 26, 2025
Merged via the queue into bevyengine:main with commit d79efad May 26, 2025
38 checks passed
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 targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants