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

PCL Support #15

Open
KevinBalz opened this issue Apr 11, 2015 · 12 comments
Open

PCL Support #15

KevinBalz opened this issue Apr 11, 2015 · 12 comments

Comments

@KevinBalz
Copy link
Contributor

Would it be possible to convert TiledSharp to a Portable Class Libray ?

It would be really useful for my Monogame project, because i can't put all map related code into my Game PCL.

I've started porting, so far i've replaced the current zlib with Zlib.Portable. The main issues i encountered is the current Constructor, loading files with paths is not possible in PCL's, so you have to pass a Stream or pass a class which implements some kind of 'IMapLoader' interface, see http://timheuer.com/blog/archive/2014/05/23/porting-taglib-sharp-to-portable-class-library.aspx , which are obviously breaking changes to the current API.

So i wanted to aks if it would be ok to do this, or if there any other plans or suggestions before i continue.

@marshallward
Copy link
Owner

I must admit that I had not heard of PCLs until you mentioned them, so I'm happy to defer to you on the advantages of supporting them,

For the specific issues:

  • Replacing Zlib is not a problem. As far as I know, the DotNetZip version bundled into the source is not even supported anymore. Moving to Zlib.Portable sounds like a great idea.
  • Replacing the constructor could be an issue for some people, but I am not against the idea if it improves PCL support.

Personally I try to use embedded resources, rather than explicit paths. The pathname resolution is supposed to be the fallback option. But I do know of people who still rely on pathnames.

Do you think there is some way to support multiple constructors? Or would that break the PCL support?

As I said, I do not know much about this stuff, so maybe the best thing to do is start with a pull request, if you have it working. Then we can iterate from there.

@KevinBalz
Copy link
Contributor Author

Multiple constructors could be possible, but pathname resolution doesn't work, but resource loading could work, i would have to look into that.
Maybe it's possible to make use of conditional compiling, so if IO is usable on the current plattform it will use pathname resolution, i will look into it, as im fairly new to it.
I will start by replacling Zlib, the rest could take some time.

@marshallward
Copy link
Owner

Hi Kevin,
I just wanted to say that I haven't forgotten about this. I never heard back from Robert, and since then I've been overseas visiting my family. But I'd like to get this working when I return in a couple weeks.

I was looking into using ZipArchive to do the .zip unpacking (which is what zlib is presumably using). It is now bundled with .NET 4.5 (so I'm told) and may resolve the PCL issue that you are seeing. It would also remove an annoying dependency. (Apparently it is also slower than DotNetZip but that doesn't worry me too much.)

But I'll be able to look into all this when I return.

@marshallward marshallward reopened this May 8, 2015
@marshallward
Copy link
Owner

I'm finally putting some time into this problem of PCL migration.

I'm relying on the Xamarin scanner to check for portability. This is the report:

http://scan.xamarin.com/Home/Report?scanId=4475954e-d645-41da-9788-516500cba1a2

Am I right that the following issues would need to be resolved before it could be compiled as a PCL? (I can see how most, but not all, of them are due to DotNetZip)

@KevinBalz
Copy link
Contributor Author

Most are caused by DotNetZip ( because some functions of System.IO and System.Console), some are caused in the library, because System.Path strangely isn't supported that well(Combine ...) and the use of Assembly.GetEntryAssembly.

And yes you're right, the calls of unsupported libraries have to be replaced with supported libraries, depending on the configuration of the PCL ( configuration = plattforms the PCL will support ).

@marshallward
Copy link
Owner

OK, I think I understand this problem much better now.

Dumping the Zlib.Portable source on top of the old DotNetZip code, and switching from GZipStream to Ionic.Zlib.GZipStream seems to have sorted out just about all of the problems.

The only other major issue is the Assembly and path resolution stuff, but I reckon that there are other ways around those problems. We can talk about that afterwards.

The only other problem I see at the moment is that Zlib.Portable has some code that won't compile under .NET 3.5, which is only an issue for Unity users (which I've only just learned about this week).

I'll try to put some time into this during the weekend, though it will probably just be a zlib.portable dump into the source (assuming it's ok with the AdvancedREI authors).

I'm sorry that it took me so long to understand this issue!

@marshallward
Copy link
Owner

I've improved some (though not all) of the issues related to Zlib, but still have not resolved the dependencies on Assembly.

I think you are right, and that the current interface needs to be changed. I think the correct solution is to only allow an input stream containing the XML file, and leave the particulars of path or resource management to the executable.

I hate changing the current API, but the current one just has too many quirks and incompatibilities.

(And I see that was your original suggestion... oops)

@marshallward
Copy link
Owner

The first problem I can see is in TmxTileset, when the source attribute is set. This is basically a filename (or path) which would need to be opened. So the code still needs to have some capacity to read and open files based on filename, rather than solely working with input streams.

Now that I think about it, image resources are also referenced by their filenames in the TMX file.

But, none of this is necessarily dependent on the GetEntryAssembly call.

@KevinBalz
Copy link
Contributor Author

The image resource path in the TMX file should be no problem, because the user would anyway load the image by themselfes.

Regarding TmxTileset:
It's possible to solve this using a interface which is capable of loading files, for example:

public interface IFileLoader {
    Stream load(string path); 
}

which will then be used to get a stream of the tsx file
This obviously requires the user to implement said interface and giving it as a argument to the constructor. But maybe we could make this interface optional, so if the user uses no TSX files he can use the api as before. The drawback would be the need to throw an error if the uses omits this interface but has TMX referencing TSX files.

This use of this method was inspired from here:
http://timheuer.com/blog/archive/2014/05/23/porting-taglib-sharp-to-portable-class-library.aspx

@KevinBalz
Copy link
Contributor Author

The way PCL's work will change it seems:
http://blogs.msdn.com/b/mvpawardprogram/archive/2015/06/18/demystifying-pcls-net-core-dnx-and-uwp-redux.aspx

As far as i understood the article there will be no profiles you have to set yourself, instead the library will work on every plattform , as long as the library does not use a library/api call which is not supported on the given plattform.

@marshallward
Copy link
Owner

OK, I think I understand your idea now... so the user would create their own FileLoader class (or whatever) using the IFileLoader interface and provide it to the Load method. (Very sorry that you've had to repeat this so many times.)

I agree that would be very different, and it would shift the burden of path manipulation onto the user, in addition to encapsulating it into a proper class. But maybe that's a good thing.

I think that I am OK with removing the whole Embedded Resource scanner, which was always a bit wonky. It probably makes more logical sense to move that to my own personal FileLoader class. Same could be said for the whole parent TmxDirectory tracking.

So yeah, I think I am convinced, assuming that the transition is not too painful.

It would be nice to keep the old path management code somehow available, perhaps as example FileLoader classes or as additional projects in the solution. Defaulting to filepaths would be wonderful, although I'm guessing that would break the ability to compile as a PCL.

@KevinBalz
Copy link
Contributor Author

I guess i'm not that good in explaining^^

Regarding the File managment, the link i've provided (the taglib example) explained a way to give the user a default implementation of this file loader, although i not fully understanded how. I would wait how .net Core will probably change the way PCL's work, maybe drastically easing the process ( having more api calls availabe etc.. )

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

2 participants