search

AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 290 forks source link

Switch From PCLs to .NET Standard #573

Closed ilexp closed 4 years ago

ilexp commented 6 years ago

Summary

Microsoft has officially deprecated Portable Class Libraries, which is what Duality is based on with regard to portability. Projects still compile just fine and will for the foreseeable future, but it will get progressively harder and less intuitive to keep things working and expanding on it. Consider switching all PCL projects to .NET Standard for v3.0.

Analysis

Barsonax commented 6 years ago

Its good to switch to standard since PCL's are deprecated now and in the future that could mean it will be harder and harder to use other libraries.

However there are quite some things that have to be researched/changed beforehand as you already pointed out.

ilexp commented 6 years ago

Related note on Twitter about .NET Standard libraries has a nice and short way to put it:

This reflects the fact that .NET Standard is, unlike PCL, not a "build once, run anywhere" deal, but a "build once, then use it for building for any platform" deal. Unfortunately, this clashes with the current portability model of Duality, which is "build once, run anywhere [just switch launcher and backend]". We can deal with this for all the official packages, but there will be cases where people publish Duality packages that depend on .NET Standard libraries with different dependency situations across platforms, which will be lost when used in Duality because the editor is a .NET Framework runtime.

For the most part, this won't really matter - we'll still get Mono compatibility and a bit more on top of that, and we haven't really had a lot of cross-platform experience beyond that so far anyway. It may turn out that, for true, robust platform support, we will need a proper build pipeline and different sets of dependencies built into the editor anyway, and if anything .NET Standard might help with that by unifying this requirement across all platforms and pushing the issue.

ilexp commented 6 years ago

Side note about NuGet packing support for NetStandard packages: http://blog.tdwright.co.uk/2017/11/20/the-perils-of-publishing-a-net-standard-library-to-nuget/ See also: https://github.com/NuGet/docs.microsoft.com-nuget/blob/master/docs/Guides/Create-NET-Standard-Packages-VS2015.md

ghost commented 6 years ago

Hey @ilexp a few days ago I converted Duality to .Net Standard. I have not put up a pull request yet because I had run into some problems when trying to use .Net Standard 1.1. Because of those problems, I ended up using .Net Standard 2.0 instead. However, when using 2.0 almost no code changes were needed, and I have been able to run Duality with no problems so far. In fact only two code changes were needed for Duality and all of it's plugins (or at least the plugins in this repo) combined! And the changes were just removing code that had become obsolete because of .Net Standard. Needless to say I don't think using 2.0 instead of 1.1 will cause compatibility problems. But never the less, I wanted to make sure you are okay with using 2.0 instead of 1.1, also I wanted to know which branch I should put a pull request on (if any). On a side note, I have only been using Duality for a few weeks now, but I really like it, and hope to be able to help make it even better.

ilexp commented 6 years ago

Hey @Caylis1397, and welcome! Glad you're up for a PR on this 🙂

The base branch for working on this should be develop-3.0, but because of the magnitude of this change, I might create a new feature/3.0/netstandard branch for you to PR into when we're ready.

I think that we should try to target .Net Standard 1.1 first though, since the API surface we require should be as small as possible with multi platform support in mind. Can you be more specific which problems you encountered by targeting 1.1?

I'll be off for a week, but I'll get back to you after that. (cc @AdamsLair/duality-contributors, in case they want to join the discussion in the meantime)

ghost commented 6 years ago

Well as far as 1.1 goes, there is a lot of missing api. For instance, here are some of the calls to missing api in Duality.Backend.DefaultPluginLoader: System.IO.File System.IO.Directory System.IO.Path.GetFullPath System.IO.BufferedStream System.StringComparison.InvariantCultureIgnoreCase System.AppDomain System.ResolveEventArgs System.AssemblyLoadEventArgs System.Reflection.Assembly.GetEntryAssembly System.Reflection.Assembly.Location System.Security.Cryptography

All of those are not available in .Net Standard 1.1. Some of those have workarounds, but I can't figure out how to get past most of the missing api. In NVorbis.ParameterChangeEventArgs, the serializable attribute is also missing.

So if 1.1 is a hard requirement, then this will take a lot of work and planing to figure out. I do have a lot of free time, but I also have other projects, and some of them are semi time sensitive. So I can definitely give it a shot, it just might take a while. The real concern I have though, is that I might not have the knowledge / skill to find away around all these missing api calls.

If 1.1 is not a hard requirement, and you don't want to use 2.0. Then I am sure there is a workable middle ground.

