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 create rendering device or parse glsl shader in headless mode #98247

Conversation

TCROC
Copy link
Contributor

@TCROC TCROC commented Oct 16, 2024

This PR resolves: #98246

It appears that some recent code changes were made that parse glsl shaders in new locations as well as some logic surrounding DisplayServer::can_create_rendering_device. Both of which introduced problems when running on a machine that does not have a gpu. This may not be the correct solution so please leave your comments and / or open a different pr with the proper fix. This at least fixed it for me though. Without further ado, here is the explanation for my changes:

bool DisplayServer::can_create_rendering_device() {
	if (get_singleton()->get_name() == "headless") {
		return false;
	}

^ We can't create a DisplayServer in headless mode... at least I don't think we can. I'm mostly going off my intuitive understanding of the word Display and regarding the fact that there is no display on a headless server without a gpu. But that is grammatical rather than technical reasoning which I know does not necessarily apply in the world of software.

void BetsyCompressor::_init() {
	if (!DisplayServer::can_create_rendering_device()) {
		return;
	}

^ Don't try to initalize a new BetsyCompressor in headless mode. I found that for whatever reason, BetsyCompressor was trying to hook into the gpu on my device and crashing if it couldn't find one. This may or may not be the correct solution.

Error RDShaderFile::parse_versions_from_text(const String &p_text, const String p_defines, OpenIncludeFunction p_include_func, void *p_include_func_userdata) {
	ERR_FAIL_COND_V_EDMSG((DisplayServer::get_singleton()->get_name() == "headless"), ERR_UNAVAILABLE, "Cannot import custom .glsl shaders when running in headless mode.");
	ERR_FAIL_NULL_V(RenderingDevice::get_singleton(), ERR_UNAVAILABLE);

^ This is really the only change that I'm fairly confident in. I actually copied this ERROR_FAIL from a different area of code that prevents glsl imports in headless mode

ERR_FAIL_COND_V_EDMSG((DisplayServer::get_singleton()->get_name() == "headless"), ERR_UNAVAILABLE, "Cannot import custom .glsl shaders when running in headless mode.");

And if you notice, that is actually preventing shader_file->parse_versions_from_text from being called on line 106: https://github.com/godotengine/godot/blob/04692d83cb8f61002f18ea1d954df8c558ee84f7/editor/import/resource_importer_shader_file.cpp#L106C8-L106C44

This is introduces a crash. So we might as well copy that check to the root cause method as well for stability purposes. Which is what I did in my change :)

NOTE:

The change I said I was "confident in" will no longer be necessary when this issue gets resolved: #94734

Which also happens to be an issue I opened 😎. But until then, this change is necessary to prevent crashes and increase stability.

I look forward to hearing your guys thoughts! As always, thanks for the awesome engine! :)

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

This looks mostly good. Just a bit more feedback to get it fully finished

@TCROC TCROC force-pushed the fix-headless-graphics-driver-and-shader-crash branch from 38efffe to c60b3ee Compare October 22, 2024 20:11
@clayjohn
Copy link
Member

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

@TCROC TCROC force-pushed the fix-headless-graphics-driver-and-shader-crash branch from c60b3ee to 41a468d Compare October 22, 2024 20:15
@TCROC TCROC force-pushed the fix-headless-graphics-driver-and-shader-crash branch from 41a468d to 2e1fc24 Compare October 22, 2024 20:17
@TCROC
Copy link
Contributor Author

TCROC commented Oct 22, 2024

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

done :) Let me know if you need anything else :)

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you

@TCROC
Copy link
Contributor Author

TCROC commented Oct 22, 2024

Looks great! Thank you

You are very welcome! And thank you as well! ❤️

@Repiteo Repiteo merged commit 52bbbd4 into godotengine:master Oct 24, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 24, 2024

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.

Can't import GLSL shaders in headless mode Headless import without gpu crashes
4 participants