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

Make all render driver project settings require restart #82008

Merged
merged 1 commit into from
Sep 24, 2023
Merged

Make all render driver project settings require restart #82008

merged 1 commit into from
Sep 24, 2023

Conversation

KoTeYkA23
Copy link
Contributor

Since I tested #70315 I saw that on change renderer in project settings does not offer to restart editor, so I made them restart editor 🙂

@KoTeYkA23 KoTeYkA23 requested a review from a team as a code owner September 20, 2023 20:17
@AThousandShips AThousandShips changed the title Set all render drivers in Project Settings to be project restatable Make all render driver project settings require restart Sep 20, 2023
@AThousandShips AThousandShips added this to the 4.x milestone Sep 20, 2023
@AThousandShips
Copy link
Member

Can you please update the comment message to match the PR title, more correct terminology, use got commit --amend and then git push --force

@KoTeYkA23
Copy link
Contributor Author

done

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 21, 2023
@akien-mga
Copy link
Member

I just merged #72831 which introduced some merge conflicts as it's changing the same project settings. Could you rebase to solve the merge conflicts?

@KoTeYkA23
Copy link
Contributor Author

done

@AThousandShips
Copy link
Member

Please squash your commits into one, see here

@KoTeYkA23
Copy link
Contributor Author

KoTeYkA23 commented Sep 21, 2023

yeah, sorry insead rebase I merged it accidently, hopefully I fixed everything

@KoTeYkA23
Copy link
Contributor Author

oh, I accidentally remove angle hints, one sec

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

You changed the wrong part of the code

@AThousandShips
Copy link
Member

Please squash again :)

Update main/main.cpp

Co-authored-by: A Thousand Ships <[email protected]>

Update main/main.cpp

Co-authored-by: A Thousand Ships <[email protected]>
@KoTeYkA23
Copy link
Contributor Author

done, sorry for newbie git mistakes

@AThousandShips
Copy link
Member

It's a learning process :)

@akien-mga
Copy link
Member

Technically, most of these settings don't really require a restart. A restart is only needed for the setting to be picked up for the editor process, but it will pick up the changed setting fine when running the game. Only the platform override which matches the editor's host platform also really warrants restarting to pick it up in the editor.

Still, it's not a bad idea to suggest a restart when changing those. I'm just slightly worried about the UX - when you change those settings, you typically need to change several of them (e.g. all platform specific overrides), and having a nagging message that one should restart the editor after you change the first, when you're meant to change several of them, might be a bit annoying. But maybe it's fine.

@KoTeYkA23
Copy link
Contributor Author

KoTeYkA23 commented Sep 22, 2023

Technically, most of these settings don't really require a restart. A restart is only needed for the setting to be picked up for the editor process, but it will pick up the changed setting fine when running the game. Only the platform override which matches the editor's host platform also really warrants restarting to pick it up in the editor.
Still, it's not a bad idea to suggest a restart when changing those. I'm just slightly worried about the UX - when you change those settings, you typically need to change several of them (e.g. all platform specific overrides), and having a nagging message that one should restart the editor after you change the first, when you're meant to change several of them, might be a bit annoying. But maybe it's fine.

Well, editor not closing forcefully, only suggest to restart, and if Godot have platform specific preprocessors we could just define restartable options only on current platform, we could ofk just platform check with default if, but I think this will be unnecessary check

@akien-mga akien-mga merged commit 2e109b1 into godotengine:master Sep 24, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@KoTeYkA23 KoTeYkA23 deleted the render-selector-restart-fix branch September 25, 2023 08:11
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.

4 participants