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

Disable *glGetProcAddress() on the web #93489

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Jun 22, 2024

In PR #87981, we added the Emscripten flag (supported in 3.1.51 or later) to enable glGetProcAddress() on the web.

However, we shouldn't need it (since the GL functions are "statically linked" on the web) and it's known to be inefficient.

See these references from the Emscripten docs:

This PR explicitly disables the feature (rather than just reverting PR #87981) since the docs say it's enabled by default - since that didn't seem to be the case originally, either the docs are wrong or the default may have changed?

Marking as draft because I haven't had much time to test it yet. It succeeds in compiling and seems to work, but I'd like to be more sure that everything is good. :-)

@adamscott
Copy link
Member

Sorry, your comments on #87981 fell in one of my cracks. 😬

Seems good. I think I added the functionality because it was a breaking change otherwise. But if it works without, then everything is good.

@adamscott
Copy link
Member

What about this file? How can we make sure that this either doesn't get run or replace it with something else for the Web?

#ifdef GLES_API_ENABLED
if (!gles_over_gl) {
if (OS::get_singleton()->is_stdout_verbose()) {
DebugMessageCallbackARB callback = (DebugMessageCallbackARB)eglGetProcAddress("glDebugMessageCallback");
if (!callback) {
callback = (DebugMessageCallbackARB)eglGetProcAddress("glDebugMessageCallbackKHR");
}
if (callback) {
print_line("godot: ENABLING GL DEBUG");
glEnable(_EXT_DEBUG_OUTPUT_SYNCHRONOUS_ARB);
callback((DEBUGPROCARB)_gl_debug_print, nullptr);
glEnable(_EXT_DEBUG_OUTPUT);
}
}
}

Do you have an idea, @bruvzg?

@dsnopek
Copy link
Contributor Author

dsnopek commented Jun 24, 2024

What about this file? How can we make sure that this either doesn't get run or replace it with something else for the Web?

On the web, I think we should be able to just #include <GLES3/glext.h> and call glDebugMessageCallback() directly, without having to load the function pointer first, but I haven't tested it.

Assuming that works, we could use #ifdef WEB_ENABLED to do that? Or, try to bake it into platform/web/pltaform_gl.h somehow?

@dsnopek
Copy link
Contributor Author

dsnopek commented Jun 24, 2024

Or, actually, maybe we don't need to do anything to support that at all?

All that does is print messages to the console when there are OpenGL errors, but WebGL does that all on its own.

So, maybe we just need to wrap that code in #ifndef WEB_ENABLED?

Anyway, I'll do some testing later when I have a chance.

@dsnopek dsnopek force-pushed the web-disable-getprocaddress branch from e7b46f6 to 8e242fe Compare June 24, 2024 15:57
@dsnopek
Copy link
Contributor Author

dsnopek commented Jun 24, 2024

@adamscott It looks like that code is already excluded on web builds. As I also just commented here, that code is wrapped in #ifdef CAN_DEBUG, which won't be defined on the web.

In my latest push on this PR, I've added some #defines to platform/web/platform_gl.h that make it a compiler error to use glGetProcAddress() or eglGetProcAddress() on the web, and Godot still compiles just fine, so I'm pretty confident that we aren't calling those anywhere.

Given that, I'm going to take this out of draft!

@dsnopek dsnopek changed the title [DRAFT] Disable *glGetProcAddress() on the web Disable *glGetProcAddress() on the web Jun 24, 2024
@dsnopek dsnopek marked this pull request as ready for review June 24, 2024 16:02
@dsnopek dsnopek requested a review from a team as a code owner June 24, 2024 16:02
@bruvzg
Copy link
Member

bruvzg commented Jun 24, 2024

What about this file? How can we make sure that this either doesn't get run or replace it with something else for the Web?

It's inside ifdef CAN_DEBUG, which is not set on iOS and web.

@adamscott adamscott modified the milestones: 4.x, 4.3 Jun 24, 2024
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

It's great! Thanks for your hard work, @dsnopek!

@dsnopek dsnopek added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jun 24, 2024
@akien-mga akien-mga merged commit c24f2f1 into godotengine:master Jun 25, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dsnopek dsnopek deleted the web-disable-getprocaddress branch July 22, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release enhancement platform:web topic:buildsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants