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

Rendering silently does not work without "tonemapping_luts" feature #9098

Closed
NotAFile opened this issue Jul 10, 2023 · 3 comments
Closed

Rendering silently does not work without "tonemapping_luts" feature #9098

NotAFile opened this issue Jul 10, 2023 · 3 comments
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Milestone
0.12

Comments

@NotAFile
Copy link
Contributor

Bevy version

v0.11.0

[Optional] Relevant system information

`AdapterInfo { name: "AMD Radeon RX 470 Graphics (RADV POLARIS10)", vendor: 4098, device: 26591, device_type: DiscreteGpu, driver: "radv", driver_info: "Mesa 23.1.3", backend: Vulkan }`

What you did

Run the "3d_scene.rs" example under 0.11.0 without the "tonemapping_luts" feature enabled

What went wrong

The materials display as FF00FF with no error, log, crash, or other hint as to indicate what is going wrong.

image

Additional information

Presumably with the switch to TonyMcMapface, this feature is now required by default, exposing some untested error paths.
@NotAFile NotAFile added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jul 10, 2023
@mockersf
Copy link
Member

fixed by #9073

@mockersf mockersf added A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Jul 10, 2023
@NotAFile
Copy link
Contributor Author

Even when that is fixed, having some sort of error when not being able to load tonemaps might be a good idea?

@yyogo yyogo mentioned this issue Jul 12, 2023
Closed
@nicopap nicopap mentioned this issue Jul 31, 2023
Closed
@Selene-Amanita Selene-Amanita added this to the 0.12 milestone Aug 15, 2023
@Elabajaba Elabajaba mentioned this issue Oct 25, 2023
Merged
github-merge-queue bot pushed a commit that referenced this issue 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

Fixed by #10253

@JMS55 JMS55 closed this as completed Oct 27, 2023
ameknite pushed a commit to ameknite/bevy that referenced this issue 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 issue 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
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
0.12
Development

No branches or pull requests
4 participants
@NotAFile @mockersf @JMS55 @Selene-Amanita