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

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

Merged
merged 4 commits into from
May 5, 2020
Merged

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

merged 4 commits into from
May 5, 2020

Conversation

Barsonax
Copy link
Member

@Barsonax Barsonax commented Aug 19, 2019

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:

  • All projects except the 2 debugging visualizer projects in the tool solution are now sdk style
  • Members of some resource files are now internal

Completes #737

@Barsonax Barsonax changed the title Feature/sdkcsprojs Changed netframework projects to use .net sdk style csprojs Aug 19, 2019
@Barsonax Barsonax changed the title Changed netframework projects to use .net sdk style csprojs Changed some netframework projects to use .net sdk style csprojs Aug 19, 2019
@ilexp ilexp added this to the C# / .NET Upgrade milestone Aug 19, 2019
@ilexp ilexp added DevTool Area: Development tools and environment Task ToDo that's neither a Bug, nor a Feature labels Oct 29, 2019
@Barsonax Barsonax marked this pull request as draft April 22, 2020 18:04
@Barsonax
Copy link
Member Author

Barsonax commented Apr 24, 2020

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

@Barsonax Barsonax linked an issue Apr 24, 2020 that may be closed by this pull request
@Barsonax Barsonax marked this pull request as ready for review April 27, 2020 11:14
@Barsonax Barsonax requested review from ilexp and SirePi April 27, 2020 11:14
@ilexp
Copy link
Member

ilexp commented Apr 30, 2020

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):

  • (Make sure to clean all existing output before testing)
  • Does everything compile?
  • Is the Build/Output folder populated as expected?
  • When selecting a sample project as a startup project, does it run the editor as expected, with all the sample stuff working?
    • Make sure to check one that carries both content and source code.
    • I usually go for the Tilemaps sample, followed by the DualStickSpaceShooter to tick two different complexity scenarios.
  • When building nuget packages, do they end up fine, e.g. did they get the files they need?

@Barsonax
Copy link
Member Author

Barsonax commented Apr 30, 2020

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

Barsonax commented Apr 30, 2020

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

ilexp commented May 1, 2020

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

Barsonax commented May 4, 2020

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

Barsonax commented May 4, 2020

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

SirePi commented May 4, 2020

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

ilexp commented May 5, 2020

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.

@Barsonax Barsonax merged commit 1138d79 into AdamsLair:master May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DevTool Area: Development tools and environment Task ToDo that's neither a Bug, nor a Feature
Development

Successfully merging this pull request may close these issues.

Consider switching to .NET SDK style projects
3 participants