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

Check RenderingDevice availability to display LightmapGI configuration warnings #97416

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Sep 24, 2024

We can now check whether RenderingDevice can be created (which is not guaranteed when using the Compatibility rendering method), so the warning can be displayed only when relevant.

This also disables the Bake Lightmaps button with a tooltip if baking is not available.

Preview

Ignore the GPU model name (it obviously supports Vulkan), I just changed the condition to get the dialog to show up.

Screenshot_20240924_184651

Edit: Now displays dedicated warnings if lightmapper_rd was disabled at compile-time, including a specific message for mobile platforms.

Screenshot_20240928_233734

Screenshot_20240928_234202

@Saul2022
Copy link

This look’s great though i think maybe an additional comment should be added for the android editor when trying to bake since it would confuse people why they can’t bake lightmap gi if they can use the vulkan mobile backend.

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.

How often is get_configuration_warnings called? Its after every property change right? If so, we need to cache the result of can_create_rendering_device() somewhere or else this is going to make things very slow

scene/3d/lightmap_gi.cpp Outdated Show resolved Hide resolved
@Calinou Calinou force-pushed the lightmapgi-check-rendering-device-availability branch from 5d7e758 to 88a0ab6 Compare September 28, 2024 21:45
@Calinou
Copy link
Member Author

Calinou commented Sep 28, 2024

This look’s great though i think maybe an additional comment should be added for the android editor when trying to bake since it would confuse people why they can’t bake lightmap gi if they can use the vulkan mobile backend.

Done, see OP for new screenshots.

How often is get_configuration_warnings called? Its after every property change right? If so, we need to cache the result of can_create_rendering_device() somewhere or else this is going to make things very slow

I've just checked by benchmarking how time it takes to call DisplayServer::get_singleton()->can_create_rendering_device() 1000 times, and it takes 1 microsecond or less on a debug editor build. The method is already cached in its base implementation, as the result of this method can't change during a session.

Node::get_configuration_warnings() is called once per LightmapGI node present in the current scene tree when the tree changes or when the scene is reloaded, so it's not called that often anyway.

@Calinou Calinou force-pushed the lightmapgi-check-rendering-device-availability branch from 88a0ab6 to 0407618 Compare September 28, 2024 21:56
…n warnings

We can now check whether RenderingDevice can be created (which is
not guaranteed when using the Compatibility rendering method),
so the warning can be displayed only when relevant.

This also disables the Bake Lightmaps button with a tooltip if baking
is not available.
@Calinou Calinou force-pushed the lightmapgi-check-rendering-device-availability branch from 0407618 to 0807d60 Compare September 28, 2024 21:56
akien-mga
akien-mga previously approved these changes Oct 1, 2024
@akien-mga
Copy link
Member

I've just checked by benchmarking how time it takes to call DisplayServer::get_singleton()->can_create_rendering_device() 1000 times, and it takes 1 microsecond or less on a debug editor build. The method is already cached in its base implementation, as the result of this method can't change during a session.

Did you test this with the Compatibility renderer? That's where it might be expensive.
If there's already a RenderingDevice then it's basically just checking the singleton's existence so that would indeed be fast.

@Calinou
Copy link
Member Author

Calinou commented Oct 1, 2024

Did you test this with the Compatibility renderer? That's where it might be expensive.

Indeed, I just tried and it takes much longer to run there (easily over 5 minutes for 1,000 calls). Caching this result avoids the issue entirely: #97698

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.

Looks great! Thanks for taking a look at the performance. This is good to merge after #97698

@akien-mga akien-mga merged commit 1da8a2a into godotengine:master Oct 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the lightmapgi-check-rendering-device-availability branch October 2, 2024 14:34
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.

5 participants