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.

Sign up for GitHub

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

Log a warning when the tonemapping_luts feature is disabled but required for the selected tonemapper. #10253

Merged
merged 6 commits into from
Oct 27, 2023
Merged

Log a warning when the tonemapping_luts feature is disabled but required for the selected tonemapper. #10253

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a48a426
add ktx2 and zstd features to the tonemapping_luts feature
Elabajaba Oct 25, 2023
e49b360
log an error when the tonemapping_luts feature is disabled but requir…
Elabajaba Oct 25, 2023
7e869d6
update cargo_features
Elabajaba Oct 25, 2023
f0514d5
make the error message actionable
Elabajaba Oct 25, 2023
6fa3987
appease ci
Elabajaba Oct 25, 2023
50ea38a
better error message
Elabajaba Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ default = [
"multi-threaded",
"png",
"hdr",
"ktx2",
"zstd",
"vorbis",
"x11",
"bevy_gizmos",
Expand Down Expand Up @@ -223,8 +221,8 @@ android_shared_stdcxx = ["bevy_internal/android_shared_stdcxx"]
# Enable detailed trace event logging. These trace events are expensive even when off, thus they require compile time opt-in
detailed_trace = ["bevy_internal/detailed_trace"]

# Include tonemapping Look Up Tables KTX2 files
tonemapping_luts = ["bevy_internal/tonemapping_luts"]
# Include tonemapping Look Up Tables KTX2 files. If everything is pink, you need to enable this feature or change the `Tonemapping` method on your `Camera2dBundle` or `Camera3dBundle`.
tonemapping_luts = ["bevy_internal/tonemapping_luts", "ktx2", "zstd"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit ugly (since tonemapping_luts already depends on them in bevy_core_pipeline's cargo.toml), but stops the ktx2 and zstd features from appearing as optional features in the cargo_features doc when they're already enabled.


# Enable AccessKit on Unix backends (currently only works with experimental screen readers and forks.)
accesskit_unix = ["bevy_internal/accesskit_unix"]
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_core_pipeline/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ keywords = ["bevy"]
[features]
trace = []
webgl = []
tonemapping_luts = []
tonemapping_luts = ["bevy_render/ktx2", "bevy_render/zstd"]

[dependencies]
# bevy
Expand All @@ -24,6 +24,7 @@ bevy_asset = { path = "../bevy_asset", version = "0.12.0-dev" }
bevy_core = { path = "../bevy_core", version = "0.12.0-dev" }
bevy_derive = { path = "../bevy_derive", version = "0.12.0-dev" }
bevy_ecs = { path = "../bevy_ecs", version = "0.12.0-dev" }
bevy_log = { path = "../bevy_log", version = "0.12.0-dev" }
bevy_reflect = { path = "../bevy_reflect", version = "0.12.0-dev" }
bevy_render = { path = "../bevy_render", version = "0.12.0-dev" }
bevy_transform = { path = "../bevy_transform", version = "0.12.0-dev" }
Expand Down
27 changes: 25 additions & 2 deletions crates/bevy_core_pipeline/src/tonemapping/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,19 +185,42 @@ impl SpecializedRenderPipeline for TonemappingPipeline {
if let DebandDither::Enabled = key.deband_dither {
shader_defs.push("DEBAND_DITHER".into());
}

match key.tonemapping {
Tonemapping::None => shader_defs.push("TONEMAP_METHOD_NONE".into()),
Tonemapping::Reinhard => shader_defs.push("TONEMAP_METHOD_REINHARD".into()),
Tonemapping::ReinhardLuminance => {
shader_defs.push("TONEMAP_METHOD_REINHARD_LUMINANCE".into());
}
Tonemapping::AcesFitted => shader_defs.push("TONEMAP_METHOD_ACES_FITTED".into()),
Tonemapping::AgX => shader_defs.push("TONEMAP_METHOD_AGX".into()),
Tonemapping::AgX => {
#[cfg(not(feature = "tonemapping_luts"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why warn instead of gating the actual enum variants behind the flag? That way we'd make the invalid states not representable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not change the default tonemapper depending on what features are enabled, and this avoids users wondering why their bevy library or game (with default features disabled) looks different than everyone else's.

bevy_log::error!(
"AgX tonemapping requires the `tonemapping_luts` feature.
Either enable the `tonemapping_luts` feature for bevy in `Cargo.toml`,
or use a different tonemapping method in your CameraBundle."
);
shader_defs.push("TONEMAP_METHOD_AGX".into());
}
Tonemapping::SomewhatBoringDisplayTransform => {
shader_defs.push("TONEMAP_METHOD_SOMEWHAT_BORING_DISPLAY_TRANSFORM".into());
}
Tonemapping::TonyMcMapface => shader_defs.push("TONEMAP_METHOD_TONY_MC_MAPFACE".into()),
Tonemapping::TonyMcMapface => {
#[cfg(not(feature = "tonemapping_luts"))]
bevy_log::error!(
"TonyMcMapFace tonemapping requires the `tonemapping_luts` feature.
Either enable the `tonemapping_luts` feature for bevy in `Cargo.toml`,
or use a different tonemapping method in your CameraBundle."
);
shader_defs.push("TONEMAP_METHOD_TONY_MC_MAPFACE".into());
}
Tonemapping::BlenderFilmic => {
#[cfg(not(feature = "tonemapping_luts"))]
bevy_log::error!(
"BlenderFilmic tonemapping requires the `tonemapping_luts` feature.
Either enable the `tonemapping_luts` feature for bevy in `Cargo.toml`,
or use a different tonemapping method in your CameraBundle."
);
shader_defs.push("TONEMAP_METHOD_BLENDER_FILMIC".into());
}
}
Expand Down
2 changes: 1 addition & 1 deletion docs/cargo_features.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ The default feature set enables most of the expected features of a game engine,
|ktx2|KTX2 compressed texture support|
|multi-threaded|Enables multithreaded parallelism in the engine. Disabling it forces all engine tasks to run on a single thread.|
|png|PNG image format support|
|tonemapping_luts|Include tonemapping Look Up Tables KTX2 files|
|tonemapping_luts|Include tonemapping Look Up Tables KTX2 files. If everything is pink, you need to enable this feature or change the `Tonemapping` method on your `Camera2dBundle` or `Camera3dBundle`.|
|vorbis|OGG/VORBIS audio format support|
|webgl2|Enable some limitations to be able to use WebGL2. If not enabled, it will default to WebGPU in Wasm. Please refer to the [WebGL2 and WebGPU](https://github.com/bevyengine/bevy/tree/latest/examples#webgl2-and-webgpu) section of the examples README for more information on how to run Wasm builds with WebGPU.|
|x11|X11 display server support|
Expand Down
Loading