[Gustanator]

Fixes #5869 and adds ~3 FPS to the Android build (in the Black Forest benchmark).

Tests (OGLES2):

Tests (OGLES3):

Agreement

By creating a pull request in stk-code, you hereby agree to dual-license your contribution as
GNU General Public License version 3 or any later version and
Mozilla Public License version 2 or any later version.

This includes your previous contribution(s) under the same name of contributor.

You also confirm that this contribution is your own original work
and that it does not contain code generated by AI tools.

Keep the above statement in the pull request comment for agreement.

[Gustanator]

I noted some #define's are messed in gles2-ext.h and I didn't edited the files name to OGLES3-related things

I will fix them later

[Gustanator]

By the way, here is the PR apk: https://github.com/Gustanator/stk-code/releases/download/ogles3/android-bata-with-ogles3.apk

[Alayan-stk-2]

That was a quick turnaround.

However there are some things to clarify first:

• While this addresses lib/irrlicht code ; it doesn't change anything in STK's main code. In android/android_mk we still have -DUSE_GLES2 \ ; and the main code is peppered with USE_GLES2 checks.
• We still have utils/progress_bar_android.cpp:148: params.DriverType = video::EDT_OGLES2;
• We still have IRR_COMPILE_WITH_OGLES2
• Most of the line changes come from removing "OES" from the name of variables. That's perhaps required but could you link some relevant documentation explaining the difference and explain the purpose?
• All the main STK code if USE_GLES2 still make the same assumptions as before. Now, it may be too much work to do all at once, but we should probably (1) comment that GLES2 assumptions are still used for every #ifdef check that hasn't been analyzed for potential benefits of a GLES3 baseline (with USE_GLES2 being renamed to USE_GLES3, it's important to document in the code), (2) try to find at least some cases where the change allows to go down a different execution path (for higher graphical quality or perf).

[Gustanator]

I just edited the GLES files without change the #ifdefs for a fast change and test, n
but I will edit later

[Gustanator]

• Most of the line changes come from removing "OES" from the name of variables. That's perhaps required but could you link some relevant documentation explaining the difference and explain the purpose?

In a Raspberry Pi forum: https://forums.raspberrypi.com/viewtopic.php?t=302164

A screenshot that I take of Gemini:

[Gustanator]

I just edited the GLES files without change the #ifdefs for a fast change and test, n
but I will edit later

It's done ;)

[Alayan-stk-2]

The majority of the changes are fairly safe renames to remove the misleading GLES2 naming. I had a look over just in case, but that part is fine.

I uncovered a couple of issues, mentioned in my review comments. I have not analyzed the shader code changes to confirm correctness, it will need verification.

In the process of reviewing this, I had a look at code that indicates that CIrrDeviceSDL tries to get a 3.0 context normally, but has a 2.0 fallback. So that confirms that OGLES2 could in theory sometimes happen, but in practice it's already vanishingly rare as very few devices don't support OGLES3 (and even less of those have the required minimum Android version).

There are also potential issues if the legacy device settings is on while using OGLES.

My biggest concern, which hasn't a direct review comment, is with the gles2-ext.h file. That's supposed to be an explicit list of GLES2 extension and their defines (although I'm not too familiar with the specifics), and I think that going in that file to replace various things with GLES3 core equivalent misses the point. If something is already in the main GLES3 headers, why do we need to try to pull the enum values from gles2-ext?

I haven't fully considered the question, but I'm rather confident that's not the right approach.

[Alayan-stk-2]

This change sits within

else if (m_creation_params.opengl_api == CEGL_API_OPENGL)

That's regular OpenGL, which should be left untouched by any OGLES change done in this PR.

[Alayan-stk-2]

This kind of stuff is why I bother to actually review even when it looks good on the surface, and why reviewing isn't always quick.

[qwertychouskie]

If the goal is to require GLES 3.0, the old Irrlicht GLES renderer should probably be removed entirely. No point in carrying around an entire render engine that can never be activated (due to STK's own renderer being used). If stripping the old renderer is the path chosen, this PR should look very different.

That said, is there any particular benefit to stripping the old renderer?

but in practice it's already vanishingly rare as very few devices don't support OGLES3 (and even less of those have the required minimum Android version).

On Android, perhaps, but e.g. the Raspberry Pi 1-3 use GLES 2, and it's not hard to imagine a decent number of potential users there (especially since STK is included in distro repos).

[Gustanator]

If stripping the old renderer is the path chosen, this PR should look very different.

I don't know much about the Antarctica Engine (I always thought that the irrlicht forder contains this engine), so I did this basing in afaik the game structure.

If someone help me to understand the Antarctica engine, I will be very happy :3

[Alayan-stk-2]

If the goal is to require GLES 3.0, the old Irrlicht GLES renderer should probably be removed entirely. No point in carrying around an entire render engine that can never be activated (due to STK's own renderer being used). If stripping the old renderer is the path chosen, this PR should look very different.

Fair point! I hadn't particularly thought about it, but we ought to check in which cases that path is actually taken.

If the COGLES titled files are only used for the legacy GLES2 renderer, then there is no real point in modifying them, they should be stripped after confirming they only ever get used in the legacy path. The flow of instructions leading to them would need to be checked.

However a file like lib/irrlicht/source/Irrlicht/CContextEGL.cpp contains code about both the modern and legacy path so I would assume it is still relevant.

That said, is there any particular benefit to stripping the old renderer?

Currently, the GLES defines lead to some code paths that assume GLES 2.0 instead of being able to take advantage of core GLES 3.0 features. Now, in theory there are other ways this could be handled, like replacing/supplementing the defines with legacy_device checks, but if we don't have to handle the GLES2 case we get less work and meaningful code simplifications.

Simpler code is also easier to reason about in the future.

On Android, perhaps, but e.g. the Raspberry Pi 1-3 use GLES 2, and it's not hard to imagine a decent number of potential users there (especially since STK is included in distro repos).

I checked, there are indeed some people reporting playing STK on a Pi 3. I incidentally found an old thread on the Raspberry Pi forums where you participated yourself!

But:

• SBCs aren't really meant to be a primary computing device and most SBC owners will have a more appropriate device to run STK on.
• Performance sucks. We are talking about 30FPS using the very poor-looking legacy renderer. In the one video I found, the speedometer and nitro gauge didn't even work. It's the very edge of playable, and presumably the 30FPS figure is for the average track, not for the more demanding ones (the video showcase Nessie's Pond which is notoriously . Something like Black Forest would fall further down. And that's on the Pi3, Pi1 and Pi2 don't event enter the argument.
• These factors also mean that while the theoretical number of devices that run that path might be higher than anticipated, the practical number of affected players is extremely low.
• The average future standard STK tracks will be more demanding to run, not less (although we don't want to increase the peak requirements too much, the average requirements will increase significantly).
• Previous STK versions would always remain available. That's one of the beauties of an open project like SuperTuxKart. I would assume that if a user is willing to put up with various things that make for a poor experience (low FPS, crappy resolution and graphics), using the latest and greatest is not their primary concern.

Now in fairness, I suppose that users relying on distro package might find installing an old version more difficult than it is with our official packages, although the Linux arm packages that are available should work on Pi devices, I think.

[Gustanator]

I removed the gles2-ext.h file (and fixed what was needed)

@Alayan-stk-2, you can review now

[Gustanator]

@qwertychouskie, about the graphics_engine: In nowhere of the ge code I found smth related to GLES (only in irrlicht)

[qwertychouskie]

@qwertychouskie, about the graphics_engine: In nowhere of the ge code I found smth related to GLES (only in irrlicht)

That's because most rendering code in STK's rendering engine is identical between GL and GLES; the only differences are the #if defined(USE_GLES2)/#if !defined(USE_GLES2) sections.

Stepping back to a much broader view of this PR:

This PR, in its current state, has a lot of changes that don't make sense in the context of a full understanding of all of STK's rendering code. For example, making the legacy Irrlicht renderer require GLES 3.0 doesn't make sense, because then the whole renderer is just a dead code path, since STK's own rendering engine will always be used.

If removing GLES 2.0 support, GL 2.x support should probably be ripped out at the same time; i.e. completely remove the legacy Irrlicht renderers (CVS->isGLSL() is always true). Keeping the legacy renderer for desktop GL but not GLES doesn't make much sense in my opinion; if the goal if a significant cleanup of the codebase, only removing both will truly accomplish that.

If removing the legacy Irrlicht renderers is desired, it would probably be best to re-start this work fresh in a new PR, targeting the Evolution branch (not 1.5.x). (Also, this type of bulk-code-removal job may be a good task for an LLM coding tool, as LLMs are often better at "reading" the mass amount of code to find what all can/should be deleted when doing these types of mass-deletions. Copyright concerns with LLM output also aren't really applicable when just using them to delete a bunch of code.)

One change I would like to see sooner (and probably on master) is renaming the USE_GLES2 define to USE_GLES. USE_GLES2 was always an inaccurate/misleading name, and has bugged me ever since GLES support was originally added almost a decade ago.

Another (orthogonal) change I'd personally like to see some time is making GL vs GLES a runtime choice (like GL vs Vulkan already is) rather than a compile-time choice. I actually attempted this a while ago (#4439) but it wasn't accepted at the time. If anyone wants to tackle this change, feel free to use that as a base. It will probably need updating though since various rendering code has changed since then, and the GL vs GLES choice should probably also be plumbed into the UI spinner that currently allows selecting GL vs Vulkan, so it allows selecting between GL, GLES, and Vulkan.

[SewerFox]

Another (orthogonal) change I'd personally like to see some time is making GL vs GLES a runtime choice (like GL vs Vulkan already is) rather than a compile-time choice. I actually attempted this a while ago (#4439) but it wasn't accepted at the time. If anyone wants to tackle this change, feel free to use that as a base. It will probably need updating though since various rendering code has changed since then, and the GL vs GLES choice should probably also be plumbed into the UI spinner that currently allows selecting GL vs Vulkan, so it allows selecting between GL, GLES, and Vulkan.

I'm starting to revamp your PR, but I won't start it soon as I'm studying the AngelScript system to fix some warnings (and update it)