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

Draw materials in tile atlas view #77909

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jun 6, 2023

Fixes #77672

godot.windows.editor.dev.x86_64_jduJMJS1OT.mp4

Test TileSet:
Shaderset.tres.txt
(requires icon.png)

There is also a separate (and unreported?) issue where the "ghost" tiles (i.e. when drawing on TileMap) are not using their material.

Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Soooo. I've been thinking about it a bit and I am a bit worried this cached materials pool could become big as it is only cleaned when changing sources. While most of the edits only trigger queue_redraw.

I would be more confident if the cache was cleaned dynamically in the drawing functions. The idea would be, when drawing, to keep track of the materials in the cache that were not used by any tile, then free the corresponding CanvasItems at the end of the drawing function.

Also, this needs to be implemented for _draw_alternatives, which means you probably need two HashMaps to store the CanvasItems.

editor/plugins/tiles/tile_atlas_view.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 6, 2023

Soooo. I've been thinking about it a bit and I am a bit worried this cached materials pool could become big as it is only cleaned when changing sources. While most of the edits only trigger queue_redraw.

I implemented what you said and then discovered that material change always results in set_atlas_source(), so removing unused materials is useless 🙄

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 6, 2023

Ok pushed fix for alternate tiles. I did not add removing unused materials for the reason described above.

@groud
Copy link
Member

groud commented Jun 7, 2023

I implemented what you said and then discovered that material change always results in set_atlas_source(), so removing unused materials is useless roll_eyes

hmm, that sounds like a bug though. I don't really understand why that would be triggered.
I guess it can be merged as is anyway, it's not a big deal.

@akien-mga akien-mga merged commit 0fbe906 into godotengine:master Jun 7, 2023
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the all_that_matterials branch June 7, 2023 09:50
@vmedea
Copy link
Contributor

vmedea commented Jun 8, 2023

Can confirm this solves #77672. This is so much better thank u!!!

image

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.4.

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.

Tiles in the tile picker aren't rendered with their material
6 participants