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] Fix AtlasTexture nesting #56795

Merged
merged 1 commit into from
Jan 15, 2022

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Jan 14, 2022

When drawing an AtlasTexture it was directly calling the VisualServer by itself instead of just forwarding the draw request to the source atlas texture (with the proper rect/region passed in). As a result an AtlasTexture being a source atlas of another AtlasTexture could have resulted in incorrect rendering. This should be fixed now (assuming other methods work as they're supposed to).

With this being fixed it means that pretty much nothing external should care if some Texture is actually an AtlasTexture as there should be no need to treat it as some special case where AtlasTexture's region and/or margin needs to be manually taken into account or something like that. If there's such special casing somewhere then it's very likely there's something wrong conceptually in there (as AtlasTexture should probably be handling this stuff by itself).

So for example fix in #37939 was conceptually wrong, the same as my fix in #51030 on top of it. I've kind of reverted them as such special casing is not needed in there (it was never needed in there, the bug was elsewhere). Now it should work properly.
Fixes #42806.
Fixes #56767.
Just note that for the frames already saved with the incorrect data above issues still hold.

@akien-mga akien-mga added this to the 3.5 milestone Jan 14, 2022
@akien-mga
Copy link
Member

I've also added a check to AtlasTexture::set_atlas() ensuring no cyclic dependency will be created between some AtlasTextures.

I once went down that rabbit hole and there's still a lot of horrible ways to trigger cyclic dependencies by mixing and matching proxy texture formats, e.g.:

var a1 = AtlasTexture.new()
var p1 = ProxyTexture.new()
p1.base = a1
a1.atlas = p1
	
var sp = Sprite.new()
sp.texture = a1
add_child(sp)

Basically any Texture type that uses another Texture type internally can easily create such cyclic dependencies.
Also MeshTexture, and probably AnimatedTexture, etc.

@kleonc
Copy link
Member Author

kleonc commented Jan 14, 2022

I've also added a check to AtlasTexture::set_atlas() ensuring no cyclic dependency will be created between some AtlasTextures.

I once went down that rabbit hole and there's still a lot of horrible ways to trigger cyclic dependencies by mixing and matching proxy texture formats, e.g.:

var a1 = AtlasTexture.new()
var p1 = ProxyTexture.new()
p1.base = a1
a1.atlas = p1
	
var sp = Sprite.new()
sp.texture = a1
add_child(sp)

Basically any Texture type that uses another Texture type internally can easily create such cyclic dependencies. Also MeshTexture, and probably AnimatedTexture, etc.

Maybe adding a method like virtual bool Texture::is_referenced_by(const Ref<Texture> &other_texture) overridden accordingly by AtlasTexture, ProxyTexture etc. and used in AtlasTexture::set_atlas, ProxyTexture::set_base etc. would solve that?

@kleonc kleonc force-pushed the atlas_texture_nesting_fix_3x branch from 11ba471 to 63fd172 Compare January 14, 2022 23:15
@kleonc
Copy link
Member Author

kleonc commented Jan 14, 2022

I've actually removed that cyclic dependency check as that wasn't really a proper solution, it was just checking one specific case. And a proper solution (whatever it will be) can be made in another PR.

@akien-mga akien-mga merged commit afadd3d into godotengine:3.x Jan 15, 2022
@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.

2 participants