godotengine / godot-csharp-visualstudio

Godot C# extension for Visual Studio
MIT License
251 stars 31 forks source link

Fix launch issue when GodotProjectDir not same as SolutionDir #25

Open paiden opened 2 years ago

paiden commented 2 years ago

If the Solution for a Godot project is not the same as the project directory the extension will fail to launch the debug target with a MessageBox showing 'No Godot editor instance connected'.

The reason is that the extension takes SolutionDir as the Godot project dir. While this hold true for simple default Godot projects this must not be the case. In complex projects developers may restructure their projects into a alternative structure, where the solution may be at another location.

The *.csproj file has the MSBuild property 'GodotProjectDir'. Now the value of this property is fetched and will be used as the project dir.

Note: Godot itself has some issues with projects varying to the default project. To fix this godot issues, modifications in the GodotSDK MSBuild props file have to be done.

neikeq commented 2 years ago

Currently, Godot doesn't support this. It expects both the solution and csproj to be at the Godot project root (next to the project.godot). I plan to change this for Godot 4.0, but it's unlikely to ever change for Godot 3.x.

Note: Godot itself has some issues with projects varying to the default project. To fix this godot issues, modifications in the GodotSDK MSBuild props file have to be done.

Yes, the Sdk makes these assumptions because the Godot editor does.

paiden commented 2 years ago

So what does this mean for this PR? Do you consider merging it? Or not, because this fix is currently not needed?

IMO this way of resolving the GodotProjectDir is the more MSBuild like way of doing so (considering MSBuild does not know what Solutions are 😉 ). Also while not needed for Gotdot3 this fix should be fully compatible and not break anything, but should also work with Godot versions that allow a more flexible Project structure in the future (as long as the property name stays the same).

BTW: Godot3 may not officially support SlnDir!=ProjDir. The workaround to make it work was to this small change

    <GodotProjectDir Condition=" '$(SolutionDir)' != '' ">$(SolutionDir)</GodotProjectDir>
    <GodotProjectDir Condition=" '$(SolutionDir)' == '' ">$(MSBuildProjectDirectory)</GodotProjectDir>

to

    <GodotProjectDir>$(MSBuildProjectDirectory)</GodotProjectDir>

I did not test everyting, but all I need for now (some custom nodes, addins etc.) work fine as far as I can tell.

Note that the first XML fragment is a little weird under the assumption that SolutionDir == ProjectDir. In that case you can just use ProjectDir that always exists anyway. It will have the same value as SolutionDir but unlike that one, it is always defined.

Its fine if you say you will not merge this. In that case sorry for the trouble. I did not find any contribution guidelines that define what the PR process is. If so, I will make a forked version of the addin, where I will also do a second fix for #10.

BenMcLean commented 2 years ago

Godot3 may not officially support SlnDir!=ProjDir.

What I do with Godot Mono is this:

In my object tree in the Godot editor, I have one object, called Main and I attach to it one script, called Main.cs. Everything else goes in folders and is built entirely from code at runtime.

I understand that this is not the way Godot was designed to work because it's designed to be a game engine that takes care of everything for you, like Unity or Unreal and I'm forcing it to work more like a framework such as libGDX. But I didn't start form the assumption that I wanted Godot: I started from the assumption that I wanted open source, C#, VR support and spend as little time as possible on low level hardware stuff so Godot was the only option at the time. I'd probably go with StereoKit or something else if I was starting from scratch right now. (no offense to Godot)

Anyway, my point is that your project can minimally conform to the standard and still work.

neikeq commented 2 years ago

I may have misunderstood this. Is it only about the solution being on a different directory and not the csproj? The Godot editor still expects a solution to be in the project root, but I suppose you have another one you use in Visual Studio. That should be possible then, so this change makes sense. We can update the Sdk too, although you can already make this work by adding <GodotProjectDir>$(MSBuildProjectDirectory)</GodotProjectDir> in a Directory.Build.props.

paiden commented 2 years ago

Currently the extension does this:

// Pseudocode
_godotProjectDir = ActiveSolution.Directory

A project for that IsGodotProject() is true, has a MSBuild Prop $(GodotProjectDir) that already holds a value that represents this directory.

So in my opinion doing this:

// Pseudocode
_godotProjectDir = godotProject.$(GodotProjectDir)

is the correct/idiomatic approach to determine the project directory for a Godot project, while the former approach works because of implementation details.

My Godot project runs completely fine in the GodotEditor. The Visual Studio extension refuses to launch it. The PR applies changes, so that the extension will launch my project and keep launching all projects it could launch prior to this. So I'd argue this is an improvement (as long as this change does not introduce regressions).

So my impression is, this may be an up-stream worthy fix. So I opened this PR. Now the maintainer of this project has to decide if this really is an up-stream worthy change - weight risk of regression vs. impact of improvements ... - and decide accordingly.

If, Godot itself supports Solutions in some other Directory, or whatever MSBuild tricks are needed to make it work, is IMO not relevant for the PR at hand.

This is the best I can do, to articulate why I opened this PR (I'm not a native English speaker).

GeorgeS2019 commented 2 years ago

@paiden could U join here to add your idea of PR and if possible UPDATE this view to the dotnet6 Godot4