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

Fix region_filter_clip_enabled to avoid sprite bleeding for interpolated sprite sheets #97602

Merged

Conversation

patowen
Copy link
Contributor

@patowen patowen commented Sep 28, 2024

Fixes #87847

This PR modifies the fix done in #12828 to reintroduce the half_texpixel offset but fix the bug in the original implementation that led to #11417.

My understanding is that the intent of this code was originally to clamp the UV-coordinates to the centers of the outermost texels, which achieves the desired effect of avoiding any sprite bleeding caused by linear interpolation. I believe this was conceptually correct, but a math error resulted in #11417.

@patowen
Copy link
Contributor Author

patowen commented Sep 28, 2024

I marked this PR as a draft because I think it might be worthwhile to update the description of region_filter_clip_enabled in the same PR, since "the outermost pixels get blurred out" don't seem to accurately describe this attribute, but I haven't decided on the right wording for it yet (and am open to suggestions).

EDIT: I found existing documentation in AtlasTexture.xml for the same feature, so I updated the documentation in Sprite2D.xml to match.

@patowen patowen changed the title Clamp UV-coordinates to centers of outermost texels when configured to do so Fix region_filter_clip_enabled to avoid sprite bleeding for interpolated sprite sheets Sep 28, 2024
@patowen patowen marked this pull request as ready for review September 29, 2024 00:31
@patowen patowen requested review from a team as code owners September 29, 2024 00:31
@clayjohn clayjohn added this to the 4.4 milestone Sep 29, 2024
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 great! Thanks for updating the documentation as well.

I tested on the MRPs from #87847 and #11417 and confirm that this fixes the former without re-introducing the issue from the latter.

@clayjohn
Copy link
Member

The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

@clayjohn clayjohn added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 29, 2024
…o do so

In addition, fix region_filter_clip_enabled documentation to be consistent with AtlasTexture.xml, since that is the option whose behavior was fixed
@patowen patowen force-pushed the fix-region-filter-clip-enabled branch from fa15d2b to d720eb8 Compare September 29, 2024 09:59
@patowen
Copy link
Contributor Author

patowen commented Sep 29, 2024

Makes sense to me, thanks! I went ahead and squashed the commits.

@patowen
Copy link
Contributor Author

patowen commented Sep 29, 2024

Based on the latest build error, I believe the "Android / Template arm32" check needs to be rerun, as the failure seems intermittent and unrelated to my changes. I don't believe I have permission to do this myself.

EDIT: Thanks for rerunning the checks!

@akien-mga akien-mga merged commit 6c13305 into godotengine:master Oct 1, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@MeanRaccoon
Copy link

Cool, it's nice to see people working on the bugs we find eventually. Keep up!

@patowen patowen deleted the fix-region-filter-clip-enabled branch October 1, 2024 22:47
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Godot 4.1.1: Questioning today's usefulness of Sprite2D property "region_filter_clip_enabled"
5 participants