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.

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

Add Image methods for easy access to a pixel's color #10392

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

inodentry
Copy link
Contributor

Objective

If you want to draw / generate images from the CPU, such as:

  • to create procedurally-generated assets
  • for games whose artstyle is best implemented by poking pixels directly from the CPU, instead of using shaders

It is currently very unergonomic to do in Bevy, because you have to deal with the raw bytes inside image.data, take care of the pixel format, etc.

Solution

This PR adds some helper methods to Image for pixel manipulation. These methods allow you to use Bevy's user-friendly Color struct to read and write the colors of pixels, at arbitrary coordinates (specified as UVec3 to support any texture dimension). They handle encoding/decoding to the Images TextureFormat, incl. any sRGB conversion.

While we are at it, also add methods to help with direct access to the raw bytes. It is now easy to compute the offset where the bytes of a specific pixel coordinate are found, or to just get a Rust slice to access them.

Caveat: Color roundtrips are obviously going to be lossy for non-float TextureFormats. Using set_color_at followed by get_color_at will return a different value, due to the data conversions involved (such as f32 -> u8 -> f32 for the common Rgba8UnormSrgb texture format). Be careful when comparing colors (such as checking for a color you wrote before)!

Also adding a new example: cpu_draw (under 2d), to showcase these new APIs.


Changelog

Added

  • Image APIs for easy access to the colors of specific pixels.

@inodentry inodentry added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Nov 5, 2023
Copy link
Contributor

@killercup killercup left a comment

Choose a reason for hiding this comment

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

only a quick drive-by review :)

crates/bevy_render/src/texture/image.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/texture/image.rs Outdated Show resolved Hide resolved
@IceSentry
Copy link
Contributor

IceSentry commented Nov 19, 2023

I may be assuming some things, but I believe the vast majority of people using this will be in 2d. I would propose using 2d by default and having an explicit 3d variant. Something like get_color_at(pos_2d)/get_color_at_3d(pos_3d) I think most of the code could be reused so it wouldn't be too bad to maintain.

