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

Add --no-header option #79179

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Add --no-header option #79179

merged 1 commit into from
Feb 15, 2024

Conversation

abitrolly
Copy link
Contributor

The option to suppress printing Godot headers, see godotengine/godot-proposals#4998 for details.

Closes godotengine/godot-proposals#4998.
Closes #63575.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

To add this feature to the project settings, I suggest you add the following lines of code:

in core/config/project_settings.cpp to line 1200:

GLOBAL_DEF("application/run/quit_header", false); 

and in main.cpp to line 1600 :

if (bool(GLOBAL_GET("application/run/quit_header"))){
		Engine::singleton->_print_header = false;
}

that's about it!

@Calinou
Copy link
Member

Calinou commented Jul 9, 2023

GLOBAL_DEF("application/run/quit_header", false); 

Why is the setting called quit_header instead of print_header? Is that a typo?

@ghost
Copy link

ghost commented Jul 9, 2023

yes, it's a mistake on my part, we can name it "disable header"

@ghost
Copy link

ghost commented Jul 9, 2023

I'll test it and let you know if it works when I've finished running my workflow.

@ghost
Copy link

ghost commented Jul 9, 2023

I tested it with the project parameters and it worked!

image
the name "quiet_header" is Temporary

@abitrolly
Copy link
Contributor Author

@kaissouDev I'd prefer to see this PR reviewed/merged first, and then address project setting logic in a different PR. To keep it simple.

@ghost
Copy link

ghost commented Jul 9, 2023

Yes, you're right, it's better to wait until this pull request is merged. At least we know how to add this option to the project parameters.

@abitrolly
Copy link
Contributor Author

Rebased, solved conflicts. Testing with the hello script.

$ /bin/godot.linuxbsd.editor.x86_64 -s hello.gd
Godot Engine v4.3.dev.custom_build.7bfed92d9 - https://godotengine.org
Inconsistent value (1) for DRI_PRIME. Should be < 1 (GPU devices count). Using: 0
OpenGL API 4.6 (Core Profile) Mesa 23.3.5 - Compatibility - Using Device: Intel - Mesa Intel(R) HD Graphics 4400 (HSW GT2)
 
Hello!
Project is missing: /tmp/ggg/project.godot 
$ ./bin/godot.linuxbsd.editor.x86_64 -s hello.gd --no-header
Inconsistent value (1) for DRI_PRIME. Should be < 1 (GPU devices count). Using: 0
Hello!
Project is missing: /tmp/ggg/project.godot

I will now try to add this to projects settings too.

@abitrolly
Copy link
Contributor Author

This is now ready for review.

@Calinou @kaissouDev

@abitrolly abitrolly requested a review from a team as a code owner February 12, 2024 09:02
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Docs do look good now. Remember to squash your commits now. Thank you for your patience very much

main/main.cpp Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
core/config/engine.h Outdated Show resolved Hide resolved
core/config/engine.cpp Outdated Show resolved Hide resolved
@abitrolly
Copy link
Contributor Author

Squashed into a single commit.

@abitrolly
Copy link
Contributor Author

@AThousandShips what is the p_string convention for function argument? I just followed the style, but it is still interesting what p_ stands for.

@AThousandShips
Copy link
Member

"parameter" :)

main/main.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected. Should be good to merge after applying the suggestion and squashing commits together.

@abitrolly
Copy link
Contributor Author

@Calinou the rendering method info strings are gone from Vulkan backend after rebasing over #87340 but that can be added later if needed. So this is now ready.

@Calinou
Copy link
Member

Calinou commented Feb 14, 2024

@Calinou the rendering method info strings are gone from Vulkan backend after rebasing over #87340 but that can be added later if needed. So this is now ready.

I noticed this while testing – the OpenGL header is silenced, but not the Vulkan header. Could you look into resolving this so it's silenced with both rendering drivers?

@abitrolly
Copy link
Contributor Author

@Calinou can you give me the message? I don't know what to look for..

@Calinou
Copy link
Member

Calinou commented Feb 14, 2024

@Calinou can you give me the message? I don't know what to look for..

The message is here:

print_line(vformat("%s %s - %s - Using Device #%d: %s - %s", get_device_api_name(), get_device_api_version(), rendering_method, device_index, _get_device_vendor_name(device), device.name));

@abitrolly
Copy link
Contributor Author

@Calinou done.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me. Last few nitpicks :)

I noticed that @AThousandShips is credited as co-author twice in your commit message, you could amend it to remove the duplicate line. (Not critical though, but this could be done while doing the other suggested changes.)

doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 15, 2024
* Do not print empty line when header is disabled
* Do not print Vulcan header
* Also add "Print header" project setting (default On)
  (suggested by @kaissouDev)
* Add docs for the project setting
  (with suggestions by @Mickeon and @akien-mga)

Co-authored-by: Micky <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: Rémi Verschelde <[email protected]>
@abitrolly
Copy link
Contributor Author

@akien-mga done.

@akien-mga akien-mga merged commit c286c35 into godotengine:master Feb 15, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@ghost
Copy link

ghost commented Feb 15, 2024

Good job @abitrolly !

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.

Add --no-header command line argument to silence the lines printed by Godot on startup
5 participants