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 issue in TextureAtlas import of images with wrong size. #42103

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

dankan1890
Copy link
Contributor

Fix #42057

@akien-mga akien-mga added this to the 4.0 milestone Sep 16, 2020
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 16, 2020
@akien-mga
Copy link
Member

For the reference, last commit on this code was #39271 (@RandomShaper).

@akien-mga
Copy link
Member

The fix seems to work fine, and I could reproduce the issue before applying it.

For the reference, last commit on this code was #39271 (@RandomShaper).

I checked and #39271 did not cause this issue though, it's already present in 3.2.1-stable.

@RandomShaper
Copy link
Member

I'm not sure this fix is totally correct. I think that if the <s are changed to <=s, the the - 1s applied to width and height - p_offset.y should be removed, since otherwise the cases where the right and/or bottom edges of the atlas are reached the last 1-pixel row or column would be left transparent.

However, I haven't tested it. and I'm not sure I'll have time reasonably soon. If one of you was able to test it, that'd help.

@akien-mga
Copy link
Member

However, I haven't tested it. and I'm not sure I'll have time reasonably soon. If one of you was able to test it, that'd help.

It does solve the problem in #42057, but it would likely be worth testing on more configurations to see how it behaves.

This code is not the easiest to parse and indeed, it seems a good spot for having off-by-one errors here and there which might counterbalance each other.

@boruok
Copy link
Contributor

boruok commented Sep 10, 2021

@akien-mga can this PR be merged in 3.x branch? compiled it with 3.3.3-stable and it does solve #42057 issue.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I've tested that the changes fix the bug last time - couldn't get anyone to review the code change in depth as it probably gives everyone a headache, me included. Let's merge and see if we spot any regression in testing builds.

@akien-mga akien-mga merged commit 2d1699e into godotengine:master Sep 20, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 20, 2021
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.

TextureAtlas bug with transparency.
4 participants