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

Expose emulated *Unorm4x8 glsl functions in non-android builds #69521

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Dec 3, 2022

Fixes: #69489

Originally these functions were exposed on all GLSL ES 300 devices. However, that causes a build error as Android devices expose the *Unorm4x8 functions despite them not being in the ES 300 spec.

This is a pretty painful hack. In theory these functions shouldn't be exposed at all when targeting ES 300 so there should be no redefinition error.

I am a bit worried that this may be an Adreno-specific error. But I don't have any non-Adreno devices handy to test with so I can't confirm. We should merge this on the assumption that this is an Android specific issue. If we get the same issues on iOS devices we can expand to that. Or if this causes issues on Android non-Adreno devices, we can limit this to Adreno devices.

@clayjohn clayjohn added this to the 4.0 milestone Dec 3, 2022
@clayjohn clayjohn requested a review from a team as a code owner December 3, 2022 03:41
@clayjohn clayjohn changed the title Exposure emulated *Unorm4x8 glsl functions in non-android builds Expose emulated *Unorm4x8 glsl functions in non-android builds Dec 3, 2022
@akien-mga
Copy link
Member

akien-mga commented Dec 3, 2022

That's pretty weird. https://registry.khronos.org/OpenGL-Refpages/es3/html/packUnorm.xhtml indeed says that packUnorm4x8 and friends are defined in OpenGL ES SL 3.10.

Are we sure that we're not wrongly using OpenGL ES 3.1+ if available? Or could those be provided in ES SL 3.00 with an extension? That seems a pretty glaring issue if drivers were to provide ES SL 3.10 functions when we target ES SL 3.00 - the whole point of this shader language versioning should be to prevent this.

Also, isn't there a way in GLSL to query if the function is defined? So we wouldn't need to hardcode this but could do something like:

if !__has_builtin(packUnorm4x8)
...

Possibly relevant: https://bugs.chromium.org/p/angleproject/issues/detail?id=7527

Originally these functions were exposed on all GLSL ES 300 devices. However, that causes a build error as Android devices expose the *Unorm4x8 functions despite them not being in the ES 300 spec
@clayjohn
Copy link
Member Author

clayjohn commented Dec 4, 2022

@akien-mga Updated based on your comment.

I did a little bit more digging and testing on both an Adreno device and a Mali device. This is definitely an Adreno-specific bug. When version 300 es is used, only 300 es features should be exposed, but for some reason Adreno exposes these functions in the 300 es context. I looked for extensions and the closest one is ARB_shading_language_packing I believe it is not applicable to ES, but I tried disabling it anyway and it didn't help.

I also polled the GLSL version from both CPU and GPU using glGetString() and __VERSION__ respectively. glGetString() returns the highest supported version while __VERSION__ returns the version in use in that actual shader. Both my testing devices report support for up to version 320 and correctly report that the shader is compiled as a version 300 shader.

The version 320 spec guarantees support for version 300 shaders.

In the end, I went with your suggestion to define the functions with unique names as it doesn't appear there will be any way to work around this issue (short of disabling the functions when using an Adreno device).

I tested the above code on an Adreno device, a Mali device, and on Desktop GL and it appears to work fine for all three including calling the ***norm4x8() functions from user-shaders

@@ -332,7 +330,7 @@ vec4 light_shadow_compute(uint light_base, vec4 light_color, vec4 shadow_uv
shadow /= 13.0;
}

vec4 shadow_color = unpackUnorm4x8(light_array[light_base].shadow_color);
vec4 shadow_color = godot_unpackUnorm4x8(light_array[light_base].shadow_color);
Copy link
Member

Choose a reason for hiding this comment

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

Could likely keep using the unpackUnorm4x8 name since you #defined it as alias to our godot_ function.

Copy link
Member

Choose a reason for hiding this comment

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

As discussed on chat, this was kept like this on purpose to make it clear we're using emulated functions.

@akien-mga akien-mga merged commit 0697d6f into godotengine:master Dec 5, 2022
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Godot 4 beta 7 HTML5 export fails to render (missing unpackUnorm4x8)
2 participants