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

Adding tile id to tile #164

Closed
wants to merge 1 commit into from

Conversation

Anti-Alias
Copy link
Contributor

I'd like for the Tile struct to include it's own id.
For my personal project, I need to know the tile id in order to derive it's coordinates in the tileset it belongs to.

IE:

let id = tile.id;
tile_x: uint = id % tileset.columns;
tile_y: uint = id / tileset.columns;
// Draw code

@Anti-Alias
Copy link
Contributor Author

Anti-Alias commented Feb 20, 2022

Ultimately, what I need (and what others are likely to need) are the XY coordinates of a tile in it's TileSet so that its UV coordinates can be derived for rendering.

It might be better for the Tile to simply store it's XY coordinate and or UV coordinates.
Of course, all can be derived from the tile id, but it introduces more arithmetic in the rendering code than necessary unless the programmer generates a mesh during load time instead of using a simple Batch.

But what do you all think?

@aleokdev
Copy link
Contributor

aleokdev commented Feb 20, 2022

No; If you have access to Tile, you should have had access to its ID previously. The ID is the key of the tile in the hashmap, that's why it isn't in the actual Tile object. In other words: The ID of a tile is always stored along with it, not inside of it. See LayerTile for instance.

As for UV coordinates, we should probably add a helper in Tileset.

@aleokdev
Copy link
Contributor

Of course, all can be derived from the tile id,

This is not precise; You can't obtain the Tile's UV just from its ID. That's assuming that the tileset isn't an image collection tileset and it has no padding nor margin.

I do agree that obtaining the UV is a complex issue that shouldn't be left to the user. As such, adding the tile ID is not a solution nor a workaround to the issue.

That being said, I'll prioritize having UVs ready for today if possible; Thanks for using the master branch and reporting on everything that's missing!

@aleokdev aleokdev closed this Feb 20, 2022
@Anti-Alias
Copy link
Contributor Author

I have no idea how I missed LayerTile's id() function....
Just realized that this PR wasn't strictly necessary.

Thanks for using the master branch and reporting on everything that's missing!

Of course! Always good to have code battle tested.

This is not precise; You can't obtain the Tile's UV just from its ID. That's assuming that the tileset isn't an image collection tileset and it has no padding nor margin.

I should have clarified: While I'm iterating a TileLayer<'map>, I acquire a LayerTile<'map> and it's Tileset reference. With both of these, I can derive the UVs. My code does do the margin/spacing calculation. My code also ignores image collection tilesets as they're not yet supported.

That being said, I'll prioritize having UVs ready for today if possible;

Awesome! I discovered that the UV calculation is quite the involved process and easy to mess up. This would definitely make things easier for a ton of folks, I imagine.
Out of curiosity, will the UV calculation be configurable?
IE:

flip_y: bool

The reason I ask is that I'm using Bevy, which uses OpenGL-style UVs, where (0, 0) is bottom left, (1, 1) is top-right.
Some other engines might use DirectX, Metal or Vulkan UVs, where (0, 0) is top left, and (1, 1) is bottom-right, I believe.

@aleokdev
Copy link
Contributor

Since UVs depend on the API as you said, it's probably much better to just provide a tile_texture_rect function that returns the result in pixels which the user can then convert into their desired coordinates. Let's keep track of this issue in #166

@Anti-Alias Anti-Alias deleted the add-tile-facade-2 branch February 20, 2022 21:12
@aleokdev aleokdev added this to the 0.10.0 milestone Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants