TiledMap PR/Issue Cleanup Initiative

[BoBIsHere86]

Quick Overview: There are Currently 9 open Tiled Map PRs. 7 of them in a state of (what looks to be) completeness. And 11 open TiledMap issues in the Tracker.

With the help and advice of everyone involved in the related PRs. I believed we could easily knock out 8 of the 9 PRs without issue and close out 6 issues (possibly more if some issues are outdated). Plus, with the recent release of libGDX 1.13.0, I'd assume we would have PLENTY of time to get this done before the next release.

I know the TiledMap codebase has always been a point of contention and some people believe it should be refactored and split off into it's own library. (That's probably a VERY good idea) But For now, we should aim to incorporate the bug fixes and feature requests already in progress, to get us to a fairly feature complete state before any major restructuring.

Why am I clogging up the tracker with a new Issue? I'm no authority on the TiledMap codebase. But I've had a bit of free time this week and looked through all of the PRs to see what state they were in and to my surprise most of them were pretty complete. But since the tiledmap codebase is pretty tightly coupled, changes in multiple PRs have a cascading effects on other PRs causing a whole chain of back and fourth with different contributors. Based on which get's merged first. And that can be a headache.

So I thought I'd provide 1 possible path to victory. The idea here is that the contributors can make the necessary changes to support the PR that was merged before it. I will make notes for Each Contributors PR if needed.

Suggested PR Merge Order Steps: (Each step assumes the previous one has been merged to Master)

First Merge: Merged AS of October 30th, 2024 👍 ~~"FIX: Parsing imagelayer inside a group tag in TMX file" - PR: #7135 Why? Out of all the PRs this one is the most self contained and would have a smaller impact on any of the follow up PRs. As well as fixing a long standing issue that's been around forever.~~

