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

Move TileMapLayer to its own files #85791

Merged

Conversation

groud
Copy link
Member

@groud groud commented Dec 5, 2023

As I mentioned in this proposal, I'd like to move TileMap layers to their own node. This fits the

I prefer to go there step by step. This first PR moves TileMap layer to another file.

The next steps would be:

  • Change TileMap layers to be used as an internal nodes for the TileMap node (and make sure everything works).
  • Implement a TileMapGroup node, that would constraint its child TileMapLayer nodes to use a given TileSet, and to keep their transform. It should have a very simple API. Keep TileMapGroup node private for now.
  • Make TileMap expand the TileMapGroup node API.
  • To ensure compatibility, implement a tool to transform the TileMap node into the (TileMapGroup + Children TileMap layers).
  • At the same time:
    • expose the TileMapLayer node.
    • expose the TileMapGroup node.
    • Change the editor to edit TileMapLayer directly (and, for simplicity of the implementation, probably disables it for the TileMap node. That kind of forces people to move the to new workflow by using the auto-conversion, but it's probably acceptable IMO)

Basically, the TileMap node could become the equivalent of (TileMapGroup + Children TileMap layers), with layers being internal nodes instead of being explicit ones.

@groud groud added this to the 4.x milestone Dec 5, 2023
@groud groud requested a review from a team as a code owner December 5, 2023 16:55
@AThousandShips
Copy link
Member

I'm a bit concerned by the circular inclusion, can this not be handled in any other way?

@groud groud force-pushed the move_tilemap_layers_to_own_file branch from 5b5c3fd to 1cef90d Compare December 5, 2023 17:16
@groud
Copy link
Member Author

groud commented Dec 5, 2023

I'm a bit concerned by the circular inclusion, can this not be handled in any other way?

I thought there was not, but I managed to solve it.

@groud groud force-pushed the move_tilemap_layers_to_own_file branch from 1cef90d to 6ad47e3 Compare December 5, 2023 17:19
scene/2d/tile_map_layer.h Outdated Show resolved Hide resolved
scene/2d/tile_map.h Outdated Show resolved Hide resolved
@groud groud force-pushed the move_tilemap_layers_to_own_file branch 2 times, most recently from 28b2659 to d131e7b Compare December 6, 2023 09:55
@YuriSizov
Copy link
Contributor

Needs a rebase and we can merge it soon if it unblocks your further work.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Dec 13, 2023
@groud
Copy link
Member Author

groud commented Dec 14, 2023

Needs a rebase and we can merge it soon if it unblocks your further work.

I'll rebase once #84660 is merged, to avoid conflicts.

@YuriSizov
Copy link
Contributor

@groud You should be able to rebase now!

@akien-mga
Copy link
Member

New Year's rebase poke ;)

@groud groud force-pushed the move_tilemap_layers_to_own_file branch from d131e7b to 6bc5b3f Compare January 5, 2024 10:58
@groud
Copy link
Member Author

groud commented Jan 5, 2024

New Year's rebase poke ;)

Done!

@akien-mga akien-mga merged commit 1e676e4 into godotengine:master Jan 5, 2024
15 checks passed
@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.

5 participants