AdamsLair / duality

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

Changed some netframework projects to use .net sdk style csprojs #744

Closed Barsonax closed 4 years ago

Barsonax commented 5 years ago

Migrated our remaining netframework projects to sdk style projects.

Tests are green and the generated nuget packages seem to be working in the template. I think its also good to test a bit more with some samples.

Hope this is the last PR with thousands of changes for a while :)

Changes:

Completes #737

Barsonax commented 4 years ago

Did a quick test and seems that the winforms designer is now working in visual studio.

ilexp commented 4 years ago

Looks good, judging from just looking at the changes. Don't have the time for a full checkout and test right now, but maybe @SirePi can jump in, so we can ensure it's been tested on at least two machines?

Stuff to look out for (maybe verify on your machine as well @Barsonax, in case any points are missing):

Barsonax commented 4 years ago

Looks good, judging from just looking at the changes. Don't have the time for a full checkout and test right now, but maybe @SirePi can jump in, so we can ensure it's been tested on at least two machines?

@SirePi since this PR changes some csprojs visual studio can get confused. Clean everything before testing (running git clean -fdx after checking out and before opening visual studio should do the trick).

Does everything compile?

Yes

Is the Build/Output folder populated as expected?

Yes, output is the same.

When selecting a sample project as a startup project, does it run the editor as expected, with all the sample stuff working?

No, have to check this but iam probably simply missing some entries for this in the csproj/launcherconfig.

When building nuget packages, do they end up fine, e.g. did they get the files they need?

Since we are still using nuspecs this has not really changed alot except for some paths. I did some tests with our new template but I will check it again after fixing the previous point.

One of the next steps we could do is generating the nuget packages from the csprojs directly since all csprojs are now net sdk projects. It will be much cleaner as we no longer need to have duplicate references and versions etc. We do have to modify the versionupdater and nightlybuilder for this though. I think this is something we should look into after v4 is released. #790

Barsonax commented 4 years ago

When selecting a sample project as a startup project, does it run the editor as expected, with all the sample stuff working?

So how I do it now is to copy the sample stuff over myself. This is the same as it was before right? I don't see any entries in the csproj that copies over all the content before we switched to net sdk style projects. @ilexp

I don't think it will be possible to launch the sample project directly. Else we will run into the same problem that caused us to add a gamelauncher and gameditor project to the duality template. Maybe it will work again when we switch to .net core. This is not really a big deal in this case though as you can just set the dualityeditor project as the start project.

ilexp commented 4 years ago

So how I do it now is to copy the sample stuff over myself. This is the same as it was before right? I don't see any entries in the csproj that copies over all the content before we switched to net sdk style projects. @ilexp

Before, you could run it directly. This was achieved by (I think, it changed one or two times) setting the working directory for debug sessions to the sample folder where its Data and custom Plugins were, but launching the editor from the Output folder.

This is not really a big deal in this case though as you can just set the dualityeditor project as the start project.

I'd argue that it kinda is though! Maybe not a big deal, but something that would be not great to lose. Using the editor would first require to create the entire "launch this, but use that working dir" setup, right? And to do that, you'd have to know that's a possibility and get the settings right, look up relative folder paths, be careful not to commit that, and so on.

From a developer usability side, it was super easy to just run and debug any sample from the IDE, and I regularly did this to verify more critical core changes, using the samples as extended test cases. If it becomes an inconvenient thing to do, it might diminish the cases where you'd typically rely on this as a diagnostic tool. It also makes sample development more difficult if there is no longer an established / documented way to test them.

But yeah, like you said:

Else we will run into the same problem that caused us to add a gamelauncher and gameditor project to the duality template. Maybe it will work again when we switch to .net core.

Seems we can't do anything about it though at this point - closest to getting there would be to manually change the working directory of the editor project, and explicitly selecting the editor application to run.

Maybe we can at least add a "sample launcher" project that is pre-configured to run the editor application from Output in the working directory of a sample? At least that way all you'd have to change is the working directory of the project, not create the entire setup. And it would document the possibility of this workflow in the repo, which might be the biggest advantage. The feature of having a different executing and working directory for the editor (or launcher) isn't otherwise apparent right now.

Barsonax commented 4 years ago

Maybe we can at least add a "sample launcher" project that is pre-configured to run the editor application from Output in the working directory of a sample? At least that way all you'd have to change is the working directory of the project, not create the entire setup. And it would document the possibility of this workflow in the repo, which might be the biggest advantage. The feature of having a different executing and working directory for the editor (or launcher) isn't otherwise apparent right now.

Agreed I will add something like a SampleRunner project

EDIT: another possible option would be to make the paths that duality uses to find plugins configurable instead of hardcoded. This would allow us to keep the csproj's clean, make it possible to load multiple samples and give more freedom to endusers in how to structure their project. Not for this PR though.

Barsonax commented 4 years ago

Added a sample runner project that by default has its launchSettings.json set in such a way that it will use the DualStickSpaceShooter sample @ilexp

SirePi commented 4 years ago

Tried it, it appears to be working properly: compiles, the runner runs.. everything looks good. Only thing the runner's player ship rotates randomly sometimes.. but I think it's nothing to do with the PR, unless it's some indication that something else is not working as it should?

ilexp commented 4 years ago

EDIT: another possible option would be to make the paths that duality uses to find plugins configurable instead of hardcoded. This would allow us to keep the csproj's clean, make it possible to load multiple samples and give more freedom to endusers in how to structure their project. Not for this PR though.

It's not just about plugins, but also the Data folder, default application and user settings, editor data - essentially the working directory. Flexibility is always good, but not sure it would be worth the added complexity, since using a different working directory already works.

Only thing the runner's player ship rotates randomly sometimes.. but I think it's nothing to do with the PR, unless it's some indication that something else is not working as it should?

Probably nothing to do with this PR like you said, but that still is weird. If you manage to reproduce this, maybe create a new issue to investigate. Could have something to do with input detection.

Added a sample runner project that by default has its launchSettings.json set in such a way that it will use the DualStickSpaceShooter sample @ilexp

Tried it, it appears to be working properly: compiles, the runner runs.. everything looks good.

Neat! Thanks 👍 Approving this PR. Might make sense for @SirePi to do a second review before merge, since this one is a bit bigger and six eyes see more than four.