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

Added texture offset for entire TileMap2D layer #79632

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bgie
Copy link
Contributor

@bgie bgie commented Jul 18, 2023

This PR adds a simple texture offset for entire layers, allowing isometric/hex 2.5D worlds to be created with a layered tilemap.
Implements proposal 6715

I would argue this could even be considered a bugfix, since there is an inconsistency right now:

  • Individual tiles in the tileset have a texture origin AND a y-sort offset.
  • Layers only have a y-sort offset for the entire layer but no way to offset the textures.

Since the texture origin on individual tiles moves just the texture, and not collision masks/occlusions/navigation/etc, I assumed the texture offset for the entire layer should behave the same. This makes sense for the game I am making, when stacking blocks for a fake 3d voxel look, I want the collision shapes on the ground level and not offset by height.

I tested my code with the attached project.
The top row is non y-sorted and I used a quadrant of size 3 on purpose, this obviously fails but my changes do not make it worse.
The bottom row uses y-sorting.
screenshot-tilemaps-noysort-and-ysort

tilemap_test_project.zip

EDIT: I also consider this change low-risk of breaking existing projects, as I have simply added a Vector2i to the render position of each tile. If this feature is not used, the vector will be (0,0) and nothing will change.

@bgie bgie requested a review from a team as a code owner July 18, 2023 19:42
@KoBeWi KoBeWi added this to the 4.x milestone Jul 18, 2023
@bgie bgie marked this pull request as draft July 18, 2023 20:11
@bgie bgie force-pushed the tilemap_layer_offset branch from a3f3147 to 14bc0ca Compare July 18, 2023 20:28
@bgie bgie marked this pull request as ready for review July 18, 2023 20:31
@bgie bgie requested a review from a team as a code owner July 18, 2023 20:31
@bgie bgie marked this pull request as draft July 19, 2023 19:26
@bgie
Copy link
Contributor Author

bgie commented Jul 19, 2023

Putting this issue back in draft, as I have noticed that tile placement in the editor should probably use the offset too.
(the transparent tile hovering under the mouse while placing tiles)

@bgie
Copy link
Contributor Author

bgie commented Jul 25, 2023

After using this feature for a while, I came to the conclusion that for simplicity and re usability I ought to offset everything in a layer, not just the rendering of the texture. (ie. editor grid, placement of tiles with the mouse, collision shape, navigation, occlusion, ...)

I don't think the PR should be merged as-is.
But completing the feature is a lot more work (me being unfamiliar with the codebase).

Not sure if anyone is really waiting for this or whether it aligns with the core devs roadmap?
I'm willing to continue this but don't want to waste my time on something that doesn't get merged. Judging from the amount of PRs the maintainers are drowning in work and have to prioritize firmly.

For my use case, the simple texture offset per layer is sufficient, I'm using a patched build now.

@groud
Copy link
Member

groud commented Jul 25, 2023

Sorry for the late review. I think the feature might be interesting, it could work but the more we add features to layers, the more complex TileMaps become.

I think that something like godotengine/godot-proposals#7122 would probably be the way to go to avoid this complexity to grow too big.

@bgie
Copy link
Contributor Author

bgie commented Jul 25, 2023

I agree and think proposal 7122 is the way forward. The latest merge (conflicting with this PR) extracts layers as a private child class and is already moving in the direction of 7122. The abundant use of macros to dispatch a part of the tilemap api to the hidden layer classes to me indicates that the layers deserve to be first-class citizens and not just hidden helper objects.

I guess with 7122 you could get an offset for each layer basically for free, since they could be a node2D subclass?
Though I am not familiar enough with the code (yet) to make a proper assessment. I'll see if I can look into that.

By the way it was not my intent to put pressure on anyone regarding reviews.

As for this PR, I will probably keep using my patched version for a while, so I will keep this up for anyone else to use, until a proper solution is available on the main branch.

@bgie bgie force-pushed the tilemap_layer_offset branch 2 times, most recently from ee5e17f to 9b21470 Compare July 25, 2023 21:11
@YuriSizov
Copy link
Contributor

Make sure to use the doctool. You seem to have manually edited the documentation XML and put methods in an incorrect order. So your PR doesn't pass the CI.

@bgie bgie force-pushed the tilemap_layer_offset branch from 9b21470 to 6e8bcf6 Compare July 26, 2023 20:29
@bgie
Copy link
Contributor Author

bgie commented Jul 27, 2023

Make sure to use the doctool. You seem to have manually edited the documentation XML and put methods in an incorrect order. So your PR doesn't pass the CI.

It was git mergetool during the rebase. Thank you!

@bgie bgie force-pushed the tilemap_layer_offset branch from 6e8bcf6 to 0d254e8 Compare August 7, 2023 16:38
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.

Add Tilemap Layer offset for Isometric/2.5D/2D depth uses
5 participants