Change defaults for Tonemapping based on tonemapping_luts feature

[hukasu]

Objective

Fixes #20690

Solution

Have a different default for Tonemapping when tonemapping_luts is on and when it is off, and lock TonyMcMapface, BlenderFilmic and AgX behind feature

Testing

cargo run -p ci

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

[alice-i-cecile]

Good strategy, but please note the defaults in the docs.

[hukasu]

@alice-i-cecile should None be used as the default for when tonemapping_luts is off? there are a few modes that are still available with it off

[alice-i-cecile]

Yes, I think so?

[DGriffin91]

I think this makes sense as long as the tonemapping_luts feature is on by default.

Is this supposed to work in this pr?
cargo run --example tonemapping --no-default-features --features bevy_winit,bevy_pbr

I get a bunch of errors like

     | |_- associated item `TONEMAP_METHOD_TONY_MC_MAPFACE` not found for this struct
...
2503 |               } else if method == MeshPipelineKey::TONEMAP_METHOD_TONY_MC_MAPFACE {
     |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ associated item not found in `MeshPipelineKey`

[hukasu]

Should this MeshPipelineKeys be gated?

@DGriffin91

[hukasu]

i am moving forwards with the asumption that yes

[DGriffin91]

I'm not really sure of all the implications. Who/what all might depend on them existing. I would guess if they have the feature disabled they're being conscious about things like this missing. Not sure how it would affect plugins that expect them to be there or something. Might be better to resolve at runtime.

[SludgePhD]

It would at least be nice to use #[doc(cfg)] to make the fact that these are gated behind a feature apparent on docs.rs

[hukasu]

There are still some examples that use Tonemapping but i don't know if i should add tonemapping_luts to ALL of them

[JMS55]

Why not use ReinhardLuminance when the LUT feature is disabled? At least you then get some sort of tonemapping.

[alice-i-cecile]

Tonemapping can be very distressing for pixel artists: will this affect 2D workflows?

[DGriffin91]

It might be better that it's off so it's obvious that it's off.

[DGriffin91]

After thinking about this more I wonder if it's better for it to remain pink as it is in bevy main so it's blatantly obvious (along with the existing error message) that tonemapping is missing, requiring the user to resolve it.

[DGriffin91]

I'm wonder if using a feature to disable these in this way is a good fit. They are more of an asset and that kind of thing should generally be dealt with at run time. When there's an editor I would assume the situation around this kind of thing will have more clarity. I see the existing feature says:

Include tonemapping Look Up Tables KTX2 files. If everything is pink, you need to enable this feature or change the Tonemapping method for your Camera2d or Camera3d.

Would it work to only disable tonemapping on the camera if this feature is not set and not change anything else? Or maybe we shouldn't manipulate their component and just take a path further down the line that has no tonemapping. The comment for the feature being what it is implies there was a desire for this situation to be unavoidably obvious to the user and require them to explicitly resolve it (by changing the Tonemapping method Camera2d or Camera3d).
Having everything pink and an error stating the cause (as in current bevy main) seems to accomplish this well. Maybe the message could be updated to include something about seeing only pink.

[hukasu]

Would it work to only disable tonemapping on the camera if this feature is not set and not change anything else?

tonemapping_luts is only required for 3 of the tonemapping methods, all others work without LUTS