Exposing this makes sense to me, but I wonder why we put it in OS originally. This isn't really OS specific, but more something that reflects the engine configuration.

Wouldn't it be more appropriate in Engine, or in ProjectSettings?

We could still move the C++ OS implementation which wasn't exposed yet, the only potential compat breakage would be for C++ modules.

Exposing this makes sense to me, but I wonder why we put it in OS originally. This isn't really OS specific, but more something that reflects the engine configuration.

I've moved the exposed method to Engine in a separate commit (should be squashed before merging). It still works after testing.

I chose to keep the internal method in OS to avoid any risk of regressions and preserve compatibility. The rendering driver has OS-specific interactions in particular, with ANGLE, Direct3D, etc.

Should Metal be added into the document since its support was added in #88199 ?

Should Metal be added into the document since its support was added in #88199 ?

Done (I also added a mention of d3d12).

Should this be removed?
https://docs.godotengine.org/en/stable/classes/class_os.html#enum-os-renderingdriver
I can't find anything that actually references it.

Should this be removed?
https://docs.godotengine.org/en/stable/classes/class_os.html#enum-os-renderingdriver
I can't find anything that actually references it.

This enum is useless for now, there is nothing using this.
So yeah this should be remove since the method will return string directly.

I've moved the exposed method to Engine in a separate commit (should be squashed before merging). It still works after testing.

I chose to keep the internal method in OS to avoid any risk of regressions and preserve compatibility. The rendering driver has OS-specific interactions in particular, with ANGLE, Direct3D, etc.

The problem with that is that we now have a discrepancy between the core C++ API (method in OS) and the scripting API (method in Engine). Which is a bit confusing for people using both, but also breaks the expectation for godot-cpp that the C++ extension API is as close as possible to the C++ one, allowing a (relatively) easy path between C++ modules and C++ extensions.

So if it can't be moved from OS to Engine in core, I'd keep it exposed in OS to be consistent. But I don't see a problem with moving it from OS to Engine, rendering code can include core/config/engine.h just fine, no?

So if it can't be moved from OS to Engine in core, I'd keep it exposed in OS to be consistent. But I don't see a problem with moving it from OS to Engine, rendering code can include core/config/engine.h just fine, no?

Is this the only merge blocker? Testing games is getting harder, now that the RD renderers support multiple drivers. For example, a common problem is that some GPUs come with bad Vulkan drivers, but good D3D12 drivers. Being able to know the currently used rendering driver is important for end-users to know how best to play our games.

The problem with that is that we now have a discrepancy between the core C++ API (method in OS) and the scripting API (method in Engine). Which is a bit confusing for people using both, but also breaks the expectation for godot-cpp that the C++ extension API is as close as possible to the C++ one, allowing a (relatively) easy path between C++ modules and C++ extensions.

These are the changes needed to keep things consistent: akien-mga@3a1117c

WDYT @clayjohn?

Alternatively, we can keep it in OS and expose it in OS to scripting.

We discussed in the rendering meeting. We think that we should investigate exposing it through the RenderingServer as that is the most natural place for these methods to exist.

While the indirection is awkward, it should be much nicer from a user point of view

We discussed in the rendering meeting. We think that we should investigate exposing it through the RenderingServer as that is the most natural place for these methods to exist.

Done.

While I'm thinking about it, I wonder if I should name the exposed methods as such:

• RenderingServer.get_current_rendering_driver_name()
• RenderingServer.get_current_rendering_method_name()

Right now, we have the following which is inconsistent:

• RenderingServer.get_current_rendering_driver_name()
• RenderingServer.get_current_rendering_method()

We don't have exposed enums for the rendering drivers and methods (an unexposed enum for the rendering driver already exists). We should probably add one for the rendering method and expose both enums. This would allow creating and exposing the following enum-based methods:

• RenderingServer.get_current_rendering_driver()
• RenderingServer.get_current_rendering_method()

This would allow users to make comparisons that are not string-based, therefore not being prone to typos (while also allowing for autocompletion to work). We should probably decide on this before merging the PR, as changing method names later on would break compatibility.

What do you think?

TIWAGOS and I'd let the rendering team decide, but yes I feel exposing an enum version of those methods might make sense. And our internal code should likely also be refactored to use the enums, the string comparisons we have throughout the rendering code are a bit awkward.

Is it worth to make it enum?
If we made it enum, then it should replace all of the original strings, otherwise I don't think it is worth to do
And it will be a lot of work to replace all of it

Regardless of the outcome, there's an existing enum that despite being exposed, doesn't seem to be referenced anywhere:
https://docs.godotengine.org/en/stable/classes/class_os.html#enum-os-renderingdriver

Regardless of the outcome, there's an existing enum that despite being exposed, doesn't seem to be referenced anywhere: docs.godotengine.org/en/stable/classes/class_os.html#enum-os-renderingdriver

Ah, that's what I meant actually. The enum is exposed, it's just not usable anywhere in the scripting API since no methods return it.

We discussed using enums for a very long time during the initial 4.0 rewrite. I was in favour of them at the time. But the consensus was that we wanted to eventually allow people to add new rendering methods and rendering drivers through GDextensions. Having a closed enum would make that very challenging.

I agree, that using Strings allows for future expansion and adding additional renderers at runtime using GDExtensions. However, it would be great to have a way to validate the string values. An option would be to have a
Dictionary RenderingServer::get_supported_rendering_drivers()

and

Dictionary RenderingServer::get_supported_rendering_methods()

In these dictionaries the keys would be the valid settings of the get_current_rendering_*() methods, while the values could be an additional Dictionaries with information specific to the driver / rendering methods (could even be reserved for future use).

That said, I think that this should be implemented in a separate PR.

I'd like to chime in if I may: I was researching whether or not it was possible to check the rendering method idiomatically, because I wanna use a feature unavailable in the Compatibility renderer on Mobile and Forward+ builds, but also have a fallback to a feature that's supported, to have feature-complete web builds. Am I right to believe this isn't achievable as of 4.3 without something like supporting two scene duplicates at once, one for desktop/mobile and one for web, but with this PR it's possible to spawn different nodes depending on the platform programmatically?

Am I right to believe this isn't achievable as of 4.3

This can be achieved in 4.3 with the following code:

if ProjectSettings.get_setting_with_override("rendering/renderer/rendering_method") == "gl_compatibility":
	print("Using Compatibility")
else:
	print("Using Forward+/Mobile")

The _with_override() is important here, as we want to get the setting's value taking feature tags into account. The web platform always uses gl_compatibility since it has a .web override for the Rendering Method project setting.

The downside of this approach is that it doesn't take CLI argument overrides into account, nor automatic fallbacks that may happen on unsupported GPUs (but this isn't implemented in 4.3 anyway, only 4.4).

Thanks!