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 vulkan gpu selection in the project settings #81752

Closed
wants to merge 10 commits into from
Closed

Add vulkan gpu selection in the project settings #81752

wants to merge 10 commits into from

Conversation

KoTeYkA23
Copy link
Contributor

Add ability to select Vulkan GPU from project settings in Rendering/Rendering Device category

@KoTeYkA23 KoTeYkA23 requested review from a team as code owners September 16, 2023 18:48
@fire fire changed the title add selection vulkan gpu in project settings Add vulkan gpu selection in the project settings Sep 16, 2023
@Calinou
Copy link
Member

Calinou commented Sep 16, 2023

This will apply to the exported project as well, and Vulkan GPU indices are not portable across systems. This means that if you export a project with this setting set to a non-default value, the exported project may use a suboptimal GPU by default (or worse, software emulation).

I think this should be an editor setting, so that it only applies when running the project from the editor.

Comment on lines 243 to 252
//check for cmd argument override, otherwise get index by config
if (gpu_idx != -1)
return gpu_idx;
else {
int gpu_index_from_config = GLOBAL_GET("rendering/rendering_device/gpu_index");
if (gpu_index_from_config == -1)
return gpu_idx;
else
return gpu_index_from_config;
}
Copy link
Member

Choose a reason for hiding this comment

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

Please check the Code style guidelines 🙂

I recommend setting up clang-format locally as described on that page.

@KoTeYkA23
Copy link
Contributor Author

This will apply to the exported project as well, and Vulkan GPU indices are not portable across systems. This means that if you export a project with this setting set to a non-default value, the exported project may use a suboptimal GPU by default (or worse, software emulation).

I think this should be an editor setting, so that it only applies when running the project from the editor.

Can godot change config values in game code? (sorry im new to godot). I think ability to change gpu with config in final build will be nice also (if it can ofk), the main reason I add this because my system have 2 gpus (6700xt and 1070 specificly) and all vulkan applications chose 1070 for some "vulkan" reason. But anyway I agree that description must say explicitly not touch this if you dont know what it change

KoTeYkA23 and others added 2 commits September 17, 2023 00:02
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
main/main.cpp Outdated Show resolved Hide resolved
KoTeYkA23 and others added 2 commits September 17, 2023 00:04
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@Calinou
Copy link
Member

Calinou commented Sep 18, 2023

Can godot change config values in game code?

Yes, but most project setting changes are only effective after the project restarts. In exported projects, this makes for an awkward situation as you must find a way to set the value before the project even starts. The only way to achieve this is by writing a project settings override file (see the Overriding section of the description).

A complete implementation that works with in-game settings without a project settings override requires godotengine/godot-proposals#6423 to be implemented.

@KoTeYkA23
Copy link
Contributor Author

A complete implementation that works with in-game settings without a project settings override requires godotengine/godot-proposals#6423 to be implemented.

Ok, so I guess I need move this parameter to editor settings anyway if GPU selection will be build in game

@KoTeYkA23
Copy link
Contributor Author

Oh no... EditorSettings not existing when function get_gpu_index called...

@KoTeYkA23
Copy link
Contributor Author

Should we open issue or this is intended behavior?

@Calinou
Copy link
Member

Calinou commented Sep 18, 2023

Should we open issue or this is intended behavior?

The editor settings are read after the renderer is initialized, so I'm afraid this can't be done.

Alternatively, you'd have to open the editor settings using ConfigFile and read it, but this is a giant hack that probably won't be accepted 🙂

@KoTeYkA23
Copy link
Contributor Author

well, at least I tried

@KoTeYkA23 KoTeYkA23 closed this Sep 18, 2023
@KoTeYkA23
Copy link
Contributor Author

also, there is another problem with gpu selection, even if I add cmd argument to specific gpu it will be set until new godot process open or restart, so there is no way currently to explicitly select gpu for editor

@Calinou
Copy link
Member

Calinou commented Sep 18, 2023

also, there is another problem with gpu selection, even if I add cmd argument to specific gpu it will be set until new godot process open or restart, so there is no way currently to explicitly select gpu for editor

--gpu-index is already in the list of forwardable command line arguments:

godot/main/main.cpp

Lines 859 to 887 in e3e2528

#ifdef TOOLS_ENABLED
if (I->get() == "--debug" ||
I->get() == "--verbose" ||
I->get() == "--disable-crash-handler") {
forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(I->get());
forwardable_cli_arguments[CLI_SCOPE_PROJECT].push_back(I->get());
}
if (I->get() == "--single-window") {
forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(I->get());
}
if (I->get() == "--audio-driver" ||
I->get() == "--display-driver" ||
I->get() == "--rendering-method" ||
I->get() == "--rendering-driver") {
if (I->next()) {
forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(I->get());
forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(I->next()->get());
}
}
// If gpu is specified, both editor and debug instances started from editor will inherit.
if (I->get() == "--gpu-index") {
if (I->next()) {
forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(I->get());
forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(I->next()->get());
forwardable_cli_arguments[CLI_SCOPE_PROJECT].push_back(I->get());
forwardable_cli_arguments[CLI_SCOPE_PROJECT].push_back(I->next()->get());
}
}
#endif

To apply it permanently, create a desktop shortcut with the option added in the shortcut's properties.

We could read an environment variable that you can set user-wide or system-wide, but I feel adding another nonstandard way to define your preferred GPU isn't ideal 🙁
See also #81870.

@KoTeYkA23
Copy link
Contributor Author

ah it was fixed, I tested it on 4.1 stable and that not worked

@KoTeYkA23
Copy link
Contributor Author

KoTeYkA23 commented Sep 18, 2023

We could read an environment variable that you can set user-wide or system-wide, but I feel adding another nonstandard way to define your preferred GPU isn't ideal 🙁
See also #81870.

The best way for this I only see is separate reading config variables from objects, or add specific type/object/macro to read them, but anyway cmd line property now fixed, so thanks for that

@AThousandShips AThousandShips removed this from the 4.x milestone Sep 19, 2023
@KoTeYkA23 KoTeYkA23 deleted the master branch September 20, 2023 19:43
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.

3 participants