tiled.dart
tiled.dart copied to clipboard
feat: TsxProviderBase with test function
Description
Currently a TsxProvider
needs a getter for the filename it can provider. This PR seeks to introduce a new base class TsxProviderBase
which instead uses a bool checkProvidable(String filename)
function to determine if a TsxProviderBase
is able to provide the source.
To accomplish this it also has Parser getSourceBase(String filename)
and Parser? getCachedSourceBase(String filename)
methods to retrieve the source when needed.
While do not like attaching Base to everything this naming scheme seems to be the easiest option to not break current implementations.
This new functionality is usefull for example to resolve all .tsx-files relative to a single directory.
Checklist
- [X] The title of my PR starts with a Conventional Commit prefix (
fix:
,feat:
,docs:
etc). - [X] I have read the Contributor Guide and followed the process outlined for submitting PRs.
- [ ] I have updated/added tests for ALL new/updated/fixed functionality.
- [X] I have updated/added relevant documentation in
docs
and added dartdoc comments with///
. - [ ] I have updated/added relevant examples in
examples
.
Breaking Change
- [X] Yes, this is a breaking change.
- [X] No, this is not a breaking change.
While the internal functionig does change and new APIs are exposed I have recreated the old behaviour in TsxProvider
as a sub class of the new TsxProviderBase
. Therefore classes extending TsxProvider
should work as before with no breaking changes.
However classes only implementing it will require additional methods, however if only implemented they should directly implement TsxProviderBase
since (apart from the implementation of the old behaviour) TsxProvider
does not offer more functionality. It is simply a Wrapper to emulate the old behaviour and not break convention.
Migration instructions
All classes only implementing TsxProvider
should either
- extend
TsxProvider
if the old API is needed (this allows you to not have to look at any new functionality) or - directly implement
TsxProviderBase
.
Related Issues
Completes #70 Allows for usecase described in #69
I've got some questions about the checklist:
- The old functionality was not covered by tests as far as I can see, nevertheless I will write some soon.
- There is no
example
folder in this repo. Where should examples go?
I've got some questions about the checklist:
- The old functionality was not covered by tests as far as I can see, nevertheless I will write some soon.
Super!
- There is no
example
folder in this repo. Where should examples go?
The checklist is copied from Flame, you can create an example directory if we don't have one (shame on us 😅).
While do not like attaching Base to everything this naming scheme seems to be the easiest option to not break current implementations.
I would consider just breaking current implementations, as long as you are willing to write a PR for flame_tiled
, which is the only major client of this library.
Overall it's hard to me to tell if this PR makes sense without concrete examples, maybe tests would help with that
@benni-tec any updates on this? :)
Not yet. My fork currently does everything I need for my current project. I do plan to submit proper PRs for this and the other issues I've opened, however I am currently pretty time constrained.
As of now my current project should be wrapped up in January, I am currently planning to clean up and submit PRs then.
Is this something that is time-sensitive for you? If so I will see if can get the PRs done this year!
LFTM (looks fine to me!)
Thanks for the review, I didn't have to time yet to write a proper comment:
I have generalised into a generic Provider that can also be used for templates, therby also resolving #73, also as being extensible to more easily discover the location of images in flame_tiled
.
I also moved the methods from TileMapParser into TiledMap since there were already parsing method there, and I always thought it was confusing to have this in a different class that does nothing otherwise. While moving it over I also made the names nicer and more descriptive.
The fromString
method seems to only have been used in the tests to generate TsxProvider
's for every included tileset, which always seemed like a hack to me! Anyways this is not neccessary anymore since the new Provider can resolve multiple or even all files (see tests for an example). The rest of the functionality can be found in parseTmx()
!
I took the liberty of adding the Lists of Provider
's to the parser to make it more convenient and clean (instead of passing it through all parse calls, only the initial Parser creation is affected and the copies)!
However I split them into multiple Lists so one for tmx and one for templates. This however does not allow to integrate the same mechanism for images (e.g. of tilesets), because I don't think the actual loading of the images is supposed to be in this library and nothing is ever done with them I don't think that is a big deal. flame_tiled
could still use the same Provider
mechanism to discorver images (this would be great improvment compared to the current state), but just create it's own List when parsing the RenderableMap!
However I could also only pass one List and the Provider
's could instead report their capability. What do you think is preferable?