[3.x] Clear glErrors instead of crashing when initializing GLES3

[madmiraal]

In GLES3, sometimes, another application can set a GLError flag. For example, the Android emulator's OpenGL rendering engine with: glUtilsParamSize: unknow param 0x000082da. Later, before Godot tries to initialize the RasterizerCanvasGLES3, it checks whether there is already a GLError:

godot/drivers/gles3/rasterizer_canvas_gles3.cpp

Lines 2151 to 2155 in 2966084

	void RasterizerCanvasGLES3::initialize() {
	gl_checkerror();
	RasterizerCanvasBaseGLES3::initialize();
	batch_initialize();

and crashes if there is:

godot/drivers/gles3/rasterizer_canvas_gles3.cpp

Lines 68 to 71 in 2966084

	void RasterizerCanvasGLES3::gl_checkerror() {
	GLenum e = glGetError();
	CRASH_COND(e != GL_NO_ERROR);
	}

This check does not happen in GLES2, so even if there is an existing error, it does not crash:

godot/drivers/gles2/rasterizer_canvas_gles2.cpp

Lines 2322 to 2325 in 2966084

	void RasterizerCanvasGLES2::initialize() {
	RasterizerCanvasBaseGLES2::initialize();
	batch_initialize();

Instead of crashing if there is an existing GLError, this PR clears the GLError before trying to initialize RasterizerCanvasGLES3.

Note 1: It will still check after initializing for errors and crash if the initializing causes a GLError:

godot/drivers/gles3/rasterizer_canvas_gles3.cpp

Lines 2283 to 2284 in 2966084

	gl_checkerror();
	}

Again, this check does not happen on GLES2, so GLES2 won't crash there either. These crashing checks were added as part of #42119. Perhaps they should be removed entirely. cc @lawnjelly

Note 2: This PR is made against 3.x, because there is no GLES3 on master.

[lawnjelly]

Ah yes this is something left in from development, good spot. 👍

Note also there is a RasterizerGLES2::gl_check_for_error function and corresponding version in GLES3, these are an attempt to translate the gl error codes into something a little more useful rather than just numbers (they still don't tell you much)... I used these during development and finding bugs, although they aren't called from the current build.

It didn't occur to me also that something external might set an error condition! 😄

Maybe we can centralize all this in one place, probably in the Rasterizer rather than RasterizerCanvas so it can be accessed from either the canvas or scene rasterizer (I think that's why I put it there originally). Then we can print the error messages clear them out, and not call to crash, that can always be added during debugging. It does seem to make sense to have a convenient central function for doing all this error checking.

I think for the initialize function, providing they just print the errors and don't crash, there shouldn't be a problem leaving them in, it might alert us to problems occurring silently and help diagnose issues.

The other thing is that some GL implementations are more verbose with their errors than others, and something that passes fine in development might generate an error that is more of a warning in practice, and we don't want it to crash in that circumstance. From memory some things OpenGL doesn't always pick up are parameters, like using an int where there should be a bool or vice versa etc.

[madmiraal]

How about a static bool gl_check_errors() method in RasterizerGLES2 and RasterizerGLES3 that clears the errors, and if DEBUG_ENABLED is defined, prints them. It then returns true if errors were found, allowing the caller to decide what to do?

This would replace:

• virtual Rasterizer::gl_check_for_error() and its inherited implementations which are currently unused.
• RasterizerCanvasGLES3::gl_checkerror()

[lawnjelly]

Yup that sounds great. 👍

[lawnjelly]

Looks fine to me. I'm not sure the comment about STACK_UNDERFLOW and STACK_OVERFLOW warrants more than a one liner, but this is fine.

[madmiraal]

I'm not sure the comment about STACK_UNDERFLOW and STACK_OVERFLOW warrants more than a one liner

It's a one line justification for why I've commented out the subsequent lines.

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 3.3.3.