flame-engine / tiled.dart

A Dart Tiled library. Parse your TMX files into useful representations. Compatible with Flame.
https://flame-engine.org/
MIT License
50 stars 30 forks source link

feat: TsxProviderBase with test function #72

Open benni-tec opened 10 months ago

benni-tec commented 10 months ago

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

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

Related Issues

Completes #70 Allows for usecase described in #69

benni-tec commented 10 months ago

I've got some questions about the checklist:

spydon commented 10 months ago

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 😅).

kurtome commented 10 months ago

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

spydon commented 9 months ago

@benni-tec any updates on this? :)

benni-tec commented 9 months ago

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!

jtmcdole commented 7 months ago

LFTM (looks fine to me!)

benni-tec commented 7 months ago

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?