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

GLES3: Allow repeat flag in viewport textures #34008

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

ricmzn
Copy link
Contributor

@ricmzn ricmzn commented Nov 29, 2019

This patch changes the texture_set_flags function in the GLES3 renderer to allow the use of the "Repeat" flag for render targets, which can fix visible seams when using viewports to generate procedural textures.

My setup for this is to generate a tiled noise texture around a sphere via the tutorial outlined here, reducing the viewport resolution to make the effect more visible, and enabling the filter and repeat flags in the texture. Notably, the repeat flag doesn't have any effect for viewport textures as of Godot 3.2.beta2.

Before applying the patch, enabling texture filtering creates a visible seam at the edges of the texture, where it wraps around the object:

editor_screenshot_2019-11-29T183148-0300

But with the patch, the repeat flag works as it should and gets rid of the seam:

editor_screenshot_2019-11-29T183226-0300

I believe there could be a reason for the flags to be set up as they were originally, however, I'm not very knowledgeable on OpenGL or Godot's rendering internals (it took me changing texture flags in many places and several compiles to nail the issue down!).

@clayjohn
Copy link
Member

clayjohn commented Dec 1, 2019

The change looks good. However, we are in feature freeze for the 3.2 release and so we won't be merging any new features (even small ones like this) because it risks introducing bugs.

Unfortunately, that means that this PR will become irrelevant because 3.2 is the last release to have the GLES3 backend. 4.0 will have Vulkan and GLES2.

For now, I am setting the milestone to 4.0. However, for it to be merged it will need to be updated to work on both Vulkan and GLES2.

@clayjohn clayjohn added this to the 4.0 milestone Dec 1, 2019
@Calinou
Copy link
Member

Calinou commented Dec 1, 2019

@clayjohn I guess it could still be merged in 3.2.1. We tend to allow small features like this one in point releases.

@aaronfranke
Copy link
Member

@ricmzn Is this still desired? If so, it needs to be rebased on the latest master branch and redone to work with Vulkan.

@ricmzn
Copy link
Contributor Author

ricmzn commented Jul 2, 2020

@aaronfranke I'm not sure if this fix would be applicable to Vulkan, as this whitelists a specific texture flag that's only ignored in the GLES3 renderer. As such, this would only be applicable for future 3.X releases.

I would have to check if the same thing happens on the Vulkan renderer on master, and if it does, both the cause and the fix would be completely different, especially as my use case (ie. procedurally generated sky textures in shaders) has also been given some attention.

Would it be better to change the target branch of this PR to accommodate the fact it works on a deprecated renderer in maintenance mode?

@ricmzn
Copy link
Contributor Author

ricmzn commented Dec 15, 2020

Hello, I know this is an old PR, but I believe it can still be merged into 3.2, as it's not pertinent to the renderer rewrite in 4.0.

Or, if the team believes it would be a better approach, this could be closed as the 3.2 branch is in maintenance mode.

@Calinou Calinou changed the base branch from master to 3.2 December 15, 2020 21:29
@Calinou
Copy link
Member

Calinou commented Dec 15, 2020

Or, if the team believes it would be a better approach, this could be closed as the 3.2 branch is in maintenance mode.

The 3.2 branch is still maintained until 4.0 is released, if not more 🙂

@ricmzn ricmzn requested a review from a team as a code owner March 12, 2021 12:26
Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

I think this is harmless for 3.3 as it only allows a new flag (rather than changing existing behavior).

@akien-mga akien-mga modified the milestones: 4.0, 3.3, 3.4 Mar 23, 2021
@akien-mga akien-mga merged commit 135fd8f into godotengine:3.x Apr 28, 2021
@akien-mga
Copy link
Member

Thanks!

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