馃 #48 API cleanup #53
Conversation
There was a problem hiding this comment.
I think these changes look good. Unfortunately I can't find the original feedback by @MarijnS95 that requested these changes, so I can't really know whether all concenrs were addressed, but otherwise this seems fine to go.
KYovchevski
force-pushed
the
image-pixel-format
branch
2 times, most recently
from
3222d61
to
70c318c
Compare
KYovchevski
force-pushed
the
image-pixel-format
branch
from
70c318c
to
7d5f4e6
Compare
| Format::Rgba8Unorm | ||
| AlbedoFormat::Rgba8Unorm | ||
| } else { | ||
| Format::Rgb8Unorm | ||
| AlbedoFormat::Rgb8Unorm |
There was a problem hiding this comment.
Shouldn't this use your Srgb format?
| Format::Rgba8Unorm | ||
| AlbedoFormat::Rgba8Unorm | ||
| } else { | ||
| Format::Rgb8Unorm | ||
| AlbedoFormat::Rgb8Unorm |
There was a problem hiding this comment.
Image crate outputs Srgb, even though we don't action on it yet.
There was a problem hiding this comment.
The Srgb8 is not treated differently from the Rgb8 formats.
| pub trait ImagePixelFormat { | ||
| fn num_channel_in_memory(&self) -> usize; | ||
| fn channel_size_in_bytes(&self) -> usize; | ||
|
|
||
| fn pixel_size(&self) -> usize { | ||
| self.channel_size_in_bytes() * self.num_channel_in_memory() | ||
| } |
There was a problem hiding this comment.
Doc-comments? I'll continue reading the PR, but already have to confess that I don't understand what num_channel_in_memory() is supposed to return from just this definition.
There was a problem hiding this comment.
For API-cleanliness, maybe the trait could require : Copy, and pass self by value.
| width: u32, | ||
| height: u32, | ||
| format: F, | ||
| pixel_stride_in_bytes: usize, |
There was a problem hiding this comment.
I guess this is to skip pixels without a meaning, i.e. an (s)rgb(a)8 loaded PNG where only the RG channels are valid for an encoded normal? Could we achieve the same by having the input texture be a plain old (s)rgb(a)8 format, and the output an r8g8normal thing however it is called?
Just thinking out loud what could happen with ambiguity between Image::format and Image::pixel_stride_in_bytes.
There was a problem hiding this comment.
We have an exact case like this in Breda, which is the normal maps there. Our pipeline pads all textures to 4 channels, normal maps included, so we need to skip the 4th channel when downsampling then.
pixel_stride_in_bytes is meant as an override of the stride that the format would imply. I think having differing input and output formats makes this much more difficult than being able to specify the stride, while also limiting us more.
|
Github didn't notify me of Marijn's comments until after I had clicked squash and merge. Thanks Github. |
In #48 we added an alternative downsampling path specifically for normal maps which renormalizes them and allows for format that have the Z-component reconstructed from X and Y. Also, due to our app using 4-channel texture for the normal maps during asset building, we needed to introduce a pixel stride parameter so we can correctly sample the normal maps.
This lead to several API changes which, in hindsight, were not done the best, such as exposing a mandatory pixel stride parameter for the downsampling function and the format getting separated from the
Imagestruct because of the existence of two different format enums.This PR attempts to correct these issues by doing a few things:
trait ImagePixelFormatis added which establishes an interface that bothNormalMapFormatandAlbedoFormat(Previously named onlyFormat) need to fulfil.struct Imagenow uses a genericF: ImagePixelFormatso that the struct can be reused for both regular images and for normal maps.Imagestruct. UsingImage::new()will assume the stride is the same as the Format's pixel side, while a newly addedImage::new_with_pixel_stride()allows a specific value to be given to the stride.Imageinstead.downsample_normal_maprequiresImageto use aNormalMapFormat, whiledownsampleandscale_alpha_to_original_coveragerequireAlbedoFormatto be used instead.