Second Merge: Merged as of November 21st, 2024 👍 ~~"Add a tiledmap loader for json files" PR(#7505) (old: PR: #7386) - CLOSES Issue #7291 and #5707 (Duplicate issues) A New PR(#7505) has been made to replace the old one one, since the original Author could no longer continue work on it. This PR right here is the biggest decision I feel. As this PR has the largest impact on all the others. I believe it would be best to get it done first. Which allows the other contributors to properly add and test changes to their own PRs based around the new BaseTmjMapLoader.java~~

Third Merge: (These next 3 PRs could be done in any order from each other):

~~"Added support for 'class' property in tiled tmx maps." - PR: #7007 (This PR was closed without Merging by original author) Note for @lyze237 The changes made here would now need to be replicated in the BaseTmjMapLoader.java because of PR: #7386~~

"implement loading Tiled Point objects" - (new PR:#7523) PR: #6384 Note for @happy-bracket (This is one of the older PRs, I don't know if you will see this) The changes made here would need to be replicated in the BaseTmjMapLoader.java from PR: #7386

"Support for Layer TintColor in TiledMap" - PR: #7076 CLOSES Issue #6503 Note to myself @BoBIsHere86, changes would be needed in BaseTmjMapLoader.java to match BaseTmxMapLoader.java to add functionality because of PR #7386

Fourth Merge: "Removed duplicate code in TmxMapLoader" - PR: #7136 (Good PR but since it's not a bug fix or feature it's lower on the list.) Note for @DaveH355 this PR would need to have another quick look over after PR #7386 is merged since it attempts to modify getDependencyFileHandles() which is changed significantly in #7386, there were 2 methods added called "getTileSetDependencyFileHandle()" which is where the refactored code from this PR would need to be moved to

"Support for Text Objects in BaseTmxMapLoader + bugfix." - PR: #7498 Note to myself @BoBIsHere86, modifications needed After TintColor is merged The changes made here would need to be replicated in the BaseTmjMapLoader.java from PR: #7386

Fifth Merge: (Oldest Open TiledMap PR!) "added tiled template object support" - Replacement PR #7533 PR: #5938 CLOSES Issue #5102 Ok, So sadly the Current proposed solution is over 4 years old and out of date. I left a comment in the PR mentioning this but I don't know if the Original contributor will ever see it. But I have implemented a new solution and pushed a new PR for this feature)

"Add support for 'class' type property in TMX files" - PR:#7534 PR: #7438 Note: Quillraven has submitted an enhanced PR to replace the original, (currently a draft). So that PR will now be considered the main one to add class support.

Don't Merge : "Support infinite TMX maps Closes #5764" - PR: #6025 CLOSES Issue #5764 Sadly there is currently no accepted Solution for this issue the PR is kind of dead for now. Unless one of you superheros want's to save the day.

~~So that's 1 possible path we could take. An alternate path would be to save the #7386 PR for last. But then I feel that puts the burden all on @Kevin-Garnier to implement everyone else's changes by himself. So I didn't think that was quite fair, all things considered.~~

Once again, I am not claiming to be an authority of this tiledmap stuff. I just thought I would try to rally the libGDX troops together to defeat these evil PRs.

All suggestions welcome!

[silas-hw]

This seems well thought out and I'm happy to help however! Lmk when the tmj loader is merged and I can work on adding my fix to that too

[patton73]

Looking forward for some admins to comment on this issue. Hope all those PR's can be merged fast. This code is really needed.

[BoBIsHere86]

All Glory to the libGDX admins! PR: #7135 has been merged folks! We have movement! On to the JSON loader PR, lots of changes needed. Hopefully @Kevin-Garnier checks his github notifications before 2025 and has some free time. :-D

[BoBIsHere86]

Alrighty, it's showtime! 🥳 The biggest roadblock (JSON support) has finally been merged. Now we can focus on the first set of smaller PRs: #7438, #7007 and #6384 All of these PRs "seemed" to be left in an pretty accepted state. Now we just need to update the PRs to add the functionality into the BaseTmjMapLoader.

All 3 PRs should be a fairly simple to update once u rebase, just by dropping it into the equivalent section of the new BaseTmjMapLoader. Please note that there was a small refactor. But it shouldn't touch any of the methods where your code sat. All of the non, XML/JSON specific methods have been moved into a parent class, significantly reducing duplicate code and giving us a place to put any future tiledmap specific utility methods for loading maps.

If anyone doesn't have the time. I will gladly help things along by sending a PR to your repo if you would like. We are not in a rush here, so don't feel pressured. Once all 3 are good. I will make a note here to alert the proper authorities to take a look and see what they say.

Pinging just to alert. @silas-hw, @lyze237, @happy-bracket

Thanks again to the libGDX admins. 🚀

[tommyettinger]

Or you can fork those PRs like you did the JSON one, make any changes you see fit, and issue them as your PRs with additional contributor(s): (original author here). As long as each commit doesn't somehow have its author changed, they still get credit as contributing a commit (after being squashed) to libgdx.

[BoBIsHere86]

Sounds like a plan. 👍 silas and lyze seem pretty active, so I'll give them some time. But I definitely think I may have to take over PR 6384 since the original contributor hasn't been active for about year.

[lyze237]

I think there is no need to implement https://github.com/libgdx/libgdx/pull/7007 anymore as the change has been reverted over more than a year ago.

[BoBIsHere86]

I think there is no need to implement #7007 anymore as the change has been reverted over more than a year ago.

Yea, i wasn't really sure what we should do. But was waiting to hear from you. If this was merged a year ago. It made total sense. But it's been so long, 2 tiled versions have been released since then. Should we be looking to support maps made during a specific year long period when 1.9 was released and broke everything? Now its just a footnote in tiled history. Either way its your call. You can close out the PR if you would like. Thanks for the help either way Lyze. 👍

[silas-hw]

Uni work has really picked up for me so I may be unable to contribute much until around Christmas time

[BoBIsHere86]

Uni work has really picked up for me so I may be unable to contribute much until around Christmas time

No problem. As tommyettinger said, If you would like. I could pull down your PR make the necessary changes and resubmit a new PR. Which should keep you as the OG author in the commit history. Could have this done pretty quickly later today. I'm currently doing it for 6384 as well anyway.

[BoBIsHere86]

Ok so a quick update to any of the high level admins looking at this. Lyze chose to close PR 7007, so it's no longer is relevant. After taking some time to look over PR 7438. I decided to move it to the back of the list as I don't think it's in a solid ready to go state yet. And finally PR 6384 is now being replaced by PR #7523

So where does that leave us? We can now focus on PR #7523 and #7076, both have been rebased, updated and retested to support the TMJ Loader. So now it's just a matter of waiting for any new comments or reviews to the PRs and I will respond if needed.