fn main() {
App::new()
.add_plugins(DefaultPlugins)
// Let's make the fixed timestep really fast for this example.
Copy link
Member

Choose a reason for hiding this comment

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

could you explain why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make an example that pokes pixels one at a time, at a fixed rate (not the display frame rate), to draw a spiral pattern. I felt like using bevy's fixed timestep, because it naturally lets me do things at a fixed rate idiomatically, instead of having to roll my own thing using Time. But doing it one pixel at a time makes the pattern emerge very slowly, and it is hard to see what's going on, when you run the example. So I decided to make it really fast. Bevy's fixed update will loop to make sure it runs enough times.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, in the example so that it's easier to use as a learning resource

Co-authored-by: François <[email protected]>
@inodentry
Copy link
Contributor Author

inodentry commented Nov 20, 2023

@IceSentry Good point. 2D images are the most common. Also, they are directly useful with regular Bevy sprites, UI, and materials, and don't require custom shaders.

3D is mostly used for fancy GPU techniques (such as volumetrics and GI) and it is far less likely someone would want to poke 3D texels from the CPU side (though I can envision use cases) instead of compute shaders or whatever.

1D is similarly mostly useful for custom shaders, to create lookup tables. Some use cases prefer textures instead of regular buffers, so they can do lookups with sampling/filtering. Again, niche use case. Though one where it might be more viable to set the data from the CPU side.

All of these use cases should be ergonomic. Now that you suggested it, I want to add helper methods for each dimension. The 2D/1D methods can internally just call the 3D method. It would be a nicer API to use.

That said, there is an argument to be made for having symmetry in the names:

  • set_color_at_1d
  • set_color_at_2d
  • set_color_at_3d

There is also an argument to be made (as you suggested), that 2D should omit the suffix, to have a shorter name, as it is the most common use case. But then the names are asymmetric / don't follow a consistent naming scheme.

We are getting into bikeshedding territory, but could we get a vote going? Please react with 🎉 for symmetric names and 🚀 for special-casing 2D.

@inodentry
Copy link
Contributor Author

OK, I changed the API so that there are separate (public) methods for each dimension, so that they can:

  • take the x,y,z coordinates directly, instead of UVec3, making them easier to use
  • validate the dimension, to make sure the user is accessing a texture with the dimension they think they are accessing

They call a (private) internal method, to avoid code duplication.

I named them according to the suggestion above, with 2D getting special treatment. 1D and 3D have a suffix in the name, 2D does not. 2D carries the full documentation. The others refer to it, as to not have to repeat all the details.

@inodentry
Copy link
Contributor Author

BTW, in case anyone wants to comment about the need for #[inline(always)] annotations, I'd like to preemptively explain my reasoning for adding them.

I have not done any benchmarks, but in my experience, functions such as these ones benefit a lot from inlining. They have a lot of branches whose code can be eliminated by the compiler, because they match on variables whose values are not going to change across many repeated calls to the same API. These functions are also likely to be called many times in tight loops. They also contain repetitive coordinate arithmetic that can mostly be optimized away by the compiler, if inlined.

I suspect that LLVM's heuristics would almost certainly automatically inline these functions anyway, if they were in the same codegen unit and it could see them. But because we are a library, and these functions are going to be used by users in their own crates, that would not be possible without LTO. Therefore, we need the inline annotation to tell rustc to make sure these functions can be inlined across crates.

@juh9870
Copy link

juh9870 commented Dec 2, 2023

When asset processing is enabled with basis_universalis, bevy by default compresses images, making it even harder to work with color data. Are there plans to add support for those compressed images? Or would we instead need to disable image processing in order to use these APIs to read colors data?

@inodentry
Copy link
Contributor Author

Support for compressed formats doesn't make sense. But I will answer your question anyway.

It is not really feasible to write pixels to such formats, because:

  • That would require re-encoding the entire block. Encoding these compressed formats is a very complicated and slow process. You don't want to ship a texture compressor with your game and you don't want to run it at runtime just to modify a pixel.
  • Even if it could be done, it would also affect the colors of other pixels in the block, and worsen compression artifacts / degrade quality.

In order to read pixels from such formats, one would need a BCn/ASTC/ETC2 decoder in software, to do the decoding operation on the CPU (that would normally be done by the GPU when sampling from the texture). This would add a lot of complexity. Reading a pixel is no longer just a simple memory access.

These formats are really not meant for access to individual pixels. Think of them as formats designed for GPU consumption only. They are native to the GPU hardware and allow you to save VRAM and improve performance (because the hardware can decompress them for free). They are effectively "opaque" to you as a developer for most practical purposes.

So yes, only process your images into compressed formats if you intend to just load them into GPU memory and access them from GPU rendering / shaders. If you are at all interested in accessing the data from the CPU side, don't use these formats.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Assuming CI is fixed LGTM (it just needs to specify RenderAssetUsages::all() in the Image::new_fill() and some missing docs in the example)

The example seems a bit over complicated IMO, but it's not a blocker. It's also a bit hard to see the pattern since the image is pretty small. Maybe increasing the image could help? It's very well documented so that's why I don't see it as a blocker, it just seems a bit complicated and the pattern from the math is not immediately obvious to me.

Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

It has more overhead, but I'm not fully convinced we shouldn't push users to use DynamicImage from the image crate instead.

@tychedelia
Copy link
Contributor

Am fine with this approach on second thought, and can see how it could be helpful for users who want to work with our color types.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 24, 2024
Copy link
Contributor

github-actions bot commented Oct 2, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@clarfonthey
Copy link
Contributor

clarfonthey commented Oct 2, 2024

Actually, never mind. They should both be aligned, so, it shouldn't make a difference in this case.


FWIW, I chose LCH for grayscale instead of OKLCH because it's actually aligned 100% with the Y luminance vector of the XYZ space, whereas OKLCH is just an approximation of it. If you want a correct grayscale then LCH is the one you want.

@clarfonthey
Copy link
Contributor

clarfonthey commented Oct 2, 2024

Also, this file is the one that has ne_bytes instead of le_bytes like I was mentioning: https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/texture/image_texture_conversion.rs

Can probably be a later change, but it should be fixed to always cast to little endian instead of the native endian.


I tried looking through the WGPU docs to see where it mentions the little-endian thing and had no luck. It appears that most recommendations use native endian, and unless this is just ignoring big-endian targets, it really feels like it just prefers using host endianness for the API and does the conversion on copy.

https://docs.rs/wgpu/latest/wgpu/struct.Buffer.html#example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

8 participants