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

Jump to bottom

Add the required features for TonyMcMapface tonemapping to bevy_pbr #9073

Closed
wants to merge 1 commit into from
Closed

Add the required features for TonyMcMapface tonemapping to bevy_pbr #9073

wants to merge 1 commit into from

Conversation

Elabajaba
Copy link
Contributor

Objective

People keep running into issues with their materials being pink in 3rd party bevy libraries because we switched to TonyMcMapface for tonemapping, and they didn't realize they needed to include the ktx2, tonemapping_luts, and zstd features for it to work properly.

Solution

Add ktx2, tonemapping_luts, and zstd to the bevy_pbr feature.

Changelog

The bevy_pbr feature now includes the ktx2, tonemapping_luts, and zstd features.

Migration Guide

TODO

@Selene-Amanita Selene-Amanita added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 9, 2023
@superdump
Copy link
Contributor

Should be bevy_core_pipeline I think as tonemapping is used for both 2D and 3D, the node is in bevy_core_pipeline, and it is added into both core pipelines there.

@cart
Copy link
Member

cart commented Jul 9, 2023

Agreed!

@Elabajaba
Copy link
Contributor Author

Elabajaba commented Jul 9, 2023 •
edited
Loading

There's a few open questions from discord as to whether we even want to do this or not.

  1. This would mean that you could no longer use bevy_pbr (or bevy_core_pipeline) without the ktx2, tonemapping_luts, and zstd features. Do we want to make those features a required part of the renderer?
  2. Would doing that break anything we care about?
  3. edit: Is this even a big enough papercut to care about? The fix may just be putting it prominently in the migration guide that if you're disabling default features but using bevy_pbr (or 2d+tonemapping), then you just need to remember to add these features as well.

@mockersf mockersf mentioned this pull request Jul 10, 2023
Closed
@NotAFile NotAFile mentioned this pull request Jul 10, 2023
Merged
Vrixyz added a commit to Vrixyz/bevy_mod_raycast that referenced this pull request Jul 11, 2023
@Vrixyz
fix bright pink material
cc5da5b 
More info: bevyengine/bevy#9073
@nicopap
Copy link
Contributor

nicopap commented Jul 13, 2023

I think an alternative is the following:

Have two feature flags for bevy_pbr:

  1. bevy_pbr_minimal basically equals to bevy_pbr feature today, doesn't add the new dependencies. With this feature, Reinhard luminance should be used as default tonemapper (conditionally, more on this later)
  2. bevy_pbr this would add the required dependencies for TonnyMcMapface, and set TonnyMcMapface as default tonemapper unconditionally.

What happens if both features are enabled? Well, bevy_pbr prevails, it is strictly additive over bevy_pbr_minimal. The required dependencies for TonnyMcMapface are set and it itself is set as default

This allows 3rd party plugin crates to depend on bevy_pbr_minimal and users to pick between bevy_pbr_minimal and bevy_pbr at their discretion.

@cart
Copy link
Member

cart commented Jul 13, 2023

Yeah requiring those features for bevy_pbr does seem overly restrictive. I don't love having a separate "minimal" configuration. Seems like one more concept to contend with (and bucket of features to ensure work well together).

We could also just leave features as-is, detect when rendering with a fallback lut texture, and print an error message (with a pointer to the tonemapping_luts + ktx2 features and maybe a callout that they can change to reinhard). This is an error case.

@nicopap nicopap mentioned this pull request Jul 18, 2023
Closed
rs017991
rs017991 suggested changes Jul 18, 2023
View reviewed changes
Copy link

@rs017991 rs017991 left a comment

Choose a reason for hiding this comment

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

However this shakes out, please update cargo_features.md to make the interdependencies clear.

(Ref. #9179 for a newcomer's perspective on the surprising consequences given the current wording)

@Elabajaba Elabajaba mentioned this pull request Oct 25, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 27, 2023
@Elabajaba
Log a warning when the tonemapping_luts feature is disabled but req…
65b3ff1 
…uired for the selected tonemapper. (#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 (#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_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.
@JMS55
Copy link
Contributor

JMS55 commented Oct 27, 2023

Closed in favor of #10253

@JMS55 JMS55 closed this Oct 27, 2023
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
@Elabajaba @ameknite
Log a warning when the tonemapping_luts feature is disabled but req…
6a5197f 
…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
@Elabajaba
Log a warning when the tonemapping_luts feature is disabled but req…
6bdcbd1 
…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
Reviewers

@rs017991 rs017991 rs017991 requested changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Elabajaba @superdump @cart @nicopap @JMS55 @rs017991 @Selene-Amanita