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

Replace subpass textures with color in sky shader #37268

Merged
merged 1 commit into from Mar 24, 2020

Conversation

@clayjohn
Copy link
Contributor

clayjohn commented Mar 24, 2020

This changes the API of the Sky Shader a bit as discussed with reduz on IRC. Instead of giving the user access to the HALF_RES_TEXTURE and QUARTER_RES_TEXTURE directly. We perform the read for them and only allow them to access the output of the texture read. The reason for doing this is that it is the only way (that we can think of) to properly user cubemap texture lookups when doing the cubemap pass (this is needed for accurate blending across cubemap faces).

We can think about re-exposing the textures in the future if there is a need for it. As it stands, neither reduz or I could come up with a good reason to expose the subpass textures directly. Pretty much any use-case would be better served by passing in a ViewportTexture using a uniform.

I will update the blog post accordingly once this is merged.
@akien-mga Please ping reduz about this once he becomes active so we can merge ASAP and update the blog post before users start experimenting with the current API.

@clayjohn clayjohn added this to the 4.0 milestone Mar 24, 2020
@clayjohn clayjohn requested a review from reduz Mar 24, 2020
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Mar 24, 2020

BTW, not directly related to this new PR but it seems the shader cleanup does not work properly:

ERROR: 1 shaders of type SkyShaderRD were never freed
   at: ~ShaderRD (servers/visual/rasterizer_rd/shader_rd.cpp:489)
@clayjohn

This comment has been minimized.

Copy link
Contributor Author

clayjohn commented Mar 24, 2020

@akien-mga when are you getting that message? I thought I had gotten everything cleaned up properly.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Mar 24, 2020

@clayjohn When closing the editor. You can see it with godot --doctool . too.

@clayjohn

This comment has been minimized.

Copy link
Contributor Author

clayjohn commented Mar 24, 2020

@akien-mga Thanks, I was able to confirm it. It stems from a problem with all materials right now. When a material is freed, its corresponding shader isn't. This makes sense as shaders can be used by multiple materials. However, I'm not sure how reduz wants to handle it.

@reduz
reduz approved these changes Mar 24, 2020
@clayjohn clayjohn force-pushed the clayjohn:VULKAN-sky-color branch from daf792b to 61c67cd Mar 24, 2020
@clayjohn

This comment has been minimized.

Copy link
Contributor Author

clayjohn commented Mar 24, 2020

@akien-mga force push just adds the code to free the shader in ProceduralSkyMaterial. That will fix the memory leak you are seeing. :)

@akien-mga akien-mga merged commit 641c85a into godotengine:master Mar 24, 2020
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Mar 24, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.