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

Fix bad parameter for rendering_method crashes Godot #84241

Conversation

jsjtxietian
Copy link
Contributor

Print error and default to forward plus. Prevent it crash is always necessary I suppose, but maybe we can default to other rendering method?

Fixes #69815

@jsjtxietian jsjtxietian requested a review from a team as a code owner October 31, 2023 10:00
@AThousandShips AThousandShips added bug topic:rendering crash cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Oct 31, 2023
@AThousandShips AThousandShips added this to the 4.3 milestone Oct 31, 2023
@jsjtxietian jsjtxietian force-pushed the fix-render-method-crash-on-bad-input branch from c6c9765 to 08e36dc Compare October 31, 2023 11:13
@akien-mga
Copy link
Member

In my previous review comment, I also suggested doing this, to be consistent:

diff --git a/servers/rendering/renderer_rd/renderer_compositor_rd.cpp b/servers/rendering/renderer_rd/renderer_compositor_rd.cpp
index b9bda9329e..d09cd8a615 100644
--- a/servers/rendering/renderer_rd/renderer_compositor_rd.cpp
+++ b/servers/rendering/renderer_rd/renderer_compositor_rd.cpp
@@ -310,10 +310,10 @@ RendererCompositorRD::RendererCompositorRD() {
 	uint64_t textures_per_stage = RD::get_singleton()->limit_get(RD::LIMIT_MAX_TEXTURES_PER_SHADER_STAGE);
 
 	if (rendering_method == "mobile" || textures_per_stage < 48) {
-		scene = memnew(RendererSceneRenderImplementation::RenderForwardMobile());
 		if (rendering_method == "forward_plus") {
 			WARN_PRINT_ONCE("Platform supports less than 48 textures per stage which is less than required by the Clustered renderer. Defaulting to Mobile renderer.");
 		}
+		scene = memnew(RendererSceneRenderImplementation::RenderForwardMobile());
 	} else if (rendering_method == "forward_plus") {
 		// default to our high end renderer
 		scene = memnew(RendererSceneRenderImplementation::RenderForwardClustered());

First we warn, then we instantiate the fallback.

@akien-mga akien-mga changed the title Fix bad parameter of rendering_method crashes Godot Fix bad parameter for rendering_method crashes Godot Nov 9, 2023
Print error and default to forward plus
@jsjtxietian jsjtxietian force-pushed the fix-render-method-crash-on-bad-input branch from 08e36dc to b6bee1c Compare November 10, 2023 02:38
@akien-mga akien-mga merged commit 7113050 into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@jsjtxietian jsjtxietian deleted the fix-render-method-crash-on-bad-input branch December 9, 2023 16:40
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
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.

renderer/rendering_method=2 in project config crashes Godot
4 participants