Log a warning when the tonemapping_luts feature is disabled but required for the selected tonemapper.
#10253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
You added a new feature but didn't update the readme. Please run |
coreh
reviewed
Oct 25, 2023
| 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"))] |
There was a problem hiding this comment.
Why warn instead of gating the actual enum variants behind the flag? That way we'd make the invalid states not representable.
There was a problem hiding this comment.
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.
Elabajaba
commented
Oct 25, 2023
| # 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"] |
There was a problem hiding this comment.
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.
alice-i-cecile
added
A-Rendering
Agreed on both counts :) |
alice-i-cecile
approved these changes
Oct 26, 2023
cart
approved these changes
Oct 27, 2023
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
Waiting on some final tweaks that the author wanted to add :) |
This was referenced Oct 27, 2023
ameknite
pushed a commit
to ameknite/bevy
that referenced
this pull request
Nov 6, 2023
…uired for the selected tonemapper. (bevyengine#10253) # Objective Make it obvious why stuff renders pink when rendering stuff with bevy with `default_features = false` and bevy's default tonemapper (TonyMcMapFace, it requires a LUT which requires the `tonemapping_luts`, `ktx2`, and `zstd` features). Not sure if this should be considered as fixing these issues, but in my previous PR (bevyengine#9073, and old discussions on discord that I only somewhat remember) it seemed like we didn't want to make ktx2 and zstd required features for bevy_core_pipeline. Related bevyengine#9179 Related bevyengine#9098 ## Solution This logs an error when a LUT based tonemapper is used without the `tonemapping_luts` feature enabled, and cleans up the default features a bit (`tonemapping_luts` now includes the `ktx2` and `zstd` features, since it panics without them). Another solution would be to fall back to a non-lut based tonemapper, but I don't like this solution as then it's not explicitly clear to users why eg. a library example renders differently than a normal bevy app (if the library forgot the `tonemapping_luts` feature). I did remove the `ktx2` and `zstd` features from the list of default features in Cargo.toml, as I don't believe anything else currently in bevy relies on them (or at least searching through every hit for `ktx2` and `zstd` didn't show anything except loading an environment map in some examples), and they still show up in the `cargo_features` doc as default features. --- ## Changelog - The `tonemapping_luts` feature now includes both the `ktx2` and `zstd` features to avoid a panic when the `tonemapping_luts` feature was enable without both the `ktx2` and `zstd` feature enabled.
rdrpenguin04
pushed a commit
to rdrpenguin04/bevy
that referenced
this pull request
Jan 9, 2024
…uired for the selected tonemapper. (bevyengine#10253) # Objective Make it obvious why stuff renders pink when rendering stuff with bevy with `default_features = false` and bevy's default tonemapper (TonyMcMapFace, it requires a LUT which requires the `tonemapping_luts`, `ktx2`, and `zstd` features). Not sure if this should be considered as fixing these issues, but in my previous PR (bevyengine#9073, and old discussions on discord that I only somewhat remember) it seemed like we didn't want to make ktx2 and zstd required features for bevy_core_pipeline. Related bevyengine#9179 Related bevyengine#9098 ## Solution This logs an error when a LUT based tonemapper is used without the `tonemapping_luts` feature enabled, and cleans up the default features a bit (`tonemapping_luts` now includes the `ktx2` and `zstd` features, since it panics without them). Another solution would be to fall back to a non-lut based tonemapper, but I don't like this solution as then it's not explicitly clear to users why eg. a library example renders differently than a normal bevy app (if the library forgot the `tonemapping_luts` feature). I did remove the `ktx2` and `zstd` features from the list of default features in Cargo.toml, as I don't believe anything else currently in bevy relies on them (or at least searching through every hit for `ktx2` and `zstd` didn't show anything except loading an environment map in some examples), and they still show up in the `cargo_features` doc as default features. --- ## Changelog - The `tonemapping_luts` feature now includes both the `ktx2` and `zstd` features to avoid a panic when the `tonemapping_luts` feature was enable without both the `ktx2` and `zstd` feature enabled.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Make it obvious why stuff renders pink when rendering stuff with bevy with
default_features = falseand bevy's default tonemapper (TonyMcMapFace, it requires a LUT which requires thetonemapping_luts,ktx2, andzstdfeatures).Not sure if this should be considered as fixing these issues, but in my previous PR (#9073, and old discussions on discord that I only somewhat remember) it seemed like we didn't want to make ktx2 and zstd required features for bevy_core_pipeline.
Related #9179
Related #9098
Solution
This logs an error when a LUT based tonemapper is used without the
tonemapping_lutsfeature enabled, and cleans up the default features a bit (tonemapping_lutsnow includes thektx2andzstdfeatures, since it panics without them).Another solution would be to fall back to a non-lut based tonemapper, but I don't like this solution as then it's not explicitly clear to users why eg. a library example renders differently than a normal bevy app (if the library forgot the
tonemapping_lutsfeature).I did remove the
ktx2andzstdfeatures from the list of default features in Cargo.toml, as I don't believe anything else currently in bevy relies on them (or at least searching through every hit forktx2andzstddidn't show anything except loading an environment map in some examples), and they still show up in thecargo_featuresdoc as default features.Changelog
tonemapping_lutsfeature now includes both thektx2andzstdfeatures to avoid a panic when thetonemapping_lutsfeature was enable without both thektx2andzstdfeature enabled.