馃搹 Add renormalization based on pixel format to Lanczos kernel #48
Conversation
src/ispc/kernels/lanczos3.ispc
Outdated
| if (is_snorm(pixel_format)) { | ||
| // Assumption: Snorm formats are only used for normal maps, so we normalize them to preserve vector length. | ||
| color = normalize(color); | ||
| color = color * 0.5f + 0.5f; |
There was a problem hiding this comment.
I suppose the idea here is to ensure the range stays within 0..1? Since the source is already always in that range, why do we still do this?
src/ispc/kernels/lanczos3.ispc
Outdated
| // The final color is a sum of numbers that are multiplied by the weights of their respective pixels. | ||
| // Because of their numbers, floating point precision leads to the final color being potentially outside of the 0-255 range by a slight margin. | ||
| // This would cause an underflow/overflow, which we avoid with the clamps. |
There was a problem hiding this comment.
I think this comment only applies to the unorm branch, should probably be moved
src/ispc/kernels/lanczos3.ispc
Outdated
| pixel_ptr[2] = clamp(color[2], 0.0f, 255.0f); | ||
| } | ||
| if (is_snorm(pixel_format)) { | ||
| // Assumption: Snorm formats are only used for normal maps, so we normalize them to preserve vector length. |
There was a problem hiding this comment.
This assumption is probably incorrect. I would make it explicit from the name that were inputting normalmaps
src/ispc/kernels/lanczos3.ispc
Outdated
| if (is_snorm(pixel_format)) { | ||
| // Assumption: Snorm formats are only used for normal maps, so we normalize them to preserve vector length. | ||
| color = normalize(color); | ||
| color = color * 0.5f + 0.5f; |
There was a problem hiding this comment.
I can't find the code that does the reverse when unpacking, also we probably need to know the space of the normal map here (typically tangent space, and we only have 2 components). I don't think this is correct what's happening here.
There was a problem hiding this comment.
Requesting changes on this.
- Unorm vs snorm is not the right nomenclature for this. They just indicate value ranges, not intended use
- We shouldn't lanczos filter our normal maps as discussed offline. Lanczos filters can result in negative values even if all it's inputs are positive, resulting in incorrect normals (for regular images would result perceived sharpness).
- This doesn't appear to take into account the type of normal map, and so most likely it's incorrect reconstructing the normals. Notice that we use tangent space normals for which we reconstruct one of the components.
- It appears this removes srgb support as well? We're we ignoring that during downsampling? If so, we probably need a follow up also to adresss this issue, for more info see http://www.ericbrasseur.org/gamma.html?i=1
I've implemented a different path that can be taken by the user which applies a box filter, as discussed offline. This means that the format is not connected to the way the downsampling is done, and instead the user must explicitly say that it is a normal map.
I am not sure why non-tangent space normals would require a special treatment. At lower mips there will be noticeable mistakes in the normal map, but I believe that would be due to the fact that we only have a few pixels to represent any direction for the normals.
I don't think there was anything for SRGB textures before this, unless something was lost with #15 getting merged. I can investigate this and if that is the case create a separate PR about this afterwards. I don't think it belongs in this PR. |
You'd need to decode the values, normalize them, and encode them again in the proper format. For example for our tangent space normals, we store them in R & G and then use the following routine to decode them; If you'd just take the texture data's rgb value and normalize that you wouldn't end up with the correct normal in the first place. |
Ok, this makes sense now. I will add something so that it can be indicated whether the normal needs to be decoded, however I would like to point out that our framework does not downsample |
According to [GitHub documentation] it is now possible to get a Mac M1 runner using the aarch64 architecture. This means `x86_64` is no longer the only host architecture for which we expect the `x86_64-apple-darwin` target to always be natively installed. Install it unconditionally next to the `aarch64` Mac/iOS triples and let `rustup` figure out and skip what's already installed. [GitHub documentation]: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners
KYovchevski
force-pushed
the
renormalization
branch
from
c1522a9
to
816733f
Compare
KYovchevski
force-pushed
the
renormalization
branch
from
d99ecad
to
c6e6d98
Compare
| ) -> Vec<u8> { | ||
| assert!( | ||
| matches!(src.format, Format::Rgba8), | ||
| matches!(format, Format::Rgba8Unorm | Format::Rgba8Snorm), |
When downsampling normal maps we need to renormalize the values to get a correct normal map.
Unfortunately, we can't turn it into a separate pass unless we split the entire ISPCprogram up, since the program does two passes, one vertical and one horizontal, and we should be normalizing after both of them for complete correctness.
As a result, the renormalization was made a step of the writing process for the pixels, which is only ran for Snorm formats. To accommodate this,
Rgba8SnormandRgb8Snormformats were added, while previous formats were renamed to*Unorm. The formats were also reflected to the ISPC code so that we can make decisions in the program and not rely on having many more different functions that the Rust side needs to call correctly.