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

Expose copy_effects compute shader in Mobile backend #85793

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

Lasuch69
Copy link
Contributor

@Lasuch69 Lasuch69 commented Dec 5, 2023

This PR exposes compute version (the only one currently) of copy effects allowing for using some of the RenderingServer's functions on Mobile backend eg. sky_bake_to_panorama.

Baking Lightmaps on development machine works (not considering other bugs).

Note: I encourage to test it on your phone. This is a quick change/hack, feedback necessary.

Fixes: #55868

image

@AThousandShips AThousandShips added this to the 4.3 milestone Dec 5, 2023
@Lasuch69 Lasuch69 changed the title Expose copy_effects copy compute shader in Mobile backend Expose copy_effects compute shader in Mobile backend Dec 5, 2023
@Lasuch69
Copy link
Contributor Author

Lasuch69 commented Dec 5, 2023

I forgot about other checks too.

Edit: deleted them now.

@Lasuch69
Copy link
Contributor Author

Lasuch69 commented Dec 5, 2023

I can take a look at what functions (without raster version) can be exposed if not all of them need to be exposed.

Edit: At a first glance removing more checks was unnecessary, at least by looking at functions in the rendering server.

@Lasuch69
Copy link
Contributor Author

Lasuch69 commented Dec 6, 2023

The way I see it is that only copy_cubemap_to_panorama would be of any use. Should I leave other 2 functions as is (with a check that is)?

Note: I haven't tested it on a phone yet, I need to build android template.

@Lasuch69 Lasuch69 marked this pull request as ready for review December 6, 2023 11:54
@Lasuch69 Lasuch69 requested a review from a team as a code owner December 6, 2023 11:54
@clayjohn
Copy link
Member

clayjohn commented Dec 6, 2023

The way I see it is that only copy_cubemap_to_panorama would be of any use. Should I leave other 2 functions as is (with a check that is)?

Note: I haven't tested it on a phone yet, I need to build android template.

No need to add the checks back. They were there to ensure we didn't accidentally try to run a shader that wasn't compiled. Since we are now initializing those shaders anyway, we don't need the checks and its better not to have them

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 good to me!

Tested locally to confirm that it works in the test project

@clayjohn clayjohn added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 6, 2023
@Calinou
Copy link
Member

Calinou commented Dec 6, 2023

Note: I haven't tested it on a phone yet, I need to build android template.

The lightmapper_rd module isn't built on Android, so it won't work there either way.

@Lasuch69
Copy link
Contributor Author

Lasuch69 commented Dec 6, 2023

That's true however you can try to use copy_cubemap_to_panorama via RenderingServer.

@akien-mga akien-mga changed the title Expose copy_effects compute shader in Mobile backend Expose copy_effects compute shader in Mobile backend Dec 7, 2023
@YuriSizov YuriSizov merged commit 5c95fd5 into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@Lasuch69 Lasuch69 deleted the expose-compute branch December 8, 2023 17:49
@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
Development

Successfully merging this pull request may close these issues.

Vulkan Mobile backend: LightmapGI baking does not take environment lighting into account
5 participants