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 check for float texture linear filtering support #102089

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

adamscott
Copy link
Member

@adamscott adamscott commented Jan 27, 2025

This PR makes it so that, on the web platform, the curve texture is rendered using an int- half-float-based image format instead of a float based one.

Thanks to @clayjohn #102089 (comment), this PR now adds a check for float texture linear filtering support.

Fixes #102088

@Calinou
Copy link
Member

Calinou commented Jan 27, 2025

Did you try RGBH/RH instead? Using RGB8/R8 means we lose all HDR data and have much lower precision, which can be problematic for some use cases.

@adamscott adamscott force-pushed the fix-curve-texture-web branch from 5f06cc4 to 7fb5459 Compare January 27, 2025 18:26
@adamscott
Copy link
Member Author

Did you try RGBH/RH instead? Using RGB8/R8 means we lose all HDR data and have much lower precision, which can be problematic for some use cases.

@Calinou Fixed! And it works! And I greatly simplified the code, didn't know that Image::convert() existed.

@adamscott adamscott force-pushed the fix-curve-texture-web branch 2 times, most recently from c6f1fca to 5998986 Compare January 27, 2025 18:28
@adamscott adamscott changed the title [Web] Make CurveTexture use int-based image format [Web] Make CurveTexture use half-float-based image format Jan 27, 2025
@clayjohn
Copy link
Member

clayjohn commented Jan 27, 2025

Is this an iOS specific problem? AFAIK RGB32F textures should be supported by all WebGL implementations

edit: Just double checked, RGB32F is supported by WebGL 2.0 https://registry.khronos.org/webgl/specs/latest/2.0/. Therefore I suspect this is more a problem of safari on iOS not being compliant with WebGL 2

OOOOhhhh, According to https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/texImage2D the floating point formats are supported, but may not be texture filterable! Could you test on iOS with the linear filtering disabled? Edit: and that's confirmed by the OpenGL ES 3.0 spec https://registry.khronos.org/OpenGL/specs/es/3.0/es_spec_3.0.pdf

Okay, so, ideally what we would do is check if an implementation supports using the linear filter with a given texture, then convert the texture automatically. This is how we handle compressed textures:

case Image::FORMAT_DXT1: {
if (config->s3tc_supported) {
r_gl_internal_format = _EXT_COMPRESSED_RGBA_S3TC_DXT1_EXT;
r_gl_format = GL_RGBA;
r_gl_type = GL_UNSIGNED_BYTE;
r_compressed = true;
} else {
need_decompress = true;
}
} break;

And here is what we did in 3.6 for GLES2 where float textures are only supported with an extension:

if (!config.float_texture_supported) {
ERR_PRINT("RGBA float texture not supported, converting to RGBA8.");
if (image.is_valid()) {
image->convert(Image::FORMAT_RGBA8);
}
r_real_format = Image::FORMAT_RGBA8;
r_gl_internal_format = GL_RGBA;
r_gl_format = GL_RGBA;
r_gl_type = GL_UNSIGNED_BYTE;
} else {
r_gl_internal_format = GL_RGBA;
r_gl_format = GL_RGBA;
r_gl_type = GL_FLOAT;
}

@clayjohn
Copy link
Member

clayjohn commented Jan 27, 2025

I found the missing piece!

We need to check if the OES_texture_float_linear extension is supported.

It is basically supported everywhere except iOS: https://caniuse.com/?search=OES_texture_float_linear

edit: Aaaaaaannd looks like we handled this correctly in 3.x

config.texture_float_linear_supported = config.extensions.has("GL_OES_texture_float_linear");

So, to summarize. We need to add config.texture_float_linear_supported = config.extensions.has("GL_OES_texture_float_linear"); to config.cpp. Then we need to check it when creating the texture in texture_storage.cpp. If unsupported we can just fall back to RGB16H, i.e:

This:

case Image::FORMAT_RGBF: {
r_gl_internal_format = GL_RGB32F;
r_gl_format = GL_RGB;
r_gl_type = GL_FLOAT;
} break;

Becomes:

 case Image::FORMAT_RGBF: { 
 	if (config.texture_float_linear_supported) {
 	 	r_gl_internal_format = GL_RGB32F; 
 	 	r_gl_format = GL_RGB; 
 	 	r_gl_type = GL_FLOAT; 
 	} else {
 	 	ERR_PRINT("RGB32 float texture not supported, converting to RGB16."); 
 	 	if (image.is_valid()) { 
 	 		image->convert(Image::FORMAT_RGBH); 
 	 	} 
 	 	r_real_format = Image::FORMAT_RGBH; 
 	 	r_gl_internal_format = GL_RGB16F; 
 	 	r_gl_format = GL_RGB; 
 	 	r_gl_type = GL_HALF_FLOAT;
 	}
 } break; 

@adamscott adamscott force-pushed the fix-curve-texture-web branch from 5998986 to 1b7afcf Compare January 28, 2025 01:16
@adamscott adamscott requested a review from a team as a code owner January 28, 2025 01:16
@adamscott adamscott changed the title [Web] Make CurveTexture use half-float-based image format Add check for float texture linear filtering support Jan 28, 2025
@adamscott
Copy link
Member Author

@clayjohn It works!

IMG_0080

@adamscott adamscott force-pushed the fix-curve-texture-web branch from 1b7afcf to 7033606 Compare January 28, 2025 01:36
@adamscott adamscott force-pushed the fix-curve-texture-web branch from 7033606 to 0bcb79d Compare January 28, 2025 03:11
Co-authored-by: Clay John <claynjohn@gmail.com>
@adamscott adamscott force-pushed the fix-curve-texture-web branch from 0bcb79d to 6091317 Compare January 28, 2025 03:12
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great! This might even fix a few other iOS issues

r_gl_format = GL_RED;
r_gl_type = GL_FLOAT;
} else {
ERR_PRINT("R32 float texture not supported, converting to R16.");
Copy link
Member

Choose a reason for hiding this comment

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

ERR_PRINT() might be a bit spammy in projects using lots of CurveTextures, so maybe we should use WARN_PRINT_ONCE() instead. The condition can't change at runtime anyway – if float textures aren't supported when the project starts, they won't be supported afterwards in the same session.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can change this in a followup PR if it ends up being too intrusive

@Repiteo Repiteo merged commit 808c25f into godotengine:master Jan 28, 2025
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 28, 2025

Thanks!

@adamscott adamscott added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release platform:ios platform:web topic:gui topic:porting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CurveTexture doesn't render properly on iOS Safari
4 participants