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 support for some signed texture formats #1892

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oltolm
Copy link

@oltolm oltolm commented Jan 30, 2025

Even though we can not support the general case, we can easily support the case when all components are signed. See #587.

@oltolm
Copy link
Author

oltolm commented Feb 10, 2025

@abaire Sorry for the ping, but can you take a look?

@mborgerson
Copy link
Member

FYI while glancing at @coldhex's development branch in regards to #1895 and #1896, I noticed they also had been working on signed texture support. I haven't reviewed their code yet, but we should coordinate here to avoid duplicate efforts.

@oltolm
Copy link
Author

oltolm commented Feb 10, 2025

Sure, I am all for it.

@oltolm
Copy link
Author

oltolm commented Feb 15, 2025

I think this PR is useful regardless of other PRs because it solves the problem in the specific case when all components are signed. It does not conflict with other more general solutions and can coexist with them.

@oltolm
Copy link
Author

oltolm commented Mar 15, 2025

@coldhex Can you take a look?

@@ -319,4 +330,12 @@ static const SurfaceFormatInfo kelvin_surface_zeta_fixed_format_gl_map[] = {
{4, GL_DEPTH24_STENCIL8, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8, GL_DEPTH_STENCIL_ATTACHMENT},
};

static const uint32_t kelvin_signed_format_mask_gl_map[66] = {
[NV097_SET_TEXTURE_FORMAT_COLOR_SZ_R6G5B5] =
NV_PGRAPH_TEXFILTER0_RSIGNED | NV_PGRAPH_TEXFILTER0_GSIGNED | NV_PGRAPH_TEXFILTER0_BSIGNED,
Copy link

Choose a reason for hiding this comment

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

R might usually be unsigned. See also the texture data conversion done in, which should change depending on if signs are set or not:

} else if (s.color_format == NV097_SET_TEXTURE_FORMAT_COLOR_SZ_R6G5B5) {
assert(depth == 1); /* FIXME */
size = width * height * 3;
converted_data = g_malloc(size);
for (int y = 0; y < height; y++) {
for (int x = 0; x < width; x++) {
uint16_t rgb655 = *(uint16_t *)(data + y * row_pitch + x * 2);
int8_t *pixel = (int8_t *)&converted_data[(y * width + x) * 3];
/* Maps 5 bit G and B signed value range to 8 bit
* signed values. R is probably unsigned.
*/
rgb655 ^= (1 << 9) | (1 << 4);
pixel[0] = ((rgb655 & 0xFC00) >> 10) * 0x7F / 0x3F;
pixel[1] = ((rgb655 & 0x03E0) >> 5) * 0xFF / 0x1F - 0x80;
pixel[2] = (rgb655 & 0x001F) * 0xFF / 0x1F - 0x80;
}
}
} else {

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand what this code does, I can't do it.

{
PGRAPHState *pg = &d->pgraph;

const ColorFormatInfo *f = &kelvin_color_format_gl_map[texture_shape->color_format];
ColorFormatInfo f = kelvin_color_format_gl_map[texture_shape->color_format];
if (filter & kelvin_signed_format_mask_gl_map[texture_shape->color_format])
Copy link

Choose a reason for hiding this comment

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

Doesn't this only test if one of the sign bits match, not all?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I fixed it.

@coldhex
Copy link

coldhex commented Mar 15, 2025

I also considered a solution similar to this PR, but then went with implementing a general solution in my dev branch. While this PR should improve water effect in e.g. Halo 2, I have no idea if it might introduce artifacts in other games. To be safe, even if B8G8 or R8B8 texture are used, all sign bits R, G, B, A should be set if SNORM texture is used (because for e.g. B8G8 texture, hardware uses data from B and G channels also for R and A channels, but the sign setting is independent for each channel.) However, requiring all channels RGBA to have the sign bits wouldn't fix Halo 2 water, because Halo 2 doesn't set them all, and therefore is really not an option.

(And as far as my dev branch solution is concerced, it works fine, but I am still considering implementing signed texture handling in a different way. At some point I'm going to try a solution that does bilinear (or trilinear in case of mip-mapping) texture filtering manually in the fragment shader. This would avoid the need to adjust texture channel data sign bits before uploading to GPU.)

@oltolm
Copy link
Author

oltolm commented Apr 3, 2025

I don't know if makes any game worse, but it fixes the water in Ninja Gaiden 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants