marshallward / TiledSharp

C# library for parsing and importing TMX and TSX files generated by Tiled, a tile map generation tool
http://marshallward.github.io/TiledSharp
Apache License 2.0
328 stars 87 forks source link

Can't be used with Web or Android target in Unity #32

Open LaylBongers opened 7 years ago

LaylBongers commented 7 years ago

Because TmxMap takes a file path, it can't be used with a web or android target. Instead, there should be an option to provide a string of data.

In Unity, there's a variety of ways to load in streamed/external asset data. On Web and Android targets however, there's no way to directly open streamed data as a file. Here you would use in unity a WWW object, which on Web downloads the file and on Android loads it in from the zipped package.

https://docs.unity3d.com/ScriptReference/Application-streamingAssetsPath.html

However, WWW will only give you a text string, which can't be fed into TiledSharp.

Currently the only workaround I can figure out is to use temporary files (which on Web will make use of local storage). This is however a needlessly error-prone and badly performing solution.

marshallward commented 7 years ago

Yes, thanks for raising this issue. This particular issue can be rolled into the more general PCL support problem (#15), but I'll leave this issue open to highlight the problem.

Unfortunately I haven't put much effort into this for a long time, but the most likely solution is to replace filepaths with some sort of File Object (also alluded to in issue #29). But I haven't done much with file objects in C#, so am not sure how to do this in a safe way.

I can try to take another crack at this over the next few days, but feel free to offer your own suggestions. As mentioned in #29, I have no problems with changing the constructor or other parts of the API.

LaylBongers commented 7 years ago

The simplest fix would be to just add another constructor that takes an XDocument directly, and have the file path constructor call into that. Anything that needs to pass it an XDocument created using something else could use that new constructor and it wouldn't break anything else.

Edit: Keep in mind the issue here is there's no way to do anything with the file system whatsoever, a File Object wouldn't solve this.

ShawnCowles commented 7 years ago

I have some local changes that support opening maps from streams, that might work. I'll try to remember to clean that up and submit a pull request.

LaylBongers commented 7 years ago

Workaround code snippet for people encountering this issue as well:

        private const string TempFilePath = "./streamingasset.tmp";

        public static IEnumerator LoadTmx(string assetPath)
        {
            var fullPath = Application.streamingAssetsPath + assetPath;
            string finalPath;

            // Check if we need to use WWW or load directly
            if (fullPath.Contains("://"))
            {
                Debug.Log("Loading in \"" + assetPath + "\" using temporary file");

                // Load the asset in from the remote path
                var www = new WWW(fullPath);
                yield return www;

                // Unfortunately, we have to copy to a temp file in this situation
                // Android and Web targets can't load directly as a file and Tmx can only accept file paths
                File.WriteAllText(TempFilePath, www.text);
                finalPath = TempFilePath;
            }
            else
            {
                yield return null;
                finalPath = fullPath;
            }

            yield return new TmxMap(finalPath);
        }
marshallward commented 7 years ago

Thanks @ShawnCowles, that would be very much appreciated. @LaylConway 's suggestion is also fine, although I'd like to completely remove any reference to filepaths eventually.

LaylBongers commented 7 years ago

As a side-note, this problem is unrelated from the need for PCL support, as Unity (and other Mono targets) aren't limited to PCL on mobile.

Cryru commented 7 years ago

@LaylConway I can't seem to be able to implement the workaround snippet you posted, could you explain it?

LaylBongers commented 7 years ago

@Cryru it's not needed anymore as #33 allows you to open a stream or xdocument directly from a string rather than using a file in the middle.

If you really want to use the workaround, you need to call it like this:

var e = LoadTmx("somefile.tmx");
yield return e;
var map = (TmxMap) e.Current;

This is because it needs to wait on WWW to finish loading. Quite a shame Unity doesn't have support for the "async" keyword because it would simplify the APIs a lot.

LaylBongers commented 7 years ago

Oh apparently that merge was rolled back

Cryru commented 7 years ago

Oh, why was it rolled back? Also if I'm using MonoGame instead of Unity it's the same deal right?

LaylBongers commented 7 years ago

I assume MonoGame will have the same issue with Web and Android targets, but the workaround is for Unity only. It makes use of Unity's way of loading external assets from Android and Web sources, which relies on the WWW class. This class isn't included in MonoGame as it's part of the Unity Engine.

Cryru commented 7 years ago

Oh right that makes sense. Hopefully they will return the string loading function. Thanks for clearing things up.

marshallward commented 7 years ago

Sorry for the rollback, some of my tests weren't working, but I don't think it's related to the patch. Hopefully I'll restore the changes very soon.

Cryru commented 7 years ago

Thanks! Would be nice to have a guide or something on the wiki on how to load from a string as well.

marshallward commented 7 years ago

Ok so I did manage to get this working. The difference is that I needed an explicit reference in my test project to System.Xml in order to call the constructor, even though I was loading by filepath. (This may be because I already had System.Xml.Linq loaded, and confusing the assemblies, not sure).

I don't think this is a problem, and I'm happy to re-instate the patch, but I wanted to make sure it wasn't something more serious.

marshallward commented 7 years ago

I've restored the changes added by @ShawnCowles so should hopefully be working now.

But before I close it, I have to say I'm surprised to see people using this and engaged enough to contribute here. Would it be worth setting up a mailing list or something around this project?

Also, how would people feel if all the filepath parsing code was removed and everything was handled with Stream or XDocument inputs? It could be a pain for beginners, but would also let me close about half of the open issues :).

Cryru commented 7 years ago

@marshallward All of my projects use Tiled in one way or another and TiledSharp is really useful for me.

Removing file path stuff and handling everything with Stream or a giant string representing the file would solve a lot of multi-platform issues for sure. I personally do not think beginners will have that much of a hard time dealing with it as reading files is one of the first things one learns, and .tmx files are as easy to read as any .txt. As long as everything is documented and explained well it will go okay.

Thanks for the great work.

LaylBongers commented 7 years ago

@marshallward It should probably be fine for most use cases as long as you document it, it will only be one or two lines extra for trivial use.