`Image` fields should not be public

[alice-i-cecile]

What problem does this solve or what need does it fill?

The Image type has quite a few complex invariants to uphold, around data layout and byte intepretation.

As a result, it has several nice constructors, which do the math for you.

However, the fields are fully public, and can be constructed in or modified to be invalid with no protections.

What solution would you like?

All fields should be private, and getters and setters should be added, which should generally return error types.

What alternative(s) have you considered?

Leave it alone: maybe the rendering people have good reasons to do cursed transmute-style type punning?

Additional context

We might also want to add non-panicking variants to the various panicking methods at the same time.

[clarfonthey]

So, I poked around for existing issues, and this one seemed the closest to the one that fits, so, I figured I'd piggyback on the discussion here before potentially moving to a new issue if that feels more appropriate. I shared a bit on Discord but an issue is probably the better forum for discussing this.

I specifically approached this from the perspective of having an Image type that's robust enough to be useful for generating texture atlases, so, that's probably worth keeping in mind. The operations you need to generate atlases are substantially smaller than the operations you need to generate arbitrary images, and it's mostly limited to just copying rects back and forth. We can offer as many APIs as we need to access the data, however, since only the size of the buffer is the invariant we need to preserve here.

Basically, it feels like Image is trying to do too many things at once, and that's part of the reason why its fields are kind of out of control. It wants to provide access to image data that can be modified on the CPU, but it also contains all of the data it needs to be turned into a GPU texture, despite not being on the GPU yet.

One huge issue here is that a lot of this GPU information is totally irrelevant to the actual image data. For example, Rgba8Unorm and Rgba8Uint are literally identical pixel formats with the only difference being how the data is passed to shaders. This information makes CPU manipulation especially difficult since you have to deal with this massive amount of variants in texture formats when in reality, these just get in the way of making sure the image data is valid.

My thought process was that the best solution here would be to totally separate the Image data out into a separate buffer type. This buffer would only contain the raw data, the size of the image, and enough information about the format to validate that the buffer, and that's it. The current Image type can then contain that buffer type alongside all the extra data the Image has now, and perform the more complicated validation of ensuring those match. TextureDescriptor unfortunately duplicates everything, but maybe longer-term we can just store the stuff that isn't duplicated and make one by hand.

Another thing I wanted to do is add some level of type safety to the byte buffer, making it instead a buffer of floats or larger integers so those values can be more easily passed in directly. It also ensures alignment, which isn't necessarily important but feels nice.

So, this is the first draft of that I made: https://codeberg.org/clarfonthey/bevy-image-sketch

Note that most of the API is still missing and I just wanted to get the basic structure down since that affects everything else. In particular, it abuses the terms "component" and "block" in a way that makes laying out and validating the data easier, at the cost of not actually meaning what it usually means when people use those terms.

A few basic design principles:

• Our base buffer is treated, always, as an array of 2D images. Whether this array is interpreted as a 3D image doesn't really matter, since they have the same layout in memory. Similarly, 1D textures are rare enough that they can just be treated as Nx1 2D textures. Zero images in the buffer is allowed so the buffer can be empty initially.
• Each 2D image contains some number of planes, although each of those planes is guaranteed to be the same number of blocks. For block-compressed textures, these blocks are actual compressed blocks, but we can also use them to represent formats like NV12, where a 2x2 block of pixels is represented with less data on the UV plane than the Y plane, or mipmaps, where some NxN block of pixels is subsampled down to smaller levels of precision.
• Each plane says how many components, i.e. data units, it uses in each block. This allows for the difference in sampling.

We'd still want a bit more information, namely:

• If we're converting between formats, that requires actually assigning a name to the format and a conversion method. For example, whether a byte is sRGB-scaled doesn't matter for the buffer, but it does matter for format conversion.
• If we want to allow more than just copying between rects of blocks, we'd need to have some way of mutating the individual blocks directly.

But the idea was that this could act as a minimal amount of information that we'd need to allow texture atlases to copy images safely, even for block-compressed formats, multiplane formats, or mipmaps.

There's also the massive unanswered question of, how much information on the image is actually necessary, or could it simply be moved into some other component? Right now assets don't really have components, but we could potentially make multiple versions of Image that increase in complexity, and that could solve some issues, potentially.

Mostly just wanted to conceptualise the shape of this problem in my head and this is how I did it.