Skip to content
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

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

Open
LaylBongers opened this issue Oct 8, 2016 · 18 comments
Open

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

LaylBongers opened this issue Oct 8, 2016 · 18 comments

Comments

@LaylBongers
Copy link

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
Copy link
Owner

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
Copy link
Author

LaylBongers commented Oct 8, 2016

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link

Cryru commented Oct 9, 2016

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

@LaylBongers
Copy link
Author

@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
Copy link
Author

Oh apparently that merge was rolled back

@Cryru
Copy link

Cryru commented Oct 10, 2016

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

@LaylBongers
Copy link
Author

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
Copy link

Cryru commented Oct 10, 2016

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

@marshallward
Copy link
Owner

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
Copy link

Cryru commented Oct 10, 2016

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

@marshallward
Copy link
Owner

marshallward commented Oct 10, 2016

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
Copy link
Owner

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
Copy link

Cryru commented Oct 11, 2016

@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
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants