Skip to content

Add warnings for unsupported features in mobile and gl_compatibility backends#73959

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
clayjohn:GL-mobile-warnings
Feb 26, 2023
Merged

Add warnings for unsupported features in mobile and gl_compatibility backends#73959
akien-mga merged 1 commit into
godotengine:masterfrom
clayjohn:GL-mobile-warnings

Conversation

@clayjohn

@clayjohn clayjohn commented Feb 26, 2023

Copy link
Copy Markdown
Member

Closes #55880

This PR does a few things:

  1. Adds compatibility warnings when nodes that are unsupported are used
  2. Adds WARN_PRINT_ONCE_ED messages when features that are unsupported are used
  3. Removes the code to hide "high end" features in Environment and BaseMaterial
  4. Changes the default Environment to disable glow when using the gl_compatibility backend
  5. Fixes the description of Vsync modes in class reference

@BastiaanOlij and I discussed and agreed that we should stop hiding "high end" features in the editor as it was often creating an awkward situation for users where they would enable something, switch to the low end backend, and then they couldn't disable the thing without switching back. Similarly, we often got reports about missing features because the feature didn't appear in the editor. Having a toast pop up with a warning is preferable as it clearly communicates the lack of the feature and allows users to turn the feature off if it is turned on.

@clayjohn clayjohn added this to the 4.0 milestone Feb 26, 2023
@clayjohn clayjohn requested review from a team as code owners February 26, 2023 00:32
@clayjohn clayjohn requested a review from a team February 26, 2023 00:32
@clayjohn clayjohn requested a review from a team as a code owner February 26, 2023 00:32
BastiaanOlij
BastiaanOlij reviewed Feb 26, 2023
View reviewed changes
Comment thread doc/classes/DisplayServer.xml Outdated
BastiaanOlij
BastiaanOlij reviewed Feb 26, 2023
View reviewed changes
Comment thread servers/rendering/storage/environment_storage.cpp Outdated

@BastiaanOlij BastiaanOlij Feb 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We might want to add here that if fog is enabled and multiview shaders are enabled, that we note that volumetric fog does not yet work in stereo rendering as well.
Something I still have on my list of tasks

BastiaanOlij
BastiaanOlij approved these changes Feb 26, 2023
View reviewed changes

@BastiaanOlij BastiaanOlij left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks really good, sorry I didn't get to it first :)

akien-mga
akien-mga approved these changes Feb 26, 2023
View reviewed changes

@akien-mga akien-mga left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pushed a small update to change "GL_Compatibility" to "GL Compatibility", since it's how it is shown in the editor.

And added missing RTR for a bunch of configuration warnings.

@akien-mga

akien-mga commented Feb 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Tested, seems to work well aside from this which is a bit annoying: Any GL compatibility project (even empty, without any custom WorldEnvironment) prints this:

WARNING: DoF blur is only available when using the Forward+ or Mobile rendering backends.
     at: camera_attributes_set_dof_blur (servers/rendering/storage/camera_attributes_storage.cpp:70)

Seems to come from the default value of CameraAttributesPhysical in the editor doc generation:

#0  RendererCameraAttributes::camera_attributes_set_dof_blur (this=0xc5d7ef0, p_camera_attributes=..., p_far_enable=false, p_far_distance=10, p_far_transition=-1, p_near_enable=true, p_near_distance=10, 
    p_near_transition=-1, p_amount=0.00153662823) at servers/rendering/storage/camera_attributes_storage.cpp:70
#1  0x000000000989c544 in RenderingServerDefault::camera_attributes_set_dof_blur (this=0xc5efd50, p1=..., p2=false, p3=10, p4=-1, p5=true, p6=10, p7=-1, p8=0.00153662823)
    at servers/rendering/rendering_server_default.h:742
#2  0x0000000008dd3b77 in CameraAttributesPhysical::_update_frustum (this=0x7fff9800a8d0) at scene/resources/camera_attributes.cpp:397
#3  0x0000000008dd5c39 in CameraAttributesPhysical::CameraAttributesPhysical (this=0x7fff9800a8d0) at scene/resources/camera_attributes.cpp:487
#4  0x000000000835480e in ClassDB::creator<CameraAttributesPhysical> () at ./core/object/class_db.h:142
#5  0x000000000a4660e9 in ClassDB::instantiate (p_class=...) at core/object/class_db.cpp:339
#6  0x000000000a46e11f in ClassDB::class_get_default_property_value (p_class=..., p_property=..., r_valid=0x7fffffff7407) at core/object/class_db.cpp:1464
#7  0x000000000738af28 in get_documentation_default_value (p_class_name=..., p_property_name=..., r_default_value_valid=@0x7fffffff7407: false) at editor/doc_tools.cpp:346
#8  0x000000000738ba44 in DocTools::generate (this=0xdf3af70, p_basic_types=true) at editor/doc_tools.cpp:453
#9  0x00000000074bb2ad in EditorHelp::generate_doc () at editor/editor_help.cpp:2190
#10 0x00000000075970e9 in EditorNode::EditorNode (this=0xdebb200) at editor/editor_node.cpp:6625
#11 0x000000000533ded7 in Main::start () at main/main.cpp:2795
#12 0x00000000052ba699 in main (argc=2, argv=0x7fffffffd6f8) at platform/linuxbsd/godot_linuxbsd.cpp:71

@clayjohn

clayjohn commented Feb 26, 2023

Copy link
Copy Markdown
Member Author

Force pushed with the addition of:

#ifdef DEBUG_ENABLED
	if (OS::get_singleton()->get_current_rendering_method() == "gl_compatibility") {
		// Force disable DoF in editor builds to suppress warnings.
		use_far = false;
		use_near = false;
	}
#endif

To CameraAttributesPhysical::_update_frustum(). This force disables using DoF from within CameraAttributesPhysical which is kind of ugly, but I think is the best way to disable the warning without refactoring CameraAttributesPhysical

@akien-mga akien-mga merged commit 0cd1483 into godotengine:master Feb 26, 2023
@akien-mga

akien-mga commented Feb 26, 2023

Copy link
Copy Markdown
Member

Thanks!

Iniquitatis
Iniquitatis reviewed Feb 27, 2023
View reviewed changes

@Iniquitatis Iniquitatis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpicks.

Comment thread drivers/gles3/storage/material_storage.cpp
}

if (uses_sss) {
WARN_PRINT_ONCE_ED("Sub-surface scattering is only available when using the Forward+ rendering backend.");

@Iniquitatis Iniquitatis Feb 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
WARN_PRINT_ONCE_ED("Sub-surface scattering is only available when using the Forward+ rendering backend.");
WARN_PRINT_ONCE_ED("Subsurface scattering is only available when using the Forward+ rendering backend.");

Comment thread servers/rendering/renderer_rd/forward_mobile/scene_shader_forward_mobile.cpp

#ifdef DEBUG_ENABLED
if (uses_sss) {
WARN_PRINT_ONCE_ED("Sub-surface scattering is only available when using the Forward+ rendering backend.");

@Iniquitatis Iniquitatis Feb 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
WARN_PRINT_ONCE_ED("Sub-surface scattering is only available when using the Forward+ rendering backend.");
WARN_PRINT_ONCE_ED("Subsurface scattering is only available when using the Forward+ rendering backend.");

@clayjohn clayjohn deleted the GL-mobile-warnings branch February 27, 2023 19:58
@clayjohn

clayjohn commented Feb 27, 2023

Copy link
Copy Markdown
Member Author

@Iniquitatis This PR has already been merged, feel free to open another PR with the changes you have suggested!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BastiaanOlij BastiaanOlij BastiaanOlij approved these changes

@akien-mga akien-mga akien-mga approved these changes

+1 more reviewer

@Iniquitatis Iniquitatis Iniquitatis left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement topic:editor topic:rendering usability

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

Vulkan Mobile backend: Warn about unsupported features in the editor and class reference

4 participants

@clayjohn @akien-mga @BastiaanOlij @Iniquitatis