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

Add support for vscode #160

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add support for vscode #160

wants to merge 3 commits into from

Conversation

Mrcubix
Copy link

@Mrcubix Mrcubix commented Jul 26, 2023

Premises

vscode cannot make use of the Properties/launchSettings.json file as it uses an unsupported commandName, only project is supported in vscode.

What is this PR for

This PR aim to add launch & debug support of modules in vscode.

Necessary files

To achieve this, 3 files are necessary:

  • launch.json
  • settings.json
  • tasks.json

Only a single of those files, launch.json, requires the use of variables to make a newly created module, work properly.
those variables are stored in the settings.json files and is used by vscode to feed the appropriate values.

the 2 variables that are required are:

  • The game binary folder (by default, Win64_Shipping_Client)
  • The name of the module to debug

Unfortunately, the addition of 3 template symbols, including a single parameter are required.

Template symbols

-c or --code will need to be used if the user intend to use vscode as their IDE.

Generated type symbols are used to replace the placeholders present within settings.json initially.

  1. gameBinariesFolder:

If the gameWindows parameter is true, gameBinariesFolder's value is the Win64_Shipping_Client folder, if not then,
If the gameWindowsStore parameter is true , gameBinariesFolder's value is the Gaming.Desktop.x64_Shipping folder.

  1. displayName:

a separate symbol to moduleName that control its presence, in the case that it is not, sourceName will be used (the name symbol represent that value)

Folder exclusion

In the case that the user specify the use of vscode, the Properties folder will be excluded from the template.
The opposite, will be true if the user does not specify any values or false, .vscode will be excluded.

What could be done on top of what is currently provided

If necessary, i can add the publish and watch tasks back to tasks.json

@Aragas
Copy link
Member

Aragas commented Jul 27, 2023

This adds some concerns if we're implementing it this way

  • There are two independent ways for configuration - MSBuild and VSCode
  • Hardcoded gameBinariesFolder - if you change the BANNERLORD_GAME_DIR env, you'll also need to change the variable

I think that adding a new VSCode specific templates for Standard/Sdk would be better - we could move all of the dynamic variables stored in MSBuild to settings.json. When building, MSBuild will look into settings.json for the values.
I should be able to adapt the changes as a new project if you're not able. It kinda doubles the codebase, but it should provide a better user experience

Also, as a comment, I think it would be better to file a bug report to VSCode/ C# Dev Kit extension for the lack of executable support in commandName. Looks like someone reported an IIS error and it's being fixed

@Mrcubix
Copy link
Author

Mrcubix commented Jul 27, 2023

  • There are two independent ways for configuration - MSBuild and VSCode
  • I think it would be better to file a bug report to VSCode/ C# Dev Kit extension for the lack of executable support in commandName. Looks like someone reported an IIS error and it's being fixed
  • I think that adding a new VSCode specific templates for Standard/Sdk would be better

https://code.visualstudio.com/docs/csharp/debugger-settings#_launchsettingsjson-support

Seems like vscode originally planned it to be this way, i agree that mixing both configs isn't the best of ideas, that would reduce the complexity of templates, however, that will make maintaining the templates more difficult.

  • Hardcoded gameBinariesFolder - if you change the BANNERLORD_GAME_DIR env, you'll also need to change the variable

I don't really see what your concern is about here, this would mostly be an issue if the BANNERLORD_GAME_DIR variable isn't set properly, or if you don't restart vscode after changing the variable.
If you move the game to another location on steam or the store, the binary folder will stay the same.

  • we could move all of the dynamic variables stored in MSBuild to settings.json. When building, MSBuild will look into settings.json for the values.

This would require the use of an msbuild task just for that purpose, which is somewhat confusing to use, even with the documentation provided.
I will try playing around with it since, at least, it has a more proper documentation than templates does.

EDIT: i may have misunderstood what you meant for that last part.

@Aragas
Copy link
Member

Aragas commented Jul 29, 2023

I've added my own interpretation of VSCode, but as you said, MSBuild code is a mess, especially the way to read JSON without any JSON dependency
https://github.com/BUTR/Bannerlord.Module.Template/tree/Mrcubix_master_BUTR

@Mrcubix
Copy link
Author

Mrcubix commented Jul 29, 2023

you should be able to make use of System.Text.Json just for this task, it shouldn't pose any issues, or worst case scenario, you can make use of the game's newtonsoft dependency.

@Aragas
Copy link
Member

Aragas commented Jul 29, 2023

you should be able to make use of System.Text.Json just for this task, it shouldn't pose any issues, or worst case scenario, you can make use of the game's newtonsoft dependency.

Surprisingly, System.Text.Json isn't available in MSBuild because the target is netstandard2.0, as I understand

Reusage of Newtonsoft could work when the game folder is available, and reference assemblies are not set, but there are other issues
We need Newtonsoft.Json to get the Game folder where Newtonsoft.Json is, so that's a paradox. It won't also work with having Newtonsoft.Json as a NuGet package, because we need to resolve the game folder before the project dependencies are resolved.
So we need some built-in mechanism to read Json without external deps, where System.Text.Json should have helped

@Mrcubix Mrcubix marked this pull request as draft August 1, 2023 22:40
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

Successfully merging this pull request may close these issues.

None yet

2 participants