Skip to content

Add debug messaging during OpenGL init phase for non-debug build #8687

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dkasuga
Copy link
Contributor

@dkasuga dkasuga commented May 1, 2025

Currently, in Filament's optimized builds, error-checking macros like CHECK_GL_ERROR are disabled, likely for performance or code size reasons. However, this poses a problem for developers who use Filament as a pre-built library. They find it difficult to detect initialization errors originating within Filament, even if their own application is a debug build.

This change proposes the addition of functionality to output warning messages when OpenGL errors occur during initialization, even in optimized (non-debug) builds.

FIXES=[333130292]

@dkasuga dkasuga requested a review from pixelflinger May 1, 2025 08:29
@dkasuga dkasuga added the internal Issue/PR does not affect clients label May 1, 2025
@@ -50,6 +50,12 @@ void assertFramebufferStatus(utils::io::ostream& out, GLenum target, const char*
# define CHECK_GL_FRAMEBUFFER_STATUS(out, target) { GLUtils::checkFramebufferStatus(out, target, __func__, __LINE__); }
#endif

#if defined(NDEBUG) && defined(FILAMENT_ENABLE_INIT_GL_WARNINGS_FOR_OPTIMIZED_BUILD)
# define CHECK_GL_INIT_ERROR_FOR_OPTIMIZED_BUILD(out) { GLUtils::assertGLError(out, __func__, __LINE__);}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will actually terminate the program, so it's more than just a "warning".

I think this is how this should work:

  • on debug builds, we always call assertGLError
  • on release builds, by default we do nothing unless FILAMENT_ENABLE_INIT_GL_WARNINGS_FOR_OPTIMIZED_BUILD is set, in which case we call checkGLError (i.e. we don't crash in release builds, just log).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review. I now agree for using checkGLError for the "warning" purpose.

Following your suggestion I updated the macro definition and the cmake option default value (from ON to OFF), but I now wonder if assertGLError should be really added for debug builds by the new macro. If we limit assertions only to the debug build, the existing CHECK_GL_ERROR insertions should be enough, and there might be no need to add new assertions (to the parts where I inserted the new macro). WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the new CHECK_GL_INIT_ERROR_FOR_OPTIMIZED_BUILD macro has a specific purpose and isn't intended as a direct replacement for CHECK_GL_ERROR.

I guess mathias meant to keep our current idiom, assert for debug, assuming this new macro can be a replacement for CHECK_GL_ERROR. However, if your intention is for this new macro to be an additional check used alongside CHECK_GL_ERROR, meaning users might need to call both, that could be unintuitive without detailed comments. So comprehensive comments detailing the specific use case may be needed.

Either way, I would consider making the new macro sound more generic, renaming it something like CHECK_GL_ERROR_FOR_RELEASE, indicating this macro is to govern the behavior in release mode.

@pixelflinger pixelflinger requested review from z3moon and poweifeng May 1, 2025 18:24
@dkasuga dkasuga force-pushed the dk/opengl-debug-message branch from 56b09df to ff35621 Compare May 2, 2025 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants