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

Port dodge the creeps C# to Godot 4 #870

Closed
wants to merge 13 commits into from
Closed

Conversation

van800
Copy link
Contributor

@van800 van800 commented Mar 2, 2023

I have used corresponding changes from #866

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for updating this demo!

I have a PR that updates the documentation tutorial for this demo, but I didn't update the demo yet so I appreciate it.

Comment on lines +6 to +11
<ItemGroup>
<None Include="**/*.tscn" />
</ItemGroup>
<ItemGroup>
<None Include="project.godot" />
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this is used by some third-party source generator, analyzer or completion provider? I'm not sure we want to add it to the project then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Rider, if you don't include files in the csproj, it is harder to see them and edit them. I suppose the same would be in the Visual Studio.

Copy link
Member

@raulsntos raulsntos Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I don't know about Rider or Visual Studio since I don't use them. Is it useful to edit these files manually from the IDE though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was updating this example, I had to edit them a lot.

Copy link
Contributor Author

@van800 van800 Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a gdscript plugin for Rider/IDEA, which has some features working, when scenes are included in the csproj.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like plugins should be able to recommend users to add these to the csproj if they need it.

For example, when you open a C# project in VSCode you get a little notification that asks you if you want to add tasks.json and build.json files to configure debugging:

Screenshot

image


When I was updating this example, I had to edit them a lot.

Yes, but I'm assuming this is only common when porting projects between major versions which should be pretty rare. Otherwise, if there are any changes between minor versions, the project converter should be able to do it for you.

Also, doesn't Rider allow you to edit these files without adding them to the csproj? Maybe changing the explorer view to FileSystem?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There is at least one feature in Rider, which would only work with scenes added to the csproj. Run game with the scene.
    image

  2. I can't check Visual Studio for now, my windows machine is out-of-reach for some time.

mono/dodge_the_creeps/HUD.cs Outdated Show resolved Hide resolved
mono/dodge_the_creeps/Player.cs Outdated Show resolved Hide resolved
mono/dodge_the_creeps/Player.cs Outdated Show resolved Hide resolved
van800 and others added 3 commits March 2, 2023 14:53
Co-authored-by: Raul Santos <raulsntos@gmail.com>
Co-authored-by: Raul Santos <raulsntos@gmail.com>
Co-authored-by: Raul Santos <raulsntos@gmail.com>
@van800 van800 changed the base branch from 4.0 to master March 2, 2023 14:35
van800 and others added 3 commits March 2, 2023 17:43
Co-authored-by: Raul Santos <raulsntos@gmail.com>
Co-authored-by: Danil Alexeev <danil@alexeev.xyz>
Co-authored-by: Raul Santos <raulsntos@gmail.com>
@aaronfranke
Copy link
Member

aaronfranke commented Apr 12, 2024

Thank you for your work on this! I spent a few hours resolving the differences between this PR and #1000, ensuring best practices, no regressions, and consistency with the tutorial in the documentation.

Ultimately what I ended up with was quite different from both PRs. I have pushed the consolidations to both PRs so that you can view them, but I can only accept one PR. As an arbitrary choice, I will be accepting PR #1000 and closing this one. I will squash-and-merge and include you as a co-author.

@van800
Copy link
Contributor Author

van800 commented Apr 12, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants