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

Rename RenderingServer::is_low_end() to RenderingServer::is_using_gl_compatibility() #100384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Dec 13, 2024

This name is more explicit as for what the method actually does.

The original name dates back to Godot 3.1 when GLES2 was added alongside GLES3. There were only 2 rendering methods available in Godot 3.x, but we have 3 now.

Since this method isn't exposed to scripting, this doesn't break compatibility.

Verified

This commit was signed with the committer’s verified signature.
Calinou Hugo Locurcio
…gl_compatibility()`

This name is more explicit as for what the method actually does.

The original name dates back to Godot 3.1 when GLES2 was added alongside
GLES3. There were only 2 rendering methods available in Godot 3.x, but we have 3 now.

Since this method isn't exposed to scripting, this doesn't break compatibility.
@Calinou Calinou force-pushed the renderingserver-rename-is-low-end branch from f924760 to a63a20b Compare December 14, 2024 00:01
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Much clearer – thanks!

@akien-mga akien-mga requested a review from clayjohn December 14, 2024 17:12
@clayjohn
Copy link
Member

I don't have a strong preference here, but I question whether is_gl_compatibility() is the name we should go with. The intention of that function is not just to provide a quick access to check if we are using the compatibility renderer. The purpose is to highlight that a certain feature set may not be available. It was created back when we had a GLES2 renderer. And the name was kept because we thought we might have a GLES2 renderer in Godot 4 as well.

@stuartcarnie
Copy link
Contributor

And the name was kept because we thought we might have a GLES2 renderer in Godot 4 as well.

Does that mean the value returns is not binary but perhaps an enumeration? Would there be a reason to change behaviour if the back end was GLES3 vs GLES2?

@clayjohn
Copy link
Member

And the name was kept because we thought we might have a GLES2 renderer in Godot 4 as well.

Does that mean the value returns is not binary but perhaps an enumeration? Would there be a reason to change behaviour if the back end was GLES3 vs GLES2?

Not an enumeration, no. But its supposed to encapsulate a defined feature set rather than a particular backend.

I suspect that if this has been treated as a flag for the Compatibility renderer then there will be places where it is used that wouldn't work for a potential GLES2 backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants