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

Don't print warning messages for DisplayServer functions in headless mode #90549

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Apr 11, 2024

DisplayServer methods that only have a cosmetic effect print a warning (instead of an error). These warnings can be silenced in headless mode, as it's assumed that the cosmetic effect is undesired in headless mode anyway (e.g. changing mouse cursor shape).

This prevents spurious warnings from appearing when running the editor in headless mode, e.g. on CI to export a project.

Methods that print an error will continue to do so, as their returned value may not match expectations and have cascading consequences on the project (e.g. clipboard data not being retrievable).

Testing project: test_headless_methods.zip
Run with and without --headless from a terminal and compare output.

Preview

Before

$ godot --headless 
Godot Engine v4.3.dev.custom_build.b091162a9 (2024-04-10 15:49:59 UTC) - https://godotengine.org

WARNING: Custom cursor shape not supported by this display server.
     at: cursor_set_custom_image (servers/display_server.cpp:636)
WARNING: Clipboard is not supported by this display server.
     at: clipboard_set (servers/display_server.cpp:506)
ERROR: Clipboard is not supported by this display server.
   at: clipboard_get (servers/display_server.cpp:510)

ERROR: Virtual keyboard not supported by this display server.
   at: virtual_keyboard_get_height (servers/display_server.cpp:624)
0
WARNING: Native dialogs not supported by this display server.
     at: dialog_input_text (servers/display_server.cpp:652)

After

$ godot --headless
Godot Engine v4.3.dev.custom_build.67b915f63 (2024-04-11 21:14:41 UTC) - https://godotengine.org

ERROR: Clipboard is not supported by this display server.
   at: clipboard_get (servers/display_server.cpp:517)

ERROR: Virtual keyboard not supported by this display server.
   at: virtual_keyboard_get_height (servers/display_server.cpp:631)
0

Non-headless output for comparison

This is left unchanged in this PR.

$ godot           
Godot Engine v4.3.dev.custom_build.67b915f63 (2024-04-11 21:14:41 UTC) - https://godotengine.org
Vulkan 1.3.277 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce RTX 4090

test
ERROR: Virtual keyboard not supported by this display server.
   at: virtual_keyboard_get_height (servers/display_server.cpp:631)
0
WARNING: Native dialogs not supported by this display server.
     at: dialog_input_text (servers/display_server.cpp:663)

@Calinou Calinou added this to the 4.x milestone Apr 11, 2024
@Calinou Calinou requested a review from a team as a code owner April 11, 2024 21:59
@Calinou Calinou force-pushed the displayserver-headless-suppress-warnings branch from 4087f86 to c0c40a6 Compare April 11, 2024 21:59
@Riteo
Copy link
Contributor

Riteo commented Apr 11, 2024

Sorry, wouldn't it be simpler to just implement a stub method stub methods in the headless class? Since it's an exception, I think that a local solution would be better.

Edit: oops I said "stub method" but I meant like, stubs in general.

@m4gr3d
Copy link
Contributor

m4gr3d commented Apr 12, 2024

Sorry, wouldn't it be simpler to just implement a stub method stub methods in the headless class? Since it's an exception, I think that a local solution would be better.

Edit: oops I said "stub method" but I meant like, stubs in general.

I agree; this breaks encapsulation principle if the parent / base class has special logic for one of its derived class.

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Added feedback in comments.

@Calinou Calinou force-pushed the displayserver-headless-suppress-warnings branch from c0c40a6 to a6300ca Compare April 12, 2024 16:46
@Calinou
Copy link
Member Author

Calinou commented Apr 12, 2024

Sorry, wouldn't it be simpler to just implement a stub method stub methods in the headless class? Since it's an exception, I think that a local solution would be better.

Edit: oops I said "stub method" but I meant like, stubs in general.

Done. I've tested the MRP again and I get the same output as before (in both headless and non-headless mode).

@Calinou Calinou force-pushed the displayserver-headless-suppress-warnings branch from a6300ca to 46c8b04 Compare April 12, 2024 21:18
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 13, 2024
…mode

DisplayServer methods that only have a cosmetic effect print a warning (instead
of an error). These warnings can be silenced in headless mode, as it's assumed
that the cosmetic effect is undesired in headless mode anyway (e.g. changing
mouse cursor shape).

This prevents spurious warnings from appearing when running the editor in
headless mode, e.g. on CI to export a project.

Methods that print an error will continue to do so, as their
returned value may not match expectations and have cascading consequences
on the project (e.g. clipboard data not being retrievable).
@Calinou Calinou force-pushed the displayserver-headless-suppress-warnings branch from 46c8b04 to c3a4b4c Compare April 13, 2024 16:18
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

This isn't my area and I haven't tested it but the change is very straightforward and looks good to me.

@akien-mga akien-mga requested a review from bruvzg April 15, 2024 07:58
@akien-mga
Copy link
Member

I'm not against adding stubs, but at least some of those may need to be guarded with DisplayServer::get_singleton()->has_feature() checks before being called (which means that actually showing a warning when called should be kept, as it's information for end users when they themselves wrongly rely on the API).

So it might be worth reviewing places in the engine that call some of those APIs without guarding with the proper feature check.

For example:

WARNING: Custom cursor shape not supported by this display server.
     at: cursor_set_custom_image (servers/display_server.cpp:636)

Shouldn't happen anymore already after e6d0bf3.

DisplayServerHeadless::has_feature() is always false so in theory it should be sufficient.

@akien-mga
Copy link
Member

If someone wants to look into which checks may actually be missing in the core engine before merging this (see my above comment), that could be good.

But otherwise I'm also fine merging this as is if there's no objection from @bruvzg on making these no-ops (so end users won't know why they don't work on platforms that don't support them, and we need to make sure the docs flag those properly).

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Adding stubs to the headless DS should be fine.

@akien-mga akien-mga merged commit 32317f2 into godotengine:master May 13, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

6 participants