Upstream bevy_color_blindness #5606

annieversary · 2022-08-07T16:59:14Z

This PR upstreams bevy_color_blindness, as discussed in #2724.

As mentioned in that issue, we would ideally have this be a submodule in bevy_render, in order for it to be well-integrated and not it's own segmented niche. Due to cyclic dependencies that's not possible, so for the time being I'm adding it as a top-level crate, pending further discussion.

I've also copied over the main example from bevy_color_blindness, under a new Accessibility category, as a temporary thing.

Changelog

Added bevy_color_blindness as a top-level crate.
Added examples/accessibility/color_blindness_simulation.rs.

DJMcNab

I'm not really in a position to judge if this is a good thing to actually have, but it can only be good to have that conversation

I'm by no means a rendering expert, but I've put in a conversation starter

crates/bevy_color_blindness/src/lib.rs

Cargo.toml

crates/bevy_color_blindness/src/lib.rs

alice-i-cecile · 2022-08-07T17:58:03Z

Lots of feedback, but to be clear I think that this is incredibly valuable, and a fantastic foundation. Feel free to push back if you think I'm wrong; I'm not a rendering expert.

MrGVSV · 2022-08-07T18:17:45Z

To avoid the list of bevy crates from growing too large, would it make sense to put this into a bevy_a11y crate or something? Then other upstreamed a11y helpers can be grouped in there as well.

bjorn3 · 2022-08-07T18:55:11Z

crates/bevy_color_blindness/src/lib.rs

+///
+/// [`UiCameraConfig`]: bevy_ui::entity::UiCameraConfig
+#[derive(Component)]
+pub struct ColorBlindnessCamera;


Maybe call this ColorBlindCamera? The camera itself is color blind and thus simulates how a color blind person would see the game.

I'm not sure how I feel about this. It sounds nicer, but I think having all types except one have the -ness suffix would lead to confusion.

alice-i-cecile · 2022-08-07T19:04:33Z

To avoid the list of bevy crates from growing too large, would it make sense to put this into a bevy_a11y crate or something? Then other upstreamed a11y helpers can be grouped in there as well.

As I said in the linked issue, I'd really like to avoid isolating accessibility features off on their own. Accessibility is a core requirement, not something to be tacked on at the end. Down the line, I think this probably belongs in something like bevy_postprocessing or bevy_render, but there are some circular dependency troubles that block that for an initial attempt.

Vrixyz · 2022-08-07T19:54:00Z

I was the author of the PR for the post-processing example, allow me to link a few relevant discussions which we might want to address:

Window resizing

Full screen triangle

#4797 (comment)

It's discussing 2 things:

Prefer a triangle to a quad
using a vertex shader to generate the mesh rather than a cpu bound one.

I'll keep an eye on this PR, I suspect this could lead to improve the post processing example 👍

Vrixyz · 2022-08-07T20:05:36Z

crates/bevy_color_blindness/src/color_blindness.wgsl

+@fragment
+fn fragment(
+    @builtin(position) position: vec4<f32>,
+    #import bevy_sprite::mesh2d_vertex_output
+) -> @location(0) vec4<f32> {
+    // Get screen position with coordinates from 0 to 1
+    let uv = position.xy / vec2<f32>(view.width, view.height);
+
+    var c = textureSample(texture, our_sampler, uv);
+
+    return vec4<f32>(
+        c.r * p.red.x + c.g * p.red.y + c.b * p.red.z,
+        c.r * p.green.x + c.g * p.green.y + c.b * p.green.z,
+        c.r * p.blue.x + c.g * p.blue.y + c.b * p.blue.z,
+        c.a
+    );
+}


I'd love to see an informed comparison with the approach linked in the associated issue: https://github.com/electronicarts/Tunable-Colorblindness-Solution

I translated into bevy if it can help to make a comparison : https://github.com/Vrixyz/bevy/blob/colorblind/assets/shaders/colorblind.wgsl

I'm not well-versed enough in the mess that is the world of colors to be able to provide any sort of comparison, much less an informed one '^^. Is there any Bevy contributor who does and would be able to help us with this?

crates/bevy_color_blindness/src/lib.rs

MrGVSV · 2022-08-07T22:58:39Z

As I said in the linked issue, I'd really like to avoid isolating accessibility features off on their own. Accessibility is a core requirement, not something to be tacked on at the end. Down the line, I think this probably belongs in something like bevy_postprocessing or bevy_render, but there are some circular dependency troubles that block that for an initial attempt.

Oh whoops didn't read the full thread haha. Sure that makes sense and it'd be really great if a11y was treated as a vital part to the crate(s) they affect. That being said, though, it might be worthwhile to group them into their own crate to act as an intermediate step before fully integrating them (if they need an intermediate step, integration isn't straightforward, dependency issues, etc.). Just to get them in quickly and worry about refactoring/integrating in the future. Or to be a place for tools/APIs that might not fit into any one crate otherwise.

But if this isn't actually as good an idea as I think it is I totally understand haha

inodentry · 2022-08-08T13:36:48Z

Regarding the question of "should we be splitting things into crates, or integrating into existing crates", I'd like to propose the following consideration: having a larger number of smaller crates is more scalable.

Typical bevy users already depend on bevy as a whole, and access everything via reexports or prelude. In practice, there isn't much difference to user convenience or how practical the features are to use, from them being split into more crates.

bevy_render is huge, and a big bottleneck in compile times (as sprite/ui/pbr/etc all depend on it). Let's avoid stuffing it with even more stuff if possible.

NiklasEi

The new crate should also be added to the publishing script tools/publish.sh.

Vrixyz · 2022-08-08T18:47:55Z

Depending on how long future discussions/feedbacks take, this PR might be a good candidate for bevy nursery bevyengine/rfcs#63

alice-i-cecile · 2022-08-09T00:55:56Z

Yep, this was one of the crates that prompted that discussion for me :)

This commit moves `mode` and `enabled` into `ColorBlindnessCamera`, which allows for multiple cameras with different modes. This should allow multiple windows to work correctly, but it hasn't been tested yet.

annieversary · 2022-08-14T18:09:12Z

These last commits fix a bunch of stuff that was previously mentioned:

ColorBlindnessCamera is now the only thing required (no more ColorBlindnessParams), which allows different cameras to have different modes.
Uses the camera's render target to decide the size for the quad. No more relying on the primary window.
Respects the camera's render target: the new post-processing camera will have the same target as the original camera.

All of this stuff should make multiple/no windows work, but I haven't tested it yet cause I'm not too familiar with how that works. The existing example continues working correctly.

Vrixyz · 2022-09-05T15:00:14Z

examples/accessibility/color_blindness_simulation.rs

+        });
+}
+
+fn change_mode(input: Res<Input<KeyCode>>, mut cameras: Query<&mut ColorBlindnessCamera>) {


minor nitpick, to avoid useless computations when input didn't change:

fn change_mode(input: Res<Input<KeyCode>>, mut cameras: Query<&mut ColorBlindnessCamera>) { if !input.is_changed() { return; }

Edit: as noted below, it's not useful.

I don't think this works - Input is always updated iirc?

indeed! Thanks for the correction

Weibye

This looks good to me and is a needed and necessary component to the engine as a whole :) I would be happy getting this in as-is.

mockersf · 2022-09-05T19:44:26Z

I don't think this should be included in Bevy. At least not yet.

It is a dev tool to simulate color blindness and should be part of an editor, not something needed in a game directly that would modify automatically the colors to accommodate for color blindness

alice-i-cecile · 2022-09-05T21:52:18Z

It is a dev tool to simulate color blindness and should be part of an editor

@mockersf has convinced me. @annieversary, hopefully this review process has been helpful to you (and your code upstream), and I very much welcome you to submit this as an editor tool once we're there!

Selene-Amanita · 2023-06-16T21:09:38Z

I'm not sure I'm understanding the reasoning to close this PR. I agree it is a dev tool, but I don't see why it should be linked to an editor?

Ideally, if I want to make a game and make sure that it's playable by people with various levels of color-blindness, I wouldn't just view my scenes through an editor and call it a day. If I have enough resources I would make colorblind people play it, but if I have just a handful of playtesters I would want to give some of them a colorblind emulation version of the game, or if I'm by myself I would like to fully play the game with this mode myself at least.

Shouldn't there maybe be a bevy_devtools that would have all the stuff you want to make sure you don't ship with the game at the end but you might want for dev, stuff like colorblindness emulation, DebugName, gizmos, wireframe, ... ?

Vrixyz · 2023-06-17T05:04:10Z

Another reason why this PR didn't make it is that the post processing method used was more a "hack" than "the good way", it's based on an old post-processing example. Now bevy main has an updated example, and I think an "official" plugin should use that updated code as its base, it supports resizing out of the box, and doesn't impact the world with arbitrary mesh.

annieversary added 2 commits August 7, 2022 17:45

Add bevy_color_blindness as a top level crate

7bcb661

Move example to new Accessibility category

e329e76

DJMcNab reviewed Aug 7, 2022

View reviewed changes

crates/bevy_color_blindness/src/lib.rs Outdated Show resolved Hide resolved

fix imports in doc examples

1160186

alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-Accessibility A problem that prevents users with disabilities from using Bevy labels Aug 7, 2022

alice-i-cecile reviewed Aug 7, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

run cargo run -p build-example-pages -- update

905f173