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

Cubemap + WebGL float type fixes #7625

Merged
merged 9 commits into from May 8, 2023
Merged

Cubemap + WebGL float type fixes #7625

merged 9 commits into from May 8, 2023

Conversation

Jhonnyg
Copy link
Contributor

@Jhonnyg Jhonnyg commented May 2, 2023

Fixed several issues with creating cubemap textures with float types. Creating cubemaps from scripts now also works correctly for all graphics adapters.

@@ -154,63 +154,63 @@

#if defined(GL_RGB32F_EXT)
#define DMGRAPHICS_TEXTURE_FORMAT_RGB32F (GL_RGB32F_EXT)
#elif defined(GL_RGB32F) && !defined (__EMSCRIPTEN__)
#elif defined(GL_RGB32F)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are all core now on WebGL2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And for WebGL1 we have the fallback to the "correct" float types for that context.

Comment on lines 2872 to 2876
#ifdef __EMSCRIPTEN__
#define WEBGL1_WORKAROUND(fmt) \
if (!context->m_IsGles3Version) \
{ \
gl_internal_format = fmt; \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For backwards compatability we need to do a workaround for WebGL1 here to use the "old" formats. We can't rely on compile time constants since they differ between the context versions..

@@ -176,6 +176,7 @@
#define glUnmapBufferARB glUnmapBufferOES
#define GL_ARRAY_BUFFER_ARB GL_ARRAY_BUFFER
#define GL_ELEMENT_ARRAY_BUFFER_ARB GL_ELEMENT_ARRAY_BUFFER
#define GL_TEXTURE_CUBE_MAP_SEAMLESS 0x884F
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ES3 enum

Comment on lines +1282 to +1283
glEnable(GL_TEXTURE_CUBE_MAP_SEAMLESS);
CLEAR_GL_ERROR;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't guarantee that enabling seamless should work, for "most" platforms it's on by default if we use a GL3+ context so for debug builds to work if this flag DOESNT work we need to clear the errors. It's fine if we don't get seamless sampling for the platforms that don't support it since it's at least not worse than it is now where we don't have use this flag at all..

Comment on lines +2876 to +2885
#ifdef __EMSCRIPTEN__
#define EMSCRIPTEN_ES2_BACKWARDS_COMPAT(var, value) ES2_ENUM_WORKAROUND(var, value)
#else
#define EMSCRIPTEN_ES2_BACKWARDS_COMPAT(var, value)
#endif
#ifdef __ANDROID__
#define ANDROID_ES2_BACKWARDS_COMPAT(var, value) ES2_ENUM_WORKAROUND(var, value)
#else
#define ANDROID_ES2_BACKWARDS_COMPAT(var, value)
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit awkward: webgl 2 requires other enums than webgl1 when creating certain textures, and android as well but different types depending on if we are using ES2 or ES3 context.. Since we use the same headers for both ES2 and ES3 for those platforms we need to be able to use different enums..

#undef ANDROID_ES2_BACKWARDS_COMPAT
}

static void OpenGLSetTexture(HTexture texture, const TextureParams& params)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a "big" change, but it's just that I pulled out the type / format switch into a separate function

#define DMGRAPHICS_TYPE_HALF_FLOAT (GL_HALF_FLOAT_OES)
#else

#ifdef GL_HALF_FLOAT
#define DMGRAPHICS_TYPE_HALF_FLOAT (GL_HALF_FLOAT)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For android this enum will always exist, but it's not the correct one to use if we run on a es3 context..

@Jhonnyg Jhonnyg requested a review from JCash May 5, 2023 16:24
@Jhonnyg Jhonnyg added bug Something is not working as expected engine Issues related to the Defold engine labels May 5, 2023
@Jhonnyg Jhonnyg marked this pull request as ready for review May 6, 2023 08:51
JCash
JCash previously approved these changes May 8, 2023
@@ -903,7 +904,7 @@ namespace dmGraphics

selected_device = device;
selected_queue_family = queue_family;
break;
break; // Why do we break here? x_x
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess because we're happy with the device we've found?
Are we not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, lol I'm a dummy. Yes we are, I thought we didn't loop through all devices but I was probably drunk tired when I read the code

@Jhonnyg Jhonnyg merged commit 1e43cf7 into dev May 8, 2023
3 of 13 checks passed
@Jhonnyg Jhonnyg deleted the cubemap-fixes branch May 8, 2023 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working as expected engine Issues related to the Defold engine
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants