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

[3.x] Sprite3D/AnimatedSprite3D Fix drawing AtlasTextures with vertical margins differently than in 2D #66063

Merged

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Sep 18, 2022

Fixes #63520.

The actual fix is a single line of code added in the second commit, I've explained it in the comments. Exactly what I suggested in #63520 (comment) (took me that long as I somehow forgot about it).

The first commit is moving common/duplicated drawing code from Sprite3D/AnimatedSprite3D to SpriteBase3D. Drawing Sprite3D/AnimatedSprite3D is the same besides the part determining what texture and rects to use.
If doing so is not desired I guess I can instead just add the exact same changes from the second commit to the Sprite3D/AnimatedSprite3D separately (making the duplicated code even longer). Or if separate PRs are desired I could split that too.

BTW currently drawing nested AtlasTextures (an AtlasTexture having its atlas set to another AtlasTexture) doesn't work for Sprite3D/AnimatedSprite3D (works for 2D counterparts) so the fix was obviously not tested for such cases. Supporting such nesting is for potential future PRs.

Some examples for animated sprites 2D/3D (didn't bother about texture import settings):

SpriteBase3DVerticalMarginBug.zip

Before:
(3.5.stable)
After:
(this PR)
2D (no change) Z33mFdVmst l4QWb9cjUM
3D GtMgDQXG7c FLgqbxBptU

MRP from #63520.zip

Before:
(3.5.stable)
After:
(this PR)
2D (no change) offset = (30, 150)
IcDRsGzdqs
(debug border drawn in script)
offset = (30, 150)
yLAf2SkkYv
(debug border drawn in script)
3D offset = (30, 100)
(changed by the user to visually match 2D case)
NZRjggVQMN

offset = (30, 150)
j2aMepkwNh
offset = (30, 150)
TQDzDjOPHS

@kleonc kleonc added this to the 3.6 milestone Sep 18, 2022
@kleonc kleonc requested a review from a team as a code owner September 18, 2022 18:28
@kleonc kleonc added the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Sep 18, 2022
@kleonc
Copy link
Member Author

kleonc commented Sep 18, 2022

Marking for cherry-pick as it's a bugfix but note it's possible that some users use these only in 3D and thus they could have setup things according to the previous buggy behavior. Meaning if someone uses AtlasTextures with vertical margins (so I'd guess not so common case) and has tweaked some settings (like offset etc.) to make it look visually according to their needs then this PR will break things for them (they would need to change their setup). So it should be clearly pointed out in the change log if cherry-picked.

@akien-mga akien-mga merged commit edc196f into godotengine:3.x Sep 20, 2022
@akien-mga
Copy link
Member

Thanks!

@kleonc kleonc deleted the sprite3d-fix-drawing-with-vertical-margins-3x branch September 20, 2022 23:03
@timothyqiu
Copy link
Member

Cherry-picked for 3.5.2

@timothyqiu timothyqiu removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Dec 5, 2022
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