AdamsLair / duality

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

Added a launcher class and use this to launch duality in the template #840

Closed Barsonax closed 4 years ago

Barsonax commented 4 years ago

Untested and mostly intended for discussion at of this moment.

Changes:

Future:

ilexp commented 4 years ago

No time for a proper review yet, but one thing I noticed were NativeMethods used by the launcher being moved into the Duality core. However, the core is supposed to be strictly platform-agnostic, and those launcher methods are Windows only and probably don't even make sense on some of the other platforms - so they really shouldn't be moved into the core.

What they're dealing with is manually opening a console window in a non-console application, which is purely for development / debug purposes. I think it would be completely fine if the launcher API did not have this functionality at all and we'd leave it as a local add-on in the "official" launcher application. Custom launchers would then omit this feature unless users copy-pasted it, and thereby accepted the implications.

Barsonax commented 4 years ago

Moved the nativemethods part back

ilexp commented 4 years ago

Can we separate the version bump from this PR? Adds a lot of noise to the review and it's also mixing two things that are probably better off independent. I mean, we might add any number of tweaks and fixes before releasing the next alpha, and a version bump commit should be the final change for its version number.

Barsonax commented 4 years ago

Can we separate the version bump from this PR? Adds a lot of noise to the review and it's also mixing two things that are probably better off independent. I mean, we might add any number of tweaks and fixes before releasing the next alpha, and a version bump commit should be the final change for its version number.

Yeah I was thinking the same, just needed the version bump to test it (else it takes the one on nuget.org...). Better move it to a separate commit and out of this PR.

Barsonax commented 4 years ago

Expected to see the old Launcher project converted to a library and have a one-line command in the Template's Main, but this way works as well.

Clicked wrong.. should be "Requested Changes"

Yeah but the launcher had some native code in it which cannot be in core and we also still need a executable for development. The launcher project is no longer published as a nuget package though as thats the template's responsibility now.

Besides I think in the future this API will be a bit more elaborate and also allow to change some config so it could also be used in a unit test context. For that it needs to be in core as well.

ilexp commented 4 years ago

So what this PR does, if I'm reading this correctly:

Sounds pretty good to me 👍

Can we extend this one step further and make the dotnet new package auto-rename the launcher project to match the game? That way, a user-created TestGame project will have a TestGame.exe launcher right from the start.

We could even bundle a copy of the default embedded EditorUserData.xml in the solution template, with an added relative LauncherPath entry that is replaced by the dotnet new template logic, so the editor is correctly configured to use the custom launcher exe right away.

Barsonax commented 4 years ago

The template already changes the exe name.

You're right that we do need a editorconfig with a modified path now though. However might be better to do that in a future PR as to not make this one too complicated (need to add a EditorAppData.xml etc)

ilexp commented 4 years ago

You're right that we do need a editorconfig with a modified path now though. However might be better to do that in a future PR as to not make this one too complicated (need to add a EditorAppData.xml etc)

Created a issue #848 for this 👍