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

Use render method from OS instead of project settings in compositor RD #85387

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Nov 26, 2023

Fixes #85376

Not sure why Godot doesn't use the rendering method stored in OS in the first place, it's the correct one after all ( as it is computed in main.cpp ) . Feel free to correct me.

@RandomShaper
Copy link
Member

LGTM. There are other points in the codebase doing the same, though. I think all of them warrant revisiting. EditorNode may need some extra love, though, as it needs to handle the difference between an CLI-overridden and project-setting rendering method. For instance, iIt may want to sufix (overridden) to the renderer dropdown's caption, not allowing it to be further changed as it would then change the project setting.

@jsjtxietian
Copy link
Contributor Author

I think all of them warrant revisiting.

yes and I think I can do it in this pr to prevent further crashes.

EditorNode may need some extra love, though, as it needs to handle the difference between an CLI-overridden and project-setting rendering method.

That sounds like a usability improvement instead of fixing crash, and maybe can use a seperat pr.

Calinou
Calinou previously approved these changes Nov 27, 2023
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. This also fixes #85143 🙂

Forward+ project started with --rendering-method mobile:

Before (is actually using Forward+) After (correct appearance for Mobile)
image image

It also works the other way around (--rendering-method forward_plus on a Mobile project). --rendering-method gl_compatibility on a Forward+/Mobile project and --rendering-method <forward_plus|mobile> on a Comptaibility project also work.

PS: Should OS.get_current_rendering_method() also be used in the rendering dropdown in the top-right corner of the editor? Right now, the dropdown's current option will be incorrect if you use --rendering-method when opening the editor.

@RandomShaper
Copy link
Member

@Calinou, see my answer above. I think It'd be good to lock it if overridden to avoid confusion (if restarting the editor, is that one of the args that is carried over to new instances), or at least some other kind of special treatment.

clayjohn
clayjohn previously approved these changes Nov 27, 2023
@jsjtxietian jsjtxietian force-pushed the fix-forward-plus-crash branch from 830f294 to de0ad04 Compare November 28, 2023 06:46
@jsjtxietian
Copy link
Contributor Author

@RandomShaper I push a new commit to do what you said, if the rendering method is overriden then it will only show current one with a suffix overridden.

@clayjohn clayjohn dismissed stale reviews from Calinou and themself November 28, 2023 17:15

Out of date

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Good job. I've only added a comment about a refactor that would make the code more compact.

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@jsjtxietian jsjtxietian force-pushed the fix-forward-plus-crash branch from de0ad04 to 8c951bb Compare November 29, 2023 11:49
@DanielSnd
Copy link
Contributor

I have confirmed it works (and I'm using it in my game to allow for launching in compatibility through steam launch options with arguments)

editor/editor_node.cpp Outdated Show resolved Hide resolved
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.

Everything looks good to me.

I personally don't like introducing more lambdas into the codebase, but this one comes with Pedro's seal of approval :)

@clayjohn clayjohn added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 12, 2023
@jsjtxietian jsjtxietian force-pushed the fix-forward-plus-crash branch from 8c951bb to 453c224 Compare December 14, 2023 03:39
@jsjtxietian
Copy link
Contributor Author

Updated the code to use member function instead of lambda, squashed the commit.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Editor parts look fine to me, but we'll probably clean them up in the future.

@YuriSizov YuriSizov merged commit 4269a57 into godotengine:master Dec 14, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@jsjtxietian jsjtxietian deleted the fix-forward-plus-crash branch December 15, 2023 02:15
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants