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

feat: Adding configurable padding to Tiled atlas packing #2868

Merged
merged 5 commits into from
Nov 24, 2023

Conversation

erickzanardo
Copy link
Member

Description

Adds new configurations to Flame Tiled's atlas packing. Spacing which can be used to tilesets are not too close to each other in the atlas, something that can cause texture leaking causing weird rendering issues.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

Replace or remove this text.

@erickzanardo erickzanardo requested a review from a team November 23, 2023 17:40
@kurtome
Copy link
Contributor

kurtome commented Nov 24, 2023

@erickzanardo What do you think about refactoring these named params into a FlameTiledConfig struct? By my count we're at 12 named params here now, which seems like a good point to convert them into a single param that contains all the optional params.

Plus, then at that point we could use the same config struct in parts of the Leap API.

@erickzanardo
Copy link
Member Author

@erickzanardo What do you think about refactoring these named params into a FlameTiledConfig struct? By my count we're at 12 named params here now, which seems like a good point to convert them into a single param that contains all the optional params.

Plus, then at that point we could use the same config struct in parts of the Leap API.

Yeah, that might be a good idea, it will create a breaking change in flame tiled though... @spydon wdyt? We would probably have to bump flame tiled major version

@spydon
Copy link
Member

spydon commented Nov 24, 2023

Isn't the config usually only used once? Then it would seem to me like it isn't giving much compared to specifying the values in the constructor call, but I might be missing something?

@erickzanardo
Copy link
Member Author

Isn't the config usually only used once? Then it would seem to me like it isn't giving much compared to specifying the values in the constructor call, but I might be missing something?

It is! I feel the advantage in wrapping all those configs in a class is that it will improve our life a little bit, because those configurations needs to be passed down the tree in a couple of places, if all of them were in a single class, that would make adding new configs easier, as you just add it to the class and don't need to go keep passing it down the dependency tree.

But I am not sure if that is worth the big breaking change.

@kurtome
Copy link
Contributor

kurtome commented Nov 24, 2023

Isn't the config usually only used once? Then it would seem to me like it isn't giving much compared to specifying the values in the constructor call, but I might be missing something?

It is! I feel the advantage in wrapping all those configs in a class is that it will improve our life a little bit, because those configurations needs to be passed down the tree in a couple of places, if all of them were in a single class, that would make adding new configs easier, as you just add it to the class and don't need to go keep passing it down the dependency tree.

But I am not sure if that is worth the big breaking change.

Exactly, and I'm also not sure 😅

@spydon
Copy link
Member

spydon commented Nov 24, 2023

Maybe lets keep it like it is for now, but open an enhancement issue where we can discuss it further in the future and maybe bundle it together with more breaking changes? :)

@erickzanardo
Copy link
Member Author

Maybe lets keep it like it is for now, but open an enhancement issue where we can discuss it further in the future and maybe bundle it together with more breaking changes? :)

Agreed!

@erickzanardo erickzanardo merged commit d0c10cb into main Nov 24, 2023
8 checks passed
@erickzanardo erickzanardo deleted the erick.tile-atlas-padding branch November 24, 2023 14:39
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