Feature Added: A Universal Tiled Map Loader #7640
Conversation
This will seamlessly handle any map type passed into it. Passing it along to the appropriate loader based on file type and properties. The way properties will be handled not decided yet. There are 2 potential methods. But it's ugly. Might refactor and consolidate atlas param instead. Added
Cleaned up a mismatch in the TmjMapLoader, so it uses its own parameters the same way as all the other maploaders do.
…ll using the TiledMapLoader.
|
Hooray! Finally a good example for why the current map package's naming scheme is so bad! Right now, the name component "Tiled" is used for any maps that use a concept of tiles, and is actually not related at all to the maps being created by the "Tiled" 8000 editor. Previously, "Tmx" was used to differentiate Tiled's file format, which got fuzzy when "Tmj" was also introduced. And now, for something that loads either format produced by the "Tiled" editor, "Tiled" is not a sufficient name for the class because it's already (I would argue badly misused) to mean a map format with tiles. This has bitten contributors before, though I'd struggle to find a good example in the PR archives. It's almost certainly misled users, especially since any non-Tiled editors are extremely rarely used. This is part of why there was discussion about spinning off the map package, so unmaintained editor support could be dropped (Tide is one of them). I don't know if there's a good solution here, since "Tiled" actually is the only fitting name for both Tmx and Tmj formats... |
| Class<T> targetParameterClass) { | ||
|
|
||
| // If the parameters passed in are already the appropriate subclass. We return them as is. | ||
| if (targetParameterClass.isInstance(incomingParameters)) return (T)incomingParameters; |
There was a problem hiding this comment.
This fails on GWT; isInstance() isn't implemented for Class.
Error: [ERROR] Line 218: The method isInstance(BaseTiledMapLoader.Parameters) is undefined for the type Class
There was a problem hiding this comment.
Dang it! I don't know how GWT slipped my mind. I will look for a clean solution or maybe I'll just refactor the parameters after all so we don't need to jump through these hoops. Thanks for pointing that out.
|
I agree that the namings are confusing but I guess they also make sense since the editor is called I personally don't mind refactoring the parameters and I'd argue that it is not that big of a deal. I assume the map loading functionality is not spread across multiple areas in a project and therefore fixing the parameter argument (if it is even used) is not a big deal. Therefore, I vote for a refactoring and streamling it. It should also solve the GWT issue mentioned above? |
I really would use specific methods like loadXmlTiledMap() or loadJsonTiledMap() or similar. So methods will talk theirselves with the names and the programmer will avoid using those ugly parameters. Parameters will be encapsulated into the loader's code. |
|
Yes, as tommyettinger pointed out. The maps and their naming conventions are all a mess and leads to this ugly situation. But at this point, I'm not sure what we can do about that right this moment until we break it all out. @Quillraven, Yes fixing the messy parameters issue would do away with needed to jump through hoops for the loader methods. Or maybe we do a half-step, without any breaking changes. The only worry hear is it might send mixed messages to the user. Because they see TmxMapLoader.Parameters and think "Oh, maybe in the future there will be map specific parameters". or I can extend this and add my own. I guess we could could always just put deprecation warnings on them. Or maybe I am overthinking this? I don't know. If enough people just says go for it I'll make the change real quick and our problems will be solved. Well, except for the naming issue, |
Thanks for the input, assetManager.setLoader(TiledMap.class, new AtlasTmxMapLoader(new InternalFileHandleResolver())); And if I remember correctly, something like this doesn't actually work anyways because you're changing the loader for TiledMap.class each time, so the loader which was set last is what get's used to attempt to load the map. Causing a crash when it loads a map of the wrong type. So the example I gave was of how to use it with the AssetManager now, compared to the old way. It give's the user a simplified option, as well as an option to handle multiple map types. Where that option didn't actually exist previously. |
|
The next version is 1.14 because there are a lot of BREAKING CHANGES already in the CHANGES file. I think our breaking change with the parameter would be a good time for 1.14 as well. Again, I don't think it is a big deal and we shouldn't carry on the "mistakes" from the past just to avoid that people have to fix a few code lines in their project. If it is a super huge problem for someone he can always stick to the previous LibGDX version. |
Completely agree with that. |
…from the loaders. All Parameters should live in BaseTiledMapLoader.Parameters (which they already were anyway). Changed any test which was using the AtlasTmxMapLoader.AtlasTiledMapLoaderParameters to use BaseTiledMapLoader.Parameters instead.
|
Alrighty, so I went ahead and made it so only the BaseTiledMapLoader contains the Parameters class. In the end the only breaking change was related to the Atlas Map Loaders, because for some reason unlike the other loaders they were named differently. E.g. AtlasTmxMapLoader.AtlasTiledMapLoaderParameters. So I went in the test's and changed those where it was needed. |
This argument is actually not a good one. Why do you think nobody is using something just because there is no documnetation? (E.g. I was actually using the atlas loader, even with missing documentation, but I'm really happy if there are breaking changes for the better) |
Well, that wasn't much of a serious argument. Just more of a general thought of "Well, at least there are probably less people using the atlas loaders." Whether it be lack of documentation or even knowledge that it exists. But, also the Json version of the atlas loader hasn't been released outside of the snapshot yet, so luckily it's not really out in the wild. So if there were breaking changes. It would only affect the one map loader. That being said, in regards to documentation. As of a week ago, we're trying to fix that. 😄 Thanks for the input! |
|
@BoBIsHere86 Documentation looks good. And I really like the gifs, pictures explain things better than a thousand words could do (as we say in Germany). |
After discussing this with @Quillraven a few month back when we were working on the new JSON format support.
It was suggested that maybe it would be cool to have a feature that could seamlessly load any tile map without the need to specify what type it was, as well as being able to load in different types of maps with the AssetManager.
As it stands we now have 4 separate tiled map loaders. TmxMapLoader, AtlasTmxMapLoader, TmjMapLoader and AtlasTmjMapLoader.
What I've gone ahead and done was add the new, boringly named "TiledMapLoader". The idea behind this was to add a nice convenient wrapper class that "Just Works".
It can be dropped in and replace ANY map loader without any additional changes.
One of the main use cases for this was the ability for us to easily support loading multiple map types through the asset manager.
For example, if a user plans to use a .tmx maps, well as atlas .tmx maps. Or just every map type for some reason.
It would simply work like this.
The only big hassle I ran into with this was dealing with the map parameters. Because as it stands now, we have 4 maploaders extending and creating their own parameters from BaseTiledMapLoader.Parameters. And require passing in only their specific type of map parameter into the loader.
What's interesting is that none of the map loaders actually have any unique parameters. And the ones that do (The atlas loaders "forceTextureFilters") are never actually used anywhere. So it's a bit confusing.
But any changes to that overall structure would be a major breaking change and I'm not sure if we would want that.
So instead I just worked around it, so everything pertaining to parameters will just continue working as is.
Although unlike the other maps if you are using the new TiledMapLoader you can now pass in BaseTiledMapLoader.Parameters and instead of throwing an error. It will convert it to the appropriate Map's Parameters for you. Though if we don't want that. I can remove that and have it throw an error as well.
So that's pretty much it for my Tiled PRs. This was the last thing on my list to complete and push. The goal being this would make a nice Final PR after all the new features and fixes had been added.
As per usual. Any comments or suggestions are welcomed.