feat: adding custom classes and types #14
Conversation
README.md
Outdated
| In order to do so, set a new `LeapOptions` to `LeapOptions.defaults` before loading a map: | ||
|
|
||
| ```dart | ||
| LeapOptions.defaults = const LeapOptions( |
There was a problem hiding this comment.
My only qualm with this PR is the API to set the values...
Setting LeapOptions.defaults feels awkward, especially because if you are changing the defaults, does that mean the original values are the "default defaults"?
Anyways, maybe a function instead would be better, like LeapOptions.changeTiledConstants(...) ? Or LeapOptions could be a value on LeapGame, and you just set LeapGame.options to a new instance? Leaning towards the second option since the game ref is everywhere anyways
There was a problem hiding this comment.
Also, it might be good to differentiate between Leap Tiled options and other options (like physics etc), but that's probably premature
There was a problem hiding this comment.
Yeah, I was on the fence about the naming, I tried a different approach now, let me know what you think of it, I have also prepared for the future in case we want to have multiple type of configurations, not only about tiled.
| class LeapConfiguration { | ||
| /// The tiled options, change it to configure how Leap | ||
| /// interpret the tiled map. | ||
| static TiledOptions tiled = const TiledOptions(); |
There was a problem hiding this comment.
I would slightly prefer this to be non-static, which I think means it has to be a field on LeapGame. Nothing should really be static in case there are multiple games or something
There was a problem hiding this comment.
Sounds good, I went the static route as I thought It would make it easier since we would not need to pass the object down in the classes tree. But it wasn't that hard in the end. Let me know if this is what you think
There was a problem hiding this comment.
Hm, I still think it would be better to set it on the LeapGame because the name implies it's config for the whole of Leap, which isn't just the Map. And you can always access this.game because HasGameRef is everywhere. Up to you though, looks great overall
There was a problem hiding this comment.
Oh, yes! That makes more sense to put on the game instead of the map! Sounds good, I will make the change sometime this weekend
There was a problem hiding this comment.
@kurtome I have changed so the game now holds the configuration, and passes it down to the other classes. The only thing is that I still kept LeapMap and other classes to hold an instance of TiledOptions instead of accessing it through the gameRef because these classes also need to access the options in their constructor body, and the gameRef is not set there.
I believe we could do those same operations that are being do in the constructor body, in the onLoad method, which would allow us to access the gameRef. But I thought on avoiding a bigger change than needed. So my proposal would be to land this as is, and we could revisit later, wdyt?
This PR gives leap some flexibility by allowing the developer to configure the name of classes and types of some of the objects that leap looks for on the map. This can be useful when for some reason, the developer doesn't have full control of the tiled files.
This PR also ensures that by default, all the configured values, are the ones that were before, ensuring that this will not introducing any breaking change.