I would still like to help out on this. So whatever way you think we should go about this, I will definitely try to help in any way I can.

Barsonax commented 6 years ago

I don't think we want to have to implement all those :P

Is there a version before 2.0 and after 1.1 that does cover most of the used API? Also Is .NET core 2.0 really that much harder to use cross platform? Wasnt that the whole point of core?

bricelam commented 6 years ago

Do you really care about Windows 8.1 (Store apps), Windows Phone 8.1, and Windows Phone Silverlight 8.0? There's no launcher for these anyway... I say just make the jump to .NET Standard 2.0 now. All modern .NET supports it:

See this table for more details.

bricelam commented 6 years ago

But if you do care...

.NET Standard 1.3 adds most of the filesystem and crypto APIs.

The assembly load stuff can be handled by cross-targeting (e.g. use the current code on .NET Framework/Mono and use AssemblyLoadContext on .NET Core)

Barsonax commented 6 years ago

Looking at that table I say we go for 2.0. Windows phone is completely dead (I can know it I used to own one) and windows 8.1 is close to being dead. Not worth it imho.

ghost commented 6 years ago

I do agree that .Net Standard 2.0 is the way to go. After all, when converting Duality to 2.0 the only problems I found were caused by Visual Studio's new project format. And I still have not run into any bugs when using Duality running on 2.0. Or at least none that were caused switching to 2.0. But, 2.0 does have one down / up side; the number of available APIs in 2.0 is 32000 compared to 1.6's available APIs of 13000. The upside to this is that there are a lot more available tools to use, but the downside is that having so much functionality also means having less portability. I don't think this will be a problem, but it should be considered when making a decision.

deanljohnson commented 6 years ago

I would consider the following: if we go with .Net Standard 1.1, would we end up switching to 2.0 in the future anyways? If so, it seems we might as well 'bite the bullet' and go with 2.0 now.

ilexp commented 6 years ago

It seems like the discussion is currently leaning towards adopting .NET Standard 2.0, but I do think that there is value in restraint when it comes to API usage. The less we use, the easier platform adoption is - and this includes potential future platforms as well, which may work with a reduced API subset.

My point is not that we should bend over backwards to get compatibility with 1.1, but that I think we're so close to 1.1 that it makes sense to try to stick to it, rather than abandoning it on the first sight of trouble. If we can't make it work, 2.0 makes sense, but I think we should put a bit more energy into trying 1.1 first.

.NET Standard 1.3 adds most of the filesystem and crypto APIs.

That's an option too, though I don't think we need them for the most part.

Let's investigate where the problem originates from and see if we can fix remaining compiler errors on a case-by-case basis. For example, all file system access has been removed from the core lib in v2.0, and now lives in the system backend plugin, which is unaffected by the PCL-to-.NET Standard switch.

Well as far as 1.1 goes, there is a lot of missing api. For instance, here are some of the calls to missing api in Duality.Backend.DefaultPluginLoader:

DefaultPluginLoader is not part of any core library. It is a shared source file that is included by the launcher (.NET Framework 4.x), the editor (.NET Framework 4.x as well) and testing environments - if there are compiler errors for this class, then that probably means something got mixed up.

With .NET Standard 1.1, is the new .csproj project system already enabled? As far as I recall, it includes all *.cs files by default, and it is possible that the shared source file is located within the Duality folder, which would accidentally include it in the core library build.

If that's the case, can you fix this by moving the file elsewhere and adjusting the links from all other projects? How does the .NET Standard 1.1 compile hold up error-wise after the fix?

ghost commented 6 years ago

Hey @ilexp, Thanks for pointing out that the Default Plugin Loader was not part of Duality's main core, I had missed that completely. And when I moved it to somewhere else, Visual Studio stopped complaining about most of the errors, even ones that were unrelated. All that was left were 4 errors in Duality.Drawing.PixelData do to Parallel.ForEach not being part of .Net Standard 1.1 by default. However, that was easily fixed by adding a direct reference to System.Threading.Tasks.Parallel in Duality's project. It now compiles and runs without an errors!

TL;DR: I got Duality running on .Net Standard 1.1.

@ilexp all that needs to be done now is to change to a newer version of NuGet, correct? I have never published anything on NuGet so I don't know what changes will be required, but your the one who publishes Duality on NuGet anyways right? If so, then I'm sure you can do that part.

ilexp commented 6 years ago

TL;DR: I got Duality running on .Net Standard 1.1.

That's great news 🙂

@ilexp all that needs to be done now is to change to a newer version of NuGet, correct? I have never published anything on NuGet so I don't know what changes will be required, but your the one who publishes Duality on NuGet anyways right? If so, then I'm sure you can do that part.

I'm no longer sure if this is really a hard requirement (discussion in #574), and also not yet sure if this is all that needs to be done. Either way, I'd say we should proceed with this step by step and see how that works out. To make sure we'll have a safe way back, I have created a new feature/3.0/netstandard branch for you to file a PR. We can do in-detail discussion in the PR thread and proceed with a more high-level view here.

Barsonax commented 6 years ago

Nice job on the progress @ilexp @Caylis1397. Looking forward to switching to core.

With .NET Standard 1.1, is the new .csproj project system already enabled? As far as I recall, it includes all *.cs files by default, and it is possible that the shared source file is located within the Duality folder, which would accidentally include it in the core library build.

As far as I know all core projects use the new .csproj format. It includes *.cs files by default (which for the most part is good since it dramatically reduces the size of the csproj file). No need to move the file though as you still can exclude them.

ilexp commented 6 years ago

Moving this to the Future milestone, as it increasingly seems like this is going to be a bigger feature to complete than anticipated. As I'm currently moving toward slowly wrapping up v3.0, I think this will have to remain in a branch for now, to be considered for binary release at some later point. This will also give us more time to gather experience with this in practice.

tl;dr: Let's carry on pursuing this, but don't expect it in v3.0.

ilexp commented 6 years ago

Progress

ToDo:

Barsonax commented 6 years ago

In the process of porting the pathfindax plugin to netstandard/netcore I noticed that nunit doesnt seem to like this. They seem to have some major bugs regarding the new roslyn based test discovery that came in VS15.6. In my case half my tests where not discovered and I had some 'ghost' tests that didnt actually exists so basically it rendered nunit unusable.

I currently have no idea if this is getting fixed any time soon. Since duality is using nunit this might become a issue for duality as well.

I fixed the problem by replacing nunit with xunit which does not have these bugs.

DaceKonn commented 5 years ago

Necromancing the thread

I currently have no idea if this is getting fixed any time soon. Since duality is using nunit this might become a issue for duality as well.

It won't be fixed - XUnit is NUnit - the authors decided to start with new solution because of NUnit incompatibility - they also wanted to move away from MSTest

ilexp commented 5 years ago

Some MS docs on multi-targeting platforms with .NET Standard: https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting

One point to note is that they explicitly recommend not targeting any 1.x standard version unless there is a specific older platform that would otherwise be unsupported. Might be worth it to reconsider the 1.x target after verifying exactly which platforms and potential future platforms would be affected.

Barsonax commented 5 years ago

Already had a feeling .net standard/core 2.x is the way to go for projects going forward. All the nice stuff is there (SIMD, span, tiered compilation, less dependency hell etc).

Though you cannot reference a .net standard 2.x library in a .net 4.5 library I believe. Need .net 4.6.2 for that I think.

bricelam commented 5 years ago

Though you cannot reference a .net standard 2.x library in a .net 4.5 library I believe. Need .net 4.6.2 for that I think.

.NET Framework 4.6.1+ required; 4.7.2+ recommended.

ilexp commented 5 years ago

.NET Framework 4.6.1+ required; 4.7.2+ recommended.

Related background info: https://stuartlang.uk/dotnet-framework-support-for-dotnetnet-standard-2-0/

One thing that I don't understand yet, and where I'm unsure about whether or not this might be an issue is that even 4.7.1 projects referencing .NET Standard 2.0 include binding redirects to "custom" (?) versions of various .NET framework assemblies. Based on the recommendation, I would have assumed that they are only necessary in the 4.6.1 case, but not with 4.7.1.

I'm not yet sure how to handle this in Dualitys modular / runtime package-based environment, or whether anything would even need to be handled in the first place. Either way, those binding redirects and special assemblies don't appear with .NET Standard 1.1.

Edit: Just noticed, I tested this in a (separate) project using 4.7.1, not 4.7.2 - assuming the binding redirects would be gone with 4.7.2.

Barsonax commented 5 years ago

When #710 is mergedif then duality should be able to install multi framework packages that also target netstandard. No need to update nuget to v4.x it seems saving us a ton of work.

If we also update Nuget.Core to 2.14 then it even should install the netstandard version of the package if we change the targetframework in the package manager to netstandard (which in the PR is hardcoded to net452 at this time of speaking).

Barsonax commented 5 years ago

