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

Fixed white image in margins when using same image in scene #40169

Merged
merged 1 commit into from
Jul 8, 2020

Conversation

azagaya
Copy link
Contributor

@azagaya azagaya commented Jul 6, 2020

Fixes white margins discussed in here #40135

@azagaya azagaya requested a review from reduz as a code owner July 6, 2020 18:59
@akien-mga akien-mga added this to the 3.2 milestone Jul 6, 2020
@akien-mga akien-mga requested review from clayjohn and lawnjelly July 6, 2020 19:06
@lawnjelly
Copy link
Member

Yes this is fine imo.

The problem is that white is being set directly with a glBindTexture, and thus invalidating state.current_tex. So to set the texture either needs calling glBindTexture directly, or forcing as in the PR.

I would double check that this isn't necessary in GLES2 as well. Check both with batching switched on and off. I'd be tempted to use the same technique in GLES2 irrespective of whether it is currently needed, because the bug may well crop up again in GLES2, and may only be working there 'by accident', so a pre-emptive approach may be wise (welcome opinion from @clayjohn / @reduz on that).

@azagaya
Copy link
Contributor Author

azagaya commented Jul 7, 2020

@lawnjelly hey! The _bind_canvas_texture method does not have the force parameter in GLES2, so i cannot implement the same approach, unless i add that parameter myself?

@lawnjelly
Copy link
Member

Imo it would probably be ok with to leave it for now and keep an eye on GLES2 to see if that situation does occur in the wild before changing it. However as always would welcome other views, I'm not strongly opinionated one way or the other.

@akien-mga
Copy link
Member

Let's merge as is and we can iterate later if more changes are needed.

It only impacts the non-default code path where margin textures are used, and seems to fix bugs, so it's low risk :)

@akien-mga akien-mga merged commit 2eb65b5 into godotengine:3.2 Jul 8, 2020
@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.

3 participants