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.

Sign up for GitHub

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

Jump to bottom

Update the state of C#/.NET support as of 4.2 #8142

Merged
merged 1 commit into from
Oct 3, 2023
Merged

Update the state of C#/.NET support as of 4.2 #8142

merged 1 commit into from
Oct 3, 2023

Conversation

YuriSizov
Copy link
Contributor

Add more details to the documentation to avoid confusion, and also remove a scary message that we don't really need anymore. See also godotengine/godot-website#671.

@YuriSizov YuriSizov added enhancement topic:dotnet area:about Issues and PRs related to the About section of the documentation and other general articles area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Oct 3, 2023
@YuriSizov YuriSizov added this to the 4.2 milestone Oct 3, 2023
@YuriSizov YuriSizov requested a review from raulsntos October 3, 2023 13:07
raulsntos
raulsntos reviewed Oct 3, 2023
View reviewed changes
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.

I feel like "experimental Android support" doesn't say much from the user's perspective. What does experimental mean to users? Maybe we should list the limitations of Android exports somewhere.

about/list_of_features.rst Outdated Show resolved Hide resolved
tutorials/scripting/c_sharp/c_sharp_basics.rst Outdated Show resolved Hide resolved
getting_started/step_by_step/scripting_languages.rst Show resolved Hide resolved
@YuriSizov
Copy link
Contributor Author

Maybe we should list the limitations of Android exports somewhere.

Sure, I'm not aware of any specificities, but I can add whatever makes sense to the notes. As for "experimental", I think it makes it pretty clear that it's in a working state but in need of public testing. But we can also link something relevant to its status if we have any external resources, or add some details as you suggest.

@raulsntos
Copy link
Member

By the way, I'm not against using the word "experimental" and I think it's the right word to use for the current status. My concern was only that it's not descriptive enough to users about the limitations, so adding some more details somewhere and linking to it sounds good to me.

Off the top of my head, the current limitations for Android exports are:

  • Only some architectures are supported: arm64 and x64.
  • The user project needs to target .NET 7.0 (which requires them modifying the .csproj).

@YuriSizov
Copy link
Contributor Author

  • The user project needs to target .NET 7.0 (which requires them modifying the .csproj).

I'll adjust my notes, but want to mention that this is probably something that needs to be described in more detail somewhere... Not sure where though. Somewhere in the C# parts of the docs, but not exactly sure where.

@YuriSizov YuriSizov force-pushed the csharp-support-update branch from 8984127 to f25a29e Compare October 3, 2023 15:51
@YuriSizov
Copy link
Contributor Author

@raulsntos Okay, I've added more details into notes, and also improved the index page for C# with a bit of structure, some renames for clarity and more details about platform support (which I link to from all those notes). Let me know what you think.

chrome_2023-10-03_17-50-27 chrome_2023-10-03_17-50-36

raulsntos
raulsntos reviewed Oct 3, 2023
View reviewed changes
tutorials/scripting/c_sharp/index.rst Outdated Show resolved Hide resolved
tutorials/scripting/c_sharp/index.rst

.. _doc_c_sharp_platforms:

C# platform support
Copy link
Member

Choose a reason for hiding this comment

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

I love the new index page, but I'm not sure if this is the best place for this section. We have similar sections in c_sharp_basics (Current gotchas and known issues or Common pitfalls) but I'm not sure if that's better, to be honest I think the c_sharp_basics page should probably be split into multiple pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know what you mean, and I was thinking removing some stuff from the basics page that can be now found on the index page... but ultimately decided that a bit of duplication is fine. These are likely to be the two landing pages, so it makes sense to have a bit of important information on both of them.

I do support reorganizing the docs, though. I'm not entirely sure why we have articles framed as introduction to C#. That's probably unnecessary and we could go straight into Godot-specific stuff (with some notes here and there that link Godot API to C# concepts).

I could give it a go as time allows, but it would be harder for me to find a good angle here since I'm neither a C# user nor maintainer 🙃 But I can help, if you're interested.

Copy link
Member

Choose a reason for hiding this comment

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

I would love that, maybe we can do that after the 4.2 release. For now I think this is good.

@YuriSizov YuriSizov force-pushed the csharp-support-update branch from f25a29e to ac728e8 Compare October 3, 2023 16:04
raulsntos
raulsntos approved these changes Oct 3, 2023
View reviewed changes
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.

Looks great, thanks for all the improvements to the C#/.NET documentation.

@mhilbrunner mhilbrunner merged commit 4413677 into godotengine:master Oct 3, 2023
@mhilbrunner
Copy link
Member

Thanks! Merged.

@YuriSizov YuriSizov deleted the csharp-support-update branch October 4, 2023 11:33
@raulsntos raulsntos mentioned this pull request Oct 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raulsntos raulsntos raulsntos approved these changes

@mhilbrunner mhilbrunner mhilbrunner approved these changes

Assignees
No one assigned
Labels
area:about Issues and PRs related to the About section of the documentation and other general articles area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement topic:dotnet
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
3 participants
@YuriSizov @raulsntos @mhilbrunner