VS2019 no longer supports PCL's out of the box: https://github.com/AdamsLair/duality/pull/726

It is possible however to install a targeting pack for it.

Barsonax commented 5 years ago

I created a new branch here which is based upon the netstandard branch we had. Some changes:

Tests are green and iam able to start duality and do some simple stuff. Haven't gone much farther than running a scene with a rigidbody that is falling.

EDIT: some more changes:

Also noticed that starting the game from visual studio breaks debugging. Attaching after the fact works fine for some reason. Its not loading anything according to visual studio: image I believe we found this same issue earlier but seems its not yet fixed in VS2019 :(. EDIT2: here is the issue I made a year ago: https://github.com/dotnet/project-system/issues/3690

ilexp commented 5 years ago

So let's pick this up again 🙂

I created a new branch here which is based upon the netstandard branch we had. Some changes:

Looking at the branch comparison, it seems like something's wrong there. All those "merge master into release" commits shouldn't be part of the branch. Can you create a new branch off the current master and cherry-pick the commits we need?

Some changes:

Nice work 👍

Also noticed that starting the game from visual studio breaks debugging.

This could be a critical issue, so we should investigate a fix or workaround for this first to get a clear picture of our options in proceeding.

Progress

ToDo:

Did I miss any ToDos?

Barsonax commented 5 years ago

Fix the feature branch including a lot of commits that shouldn't be there, probably by creating a new one and cherry picking a bit. @Barsonax

Ugh it doesnt happen often but I made a error with git :(. Now to fix it...

EDIT: Fixed it but I had to do a force push so if you have the branch locally you have to discard your local one.

Re-introduce "aggregate binaries" build steps in each project that were accidentally removed from the csproj files during netstandard conversion.

For this I found a solution since the buildoutput is no longer copied to the build output folder with package reference:

  1. Add GeneratePathProperty="true" on the packagereference: https://github.com/Barsonax/duality/blob/feature/netstandard_barsonax/Source/Core/Duality/Duality.csproj#L11
  2. You can now use $(PkgPackageNameWithUnderscoresInsteadOfDots) to refer to the package path: https://github.com/Barsonax/duality/blob/feature/netstandard_barsonax/Source/Core/Duality/Duality.csproj#L97

Did I miss any ToDos?

Yes for the projects that stay on the netframework we need to target 4.7.2. I already did this in my netstandard branch but idk if that needs to be done elsewhere as well.

Also the project template for the plugin project that gets generated for the enduser needs to be updated to netstandard2.0 instead of 1.1.

ilexp commented 5 years ago

Extended the status message / ToDo list:

Progress

ToDo:

Yes for the projects that stay on the netframework we need to target 4.7.2. I already did this in my netstandard branch but idk if that needs to be done elsewhere as well.

Not sure whether it really makes sense to update all those dependencies. No harm done to move to the same level as the Duality editor and Desktop backends, but is there any gain from it doing it in a bulk edit? Sounds to me like we might as well leave them be and update when they require any newer framework on their own. They can be used just fine from 4.7.2, whichever framework they're targeting themselves, correct? Let me know if I'm missing something here.

Affected projects would be:

Barsonax commented 5 years ago

Fix the broken Canvas rendering unit test that was mentioned in the chat after recent updates to the netstandard feature branch.

This was broken while attempting to migrate to the new csproj format to clean up the nuget packages and make that more consistent. I decided to do it since all the plugin projects are already migrated now. Its not strictly neccessary. Since it turned out to be more complex than anticipated it might be better to do this later (and create a separate issue about it).

Not sure whether it really makes sense to update all those dependencies. No harm done to move to the same level as the Duality editor and Desktop backends, but is there any gain from it doing it in a bulk edit? Sounds to me like we might as well leave them be and update when they require any newer framework on their own. They can be used just fine from 4.7.2, whichever framework they're targeting themselves, correct? Let me know if I'm missing something here.

Yes, you will get warnings if you try to reference netstandard 2.0 though but it should work.

Barsonax commented 5 years ago

I reverted the csproj format change, tests are working again.

EDIT: Instead of adopting the new clean csproj format for the tests I only migrated to packagereference. This achieves the goal of not mixing the old and new nuget restore behavior without breaking any tests. All projects are now using packagereference.

Barsonax commented 5 years ago

Re-introduce "aggregate binaries" build steps in each project that were accidentally removed from the csproj files during netstandard conversion. Use the fix that Barsonax mentioned here.

I have been thinking about this, why not simply set the output path like so: https://github.com/Barsonax/Singularity/blob/master/src/Singularity/Singularity.csproj#L24

For the netstandard projects you also need to disable appending the targetframework like so:

<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>

That should make sure everything ends up in Build\Output or whatever output folder we require.

Test what happens when confronting the Duality package manager with a package that targets only .NET Standard, and nothing else. If that doesn't work, fix it.

Currently making a build of singularity that only targets netstandard (I only had the netframework for compatibility with duality anyway) so we should be able to test it end to end soon :).

EDIT: alternatively you could specify the output folder when calling msbuild and keep the csproj file clean?

Barsonax commented 5 years ago

Update on the current progress

Progress

ToDo:

Bugs

Test what happens when confronting the Duality package manager with a package that targets only .NET Standard, and nothing else. If that doesn't work, fix it.

EditorError:   FileNotFoundException: Could not load file or assembly 'Singularity, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. The system cannot find the file specified.
CallStack:
   at Singularity.Duality.Scopes.GameScope..ctor(ILogger logger, ISceneScopeFactory sceneScopeFactory, ISceneEventsProvider sceneEventsProvider, IEnumerable`1 moduleResources)
   at Singularity.Duality.SingularityPlugin.OnGameStarting()
   at Duality.CorePluginManager.InvokeGameStarting() in C:\Git\duality\Source\Core\Duality\CorePluginManager.cs:line 136
   at Duality.DualityApp.SwitchExecutionContext(ExecutionContext targetContext) in C:\Git\duality\Source\Core\Duality\DualityApp.cs:line 511
   at Duality.Editor.Sandbox.Play() in C:\Git\duality\Source\Editor\DualityEditor\Sandbox.cs:line 104
   at Duality.Editor.Forms.MainForm.actionRunSandbox_Click(Object sender, EventArgs e) in C:\Git\duality\Source\Editor\DualityEditor\Forms\MainForm.cs:line 577
   at System.Windows.Forms.ToolStripItem.RaiseEvent(Object key, EventArgs e)
   at System.Windows.Forms.ToolStripButton.OnClick(EventArgs e)
   at System.Windows.Forms.ToolStripItem.HandleClick(EventArgs e)
   at System.Windows.Forms.ToolStripItem.HandleMouseUp(MouseEventArgs e)
   at System.Windows.Forms.ToolStrip.OnMouseUp(MouseEventArgs mea)
   at System.Windows.Forms.Control.WmMouseUp(Message& m, MouseButtons button, Int32 clicks)
   at System.Windows.Forms.Control.WndProc(Message& m)
   at System.Windows.Forms.ToolStrip.WndProc(Message& m)
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)

image

Might be related to the fact that with packagereference only root dependencies are installed?

Barsonax commented 5 years ago

Not sure how to fix the bug I just found. Seems to be a bug in nuget but since its 2.14 I doubt it will ever get fixed.

ilexp commented 5 years ago

Didn't find time yet for a full response, but regarding the singularity NuGet thing, the runtime exception does look related to the package installation problem, could be missing or misplaced dependencies / libs. Did you try what happens when you manually package it with an explicit .nuspec where you specify files and dependencies as needed?

Barsonax commented 5 years ago

The package is correct its nuget that is not correct here.

If you change the packagemanager target to netstandard it does include singularity (and alot of other dlls).

ilexp commented 5 years ago

Assumed as much, but wondered about potential workarounds. That said, if you're using the new default way to create a package for .NET Standard, and that fails, we're in trouble regardless. While it would be possible to require Duality packages to be compatible with the old NuGet, that would cut us off from the bigger part of the global package repository at some point as potential dependencies. Not great at all.

So, we might actually be on the safer side if we backlogged this one again and first tackled #574 in order to proceed. But since we're softblocked by #668 right now anyway, might as well do that for a proper solution.

ilexp commented 5 years ago

We're no longer softblocked by #668, as we found a workaround. Proceed with #668 after #574 is solved, then continue with this issue.

Barsonax commented 5 years ago

Fix the broken Canvas rendering unit test that was mentioned in the chat after recent updates to the netstandard feature branch.

This was broken while attempting to migrate to the new csproj format to clean up the nuget packages and make that more consistent. I decided to do it since all the plugin projects are already migrated now. Its not strictly neccessary. Since it turned out to be more complex than anticipated it might be better to do this later (and create a separate issue about it).

Made a issue for this: https://github.com/AdamsLair/duality/issues/737 We should tackle that issue after we complete this one.