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

Tile collections #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Luca-spopo
Copy link

Works as expected.
In this screenshot, the blue regions are tiles from a tile collection, and the red regions are tiles from a batch sheet
Screenshot 2020-06-19 at 3 48 33 PM

@Luca-spopo
Copy link
Author

Linked issues:
#214
#173

@karai17
Copy link
Owner

karai17 commented Jun 19, 2020

I definitely appreciate the plugin but I feel like it's written in a bit of a strange way that could be simplified. Instead of adding a field called loadStyle, why not just check the loaded table to see if it has a __call function, and if so, execute it? In the same vain, instead of having a hook function, just assign it directly to __call. Tilesets don't really have any functionality, they are just data, so they don't need to have metatables, you can just process them directly. And finally, I don't think you need to create a new table for tilesets since you're just looping through them and assigning data as you go; you can use the same table that loaded in with the map.

@Luca-spopo
Copy link
Author

I'll refactor it later and update the PR.
Regarding reusing the tilesets table that loads with the map, what do you have in mind? Inserting elements into a table while iterator over it is sometimes unsafe, and I also don't know if the order of the tilesets needs to be preserved.

@karai17
Copy link
Owner

karai17 commented Jun 20, 2020 via email

@Sven65
Copy link

Sven65 commented Sep 12, 2020

Any progress on this?

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.

3 participants