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

Don't modify source TileMap TileSetAtlasSource when copying between scenes #83948

Closed
wants to merge 1 commit into from

Conversation

rakkarage
Copy link
Contributor

@rakkarage rakkarage commented Oct 25, 2023

Closes: #83218

This fixes the issue by effectively reversing some changes from #78477 for internal tilesets. and fixing it another way.

I am unsure exactly when it is the right time to delete the source from the old tilemap so maybe these conditions are wrong but seems to work.

EDIT: TODO: need to ensure this issue is not "unfixed". which it seems to be now? #77045 (ya condition must be wrong as that example uses internal tileset too.)

Thanks.

scene/resources/tile_set.cpp Outdated Show resolved Hide resolved
scene/resources/tile_set.cpp Outdated Show resolved Hide resolved
scene/resources/tile_set.cpp Outdated Show resolved Hide resolved
@groud
Copy link
Member

groud commented Oct 26, 2023

Hey, thanks for trying to fix the issue.

I am not sure this is the right way to fix the problem. Whatever happens, a TileSetSource should always be used by a single TileSet. I feel the issue is that copy-pasting a TileMap duplicates the TileSet resource while it should not, or something like that, because there's no reason copy-pasting a TileMap would cause its TileSet's sources to be duplicated.

@rakkarage
Copy link
Contributor Author

rakkarage commented Oct 26, 2023

Thanks.
Old fix seems to act as a toggle between this bug and that bug.
This seems to fix both. Removing old fix, and calling TileData.set_tile_set in TileMapLayer.get_cell_tile_data instead.

no reason copy-pasting a TileMap would cause its TileSet's sources to be duplicated.

I can understand why this is true for external (all point to same) TileSet, but not internal (each needs own)?
Idk.

@rakkarage rakkarage marked this pull request as ready for review October 26, 2023 16:19
@rakkarage rakkarage requested a review from a team as a code owner October 26, 2023 16:19
@rakkarage rakkarage requested a review from a team as a code owner October 26, 2023 23:18
@YuriSizov YuriSizov changed the title Don't modify source TileMap TileSetAtlasSource when copy from one… Don't modify source TileMap TileSetAtlasSource when copying between scenes Oct 27, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Oct 31, 2023

Copy pasting node will duplicate all built-in resources, because they can't be shared between scenes.

If the problem is that the new TileSet uses old sources, maybe the sources should be always duplicated? duplicate() is virtual, so TileSet can implement its own. Or they could use PROPERTY_USAGE_ALWAYS_DUPLICATE flag.

@groud
Copy link
Member

groud commented Nov 1, 2023

If the problem is that the new TileSet uses old sources, maybe the sources should be always duplicated? duplicate() is virtual, so TileSet can implement its own. Or they could use PROPERTY_USAGE_ALWAYS_DUPLICATE flag.

Oh ok, I did not know that there was a situation where duplicating a node would duplicate its resources automatically too. So, yes, that seems to be a good solution. PROPERTY_USAGE_ALWAYS_DUPLICATE may be enough I believe, but if it does not work, implementing a TileSet::duplicate would work too.

@rakkarage
Copy link
Contributor Author

"calling TileData.set_tile_set in TileMapLayer.get_cell_tile_data instead fixes both issues"? no?

How does old fix work to fix the null tile_set error in cell_tile_data? Delete atlas? Try doing this instead... there is no problem, it works fine.

Excuse my ignorance...
Thanks.

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Nov 12, 2023
@akien-mga
Copy link
Member

Superseded by #88280. Thanks for the contribution!

@akien-mga akien-mga closed this Feb 13, 2024
@AThousandShips AThousandShips removed this from the 4.3 milestone Feb 13, 2024
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.

Copy/Paste of TileMap from one scene to another removes TileSet from initial scene
5 participants