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

Incorporating the availability of screen and depth textures for the GLES3 backend #72361

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

the-brickster
Copy link
Contributor

@the-brickster the-brickster commented Jan 30, 2023

The PR is for the implementation of SCREEN_TEXTURE and DEPTH_TEXTURE for the GLES3 backend.

The link for the testbed for this PR can be found here: https://github.com/the-brickster/Godot4Testbed/tree/master

@the-brickster the-brickster requested review from a team as code owners January 30, 2023 07:27
SConstruct Outdated Show resolved Hide resolved
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Generally looks good, just need to remove the changes to the build files and undo the break in projection matrix values.

If multiview could be taken along that would be a bonus, but we can add that later as well.

@BastiaanOlij
Copy link
Contributor

cc @clayjohn @dsnopek

@akien-mga
Copy link
Member

This seems to go directly against #70967...?

@BastiaanOlij
Copy link
Contributor

This seems to go directly against #70967...?

Ah I overlooked that, most of this PR is still valid but it indeed reintroduces the renames.

So yeah, @the-brickster we don't need to re-add actions.renames["SCREEN_TEXTURE"] and related things. There is a new way of doing this by adding a uniform with a hint in your shader.

So in your test project, in your shader simply add:

uniform sampler2D my_screen_texture : hint_screen_texture; 

Have a look at how things have changed in the Vulkan renderer if you're not sure. The PR Akien mentions should provide you all the further background.

@the-brickster
Copy link
Contributor Author

Ahh ok, so that's why I needed to change it in my test project. I will get the required changes within the next day.

@the-brickster
Copy link
Contributor Author

This seems to go directly against #70967...?

I have integrated those changes into my PR

@Riteo
Copy link
Contributor

Riteo commented Feb 2, 2023

@the-brickster I don't get it... How can this PR be "fixed" if the whole concept of SCREEN_TEXTURE and DEPTH_TEXTURE has been scrapped in #70967?

@the-brickster
Copy link
Contributor Author

the-brickster commented Feb 2, 2023

@the-brickster I don't get it... How can this PR be "fixed" if the whole concept of SCREEN_TEXTURE and DEPTH_TEXTURE has been scrapped in #70967?

It hasn't been scrapped per se, it's just been replaced in favor of using hint_depth_texture and hint_screen_texture, you can confirm this by using the following test project I use:
https://github.com/the-brickster/Godot4Testbed/tree/master
Within the scene splat_test.tscn

@aaronfranke
Copy link
Member

@the-brickster To avoid confusion, you can edit the title and description of the PR. It would also be a good idea to include a link to the test project you're using in the PR description.

@Riteo
Copy link
Contributor

Riteo commented Feb 2, 2023

@the-brickster

It hasn't been scrapped per se, it's just been replaced in favor of using hint_depth_texture and hint_screen_texture

Oh all right. Makes sense then. I'd change the title like aaron proposed since otherwise things get pretty confusing.

@the-brickster the-brickster changed the title Implementing SCREEN_TEXTURE and DEPTH_TEXTURE for GLES3 backend Incorporating the availability of screen and depth textures for the GLES3 backend Feb 2, 2023
@the-brickster
Copy link
Contributor Author

@aaronfranke @Riteo title changed and testbed project added

@the-brickster the-brickster requested review from BastiaanOlij and akien-mga and removed request for BastiaanOlij and akien-mga February 2, 2023 08:53
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.

Running the code locally did not work with this current version. I had to make some changes myself to get it working. I have added those changes as suggestions.

Once those changes are applied and this PR is rebased it should be ready to go (and can be merged shortly after we release Godot 4.0).

drivers/gles3/storage/texture_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/rasterizer_scene_gles3.cpp Outdated Show resolved Hide resolved
@the-brickster
Copy link
Contributor Author

@clayjohn changes pushed and rebased against master

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.

This looks good to go now. We can go ahead and merge it shortly after releasing 4.0 with an aim to get it into 4.1 and perhaps cherry pick to 4.0.1

@YuriSizov YuriSizov merged commit fe0949e into godotengine:master Mar 27, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 27, 2023

Thanks! And congrats on your first merged pull request, I think! Ah, I was mistaken. But still, great work :)

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.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.

9 participants