-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix multiple external tilesets #37
Fix multiple external tilesets #37
Conversation
…aker.fix-multiple-external-tilesets Merging with latest main.
…to a list of tsxproviders
Merge with latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Some dartdocs of the changed methods would be good.
I know there weren't any previously, but always good to make it better. :)
Sure thing, will push an update with some comments later today. |
@benni-tec unfortunately it's been a pretty long time since I looked at this code (I'm no longer actively working on the project that leveraged Flame Engine), but I'll try to answer as best I can! In short, I didn't implement or change this method. I just added a comment in the commit to the Tiled repo (since there were no comments), and left it untouched in my commit to the corresponding Flame repo flame-engine/flame@80a483f. The filename parameter wasn't used in the FlameTsxProvider implementation, so I'm not sure why it was passed in initially. However, given that existing projects may have had custom implementations of TsxProvider, I didn't want to break a public contract with my changes and left the method as is. I introduced an additional getCachedSource method as a wrapper around getSource that takes no parameters in the FlameTsxProvider class. If you're implementing a custom TsxProvider with similar caching behavior to the FlameTsxProvider, you could just implement getCachedSource (as long as it doesn't return null, the parsing code won't fall back to calling getSource). You could also implement getSource in a similar manner that just ignores the input parameter. If you're using the FlameTsxProvider and just parsing your Tiled Map with external Tilesets, this should hopefully all be abstracted from you. Are you running into issues when loading a Tiled map with external Tilesets? Happy to help if I can! |
Ah yes legacy contracts that makes sense! As mentioned in #70 I'm resolving tsx files relative to their tmx, therfore I'm currently generating TsxProvider's for each file before parsing with this package by parsing the tmx myself. Just got curious as to the reason for the parameter while looking into this! |
Fix for the issue raised in #35. This is a breaking change as some public-facing contracts are being changed.
Updated tile_map_parser to take a list of external tileset TsxProviders. The parser now expects a list of loaded external tileset data for each external tileset. Each TsxProvider must also provide a method of getting the filename (the value stored in the 'source' property of a .tmx that references an external tileset. When parsing the tileset, the parser compares the tilesets 'source' property to the TsxProviders filename to find the correct TsxProvider. This assumes that the 'source' property is unique across tilesets, which seems reasonable. This allows for supporting multiple external tilesets in a map.
This is different from the solution discussed in #35. Since the Flame engine's method of loading tilesets required an async call, adopting the discussed approach (one TsxProvider that loads external tileset data on demand), would have required async code to be propagated throughout the parse tree. It seemed cleaner to generate the list of TsxProviders outside, then select the appropriate TsxProvider at parse time. I am open to making the update if the async propagation route is preferred though.