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

Merge .NET 6 branch with master #64089

Merged
merged 31 commits into from
Aug 22, 2022
Merged

Merge .NET 6 branch with master #64089

merged 31 commits into from
Aug 22, 2022

Conversation

neikeq
Copy link
Contributor

@neikeq neikeq commented Aug 8, 2022

The changes from the dotnet6 branch are ready to be merged, and future development should happen in the master branch.

I rebased the changes carefully to make sure commits can be bisected. I was careful not to remove any changes merged to master while I was working on this branch, but some things changed a lot during that time. As such, I had to re-write quite a few of these changes, so there's a risk for regression there.

For building and testing these changes, please read the README I added to the module. It may be of help to avoid confusion now that we distribute our assemblies as NuGet packages.

If you've been used C# on the master branch recently, expect heavy breaking changes. Don't expect projects to run at all out of the box without fixes. Make sure to back up your projects before attempting to open them. Remember this is still unstable.

I don't know if the CI changes are welcome. The main difference is the use of actions/setup-dotnet@v1 and that it now builds the C# projects after generating the glue. In the future, setup-dotnet will only be required for building the C# projects, so if doing that is not desired then we could get rid of it. However, right now it's still needed for building the engine (until we replace our libdotnet dependency that's used by the Godot editor to locate the .NET Sdk).

@Byteron
Copy link
Contributor

Byteron commented Aug 8, 2022

building this on windows fails.
image

adding

        if env["platform"] == "windows":
+           env_mono.Append(CPPDEFINES=["NETHOST_USE_AS_STATIC"])

to the mono_configure.py file seems to fix it.
image

@Byteron
Copy link
Contributor

Byteron commented Aug 8, 2022

and this step does not seem to be working for me either.
image

after adding a target, it works, though.

@Byteron
Copy link
Contributor

Byteron commented Aug 8, 2022

Compiling the binary and generating the glue seems to be successful. I can open the project manager, but upon opening a newly created project the editor just crashes.
image
Looks like it's trying to load the assembly, but it's not there yet since nothing has been compiled at this point in time.

@lewiji
Copy link
Contributor

lewiji commented Aug 8, 2022

@neikeq testing the editor out now on Linux, if I spot any issues is it best to keep a hold of them until this PR has been merged, then create a new issue? Don't want to spam this thread with stuff that's not related to the merge.

modules/mono/mono_gd/gd_mono.cpp Outdated Show resolved Hide resolved
modules/mono/mono_gd/gd_mono.cpp Outdated Show resolved Hide resolved
modules/mono/mono_gd/gd_mono.cpp Outdated Show resolved Hide resolved
@neikeq
Copy link
Contributor Author

neikeq commented Aug 8, 2022

@neikeq testing the editor out now on Linux, if I spot any issues is it best to keep a hold of them until this PR has been merged, then create a new issue? Don't want to spam this thread with stuff that's not related to the merge.

For Linux issues I think it's better to wait for this to be merged, as it's the platform I've tested most and I think it's in a good enough state. But it's fine if it's something serious.

@neikeq
Copy link
Contributor Author

neikeq commented Aug 8, 2022

and this step does not seem to be working for me either.
image

after adding a target, it works, though.

What do you mean by adding a target?
The error seems to be about the dot, because the example is in bash. Maybe I should add a Windows one.

Compiling the binary and generating the glue seems to be successful. I can open the project manager, but upon opening a newly created project the editor just crashes.
image
Looks like it's trying to load the assembly, but it's not there yet since nothing has been compiled at this point in time.

The exception is expected. It shouldn't cause any issues other than printing the error, so I didn't bother tweaking that yet. Perhaps the crash happens somewhere after that, for a different reason. EDIT: Ah, yes. The cause is the second exception. In any case, I have to test this better on Windows.

@lewiji
Copy link
Contributor

lewiji commented Aug 8, 2022

The exception is expected. It shouldn't cause any issues other than printing the error, so I didn't bother tweaking that yet. Perhaps the crash happens somewhere after that, for a different reason. EDIT: Ah, yes. The cause is the second exception. In any case, I have to test this better on Windows.

FWIW, I sent @Byteron a binary I compiled on Windows using the VS2022 x64 developer command prompt, and that worked as expected on their end. Unsure what the difference in build environments was, but that build is working pretty well on Windows (after applying the NETHOST_USE_AS_STATIC cpp const to the scons mono environment).

Just a quick report: Windows users were noticing [Export] annotated variables being duplicated, after searching the issue board I see that's a bug related to recent changes in the inspector UI and not to do with the dotnet6 port.

However, on my Linux machine I noticed that [Export]s from C# don't appear at all in the inspector, whereas they do from GDScript, and I didn't see any reports about that in the related issues to the duplicating behaviour. @neikeq or any other Linux users, can you repro? It seems weird that the behaviour would be different between the two platforms, it's the same code path. Just a simple [Export] public float SomeFloat { get; set; } doesn't show up in my linuxbsd dotnet6 editor, not after closing and reopening the scene or even the entire editor, on EndeavourOS, and nothing in the log to show for it. But, it could be a bug completely unrelated to this PR's changes, I suppose.

@GeorgeS2019
Copy link

GeorgeS2019 commented Aug 8, 2022

that build is working pretty well on Windows

I have a similar experience with Windows built using MSVC

@GeorgeS2019
Copy link

@reduz
From your Twitter about this merge

The way this new one is done is really awesome because it allows to optionally use it in the regular editor builds without actually having to compile against it.

Can someone elaborate on this? Very curious!

@neikeq
Copy link
Contributor Author

neikeq commented Aug 9, 2022

The way this new one is done is really awesome because it allows to optionally use it in the regular editor builds without actually having to compile against it.

Can someone elaborate on this? Very curious!

This is likely about the "Editor and export template unification (no more Classic vs C# versions)" task in the roadmap. While most of the work was already done in the process of moving to .NET 6, it's still not complete. Check the description in the roadmap task for what's missing.

@eumario
Copy link

eumario commented Aug 9, 2022

@neikeq I have to say thank you. I just tested the Dotnet6 version on Windows, doing a simple HTTPS connection, and it worked perfectly! This solves a long standing issue with bug #36958 that affects Godot, and Unity. Congrats on the work, and continued work on bringing Dotnet 6 into the Godot engine.

@neikeq neikeq force-pushed the dotnet6 branch 3 times, most recently from e1e1a75 to 7151ed4 Compare August 9, 2022 08:27
@neikeq
Copy link
Contributor Author

neikeq commented Aug 9, 2022

FWIW, I sent @Byteron a binary I compiled on Windows using the VS2022 x64 developer command prompt, and that worked as expected on their end. Unsure what the difference in build environments was, but that build is working pretty well on Windows (after applying the NETHOST_USE_AS_STATIC cpp const to the scons mono environment).

I added the NETHOST_USE_AS_STATIC env var.

Just a quick report: Windows users were noticing [Export] annotated variables being duplicated, after searching the issue board I see that's a bug related to recent changes in the inspector UI and not to do with the dotnet6 port.

This should be fixed now.

However, on my Linux machine I noticed that [Export]s from C# don't appear at all in the inspector, whereas they do from GDScript, and I didn't see any reports about that in the related issues to the duplicating behaviour. @neikeq or any other Linux users, can you repro? It seems weird that the behaviour would be different between the two platforms, it's the same code path. Just a simple [Export] public float SomeFloat { get; set; } doesn't show up in my linuxbsd dotnet6 editor, not after closing and reopening the scene or even the entire editor, on EndeavourOS, and nothing in the log to show for it. But, it could be a bug completely unrelated to this PR's changes, I suppose.

I mainly use Linux and haven't experienced this. @lewiji It could be specific to your project. I can have a look if you don't mind sharing.


namespace Godot.SourceGenerators
{
// TODO: May need to think about compatibility here. Could Godot change these values between minor versions?
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be changes to the existing enum values between minor versions, but there might be new enum values that would be added.

There might be compat breaking changes before 4.0 is released though as especially PropertyHint and PropertyUsageFlags might benefit from a cleanup pass to remove obsolete ones / rename some / reorder in a better structure.

For VariantType, I think it's unlikely to change at this stage.

Would CI pick it up if any enum value is added to core and not reflected here, or if values go out of sync?

Copy link
Contributor Author

@neikeq neikeq Aug 23, 2022

Choose a reason for hiding this comment

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

We can generate these enums in bindings_generator.cpp just like we do for the API. This way, it will be up-to-date, and if there are usages of removed values in the project, the CI will fail as I added a step to build the C# projects.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

This looks great!

Thanks @neikeq for the amazing work porting Godot to .NET 6, and to everyone else involved for the support testing, fixing and improving the dotnet6 branch.

Let's merge and move further bug reports and enhancement proposals to their dedicated issues.

@akien-mga akien-mga merged commit 8a1e598 into godotengine:master Aug 22, 2022
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 think this is ready to be merged, I gave it a quick last check and didn't find any big problems and it also seems to fix all issues mentioned (at least I couldn't reproduce them in Linux).

Just wanted to add a few comments but they're not blockers.

EDIT: Oops, it was merged while I was reviewing 😄

@aaronfranke
Copy link
Member

The documentation needs to be updated to account for the changes in this PR.

@DeafMan1983
Copy link

Does it work only unmanaged delegate ( like static delegate *unmanaged[Cdecl]<return_type, type> pfnFunc )?

I hope it works for Godot 4.x, too.

I expected Unity3D has only .Net Standard/Core 2.x and Godot 4.x uses .Net 6.x, 7.x or 8.x ???

Thanks!

@ozanyasindogan
Copy link

@neikeq I have to say thank you. I just tested the Dotnet6 version on Windows, doing a simple HTTPS connection, and it worked perfectly! This solves a long standing issue with bug #36958 that affects Godot, and Unity. Congrats on the work, and continued work on bringing Dotnet 6 into the Godot engine.

Godot 4.2.rc1.mono, I still can't use any .Net function which requires SSL/TLS authentication in published apps to
Android platforms. Although it works perfectly fine under Windows.
Please check this link for details.

@eumario
Copy link

eumario commented Nov 22, 2023

@neikeq I have to say thank you. I just tested the Dotnet6 version on Windows, doing a simple HTTPS connection, and it worked perfectly! This solves a long standing issue with bug #36958 that affects Godot, and Unity. Congrats on the work, and continued work on bringing Dotnet 6 into the Godot engine.

Godot 4.2.rc1.mono, I still can't use any .Net function which requires SSL/TLS authentication in published apps to Android platforms. Although it works perfectly fine under Windows. Please check this link for details.

This isn't the same issue as what you are experiencing in 4.2. The issue previously, was that with 3.x, it was using the Mono Project's release of the Mono Runtime for Linux, and there is an outstanding issue in the 3.x tree, that any attempt to use C# SSL Network Communications would fail. By Default, Mono does not define a Central Repo of Master Authority Certificates in which to validate SSL Certificates from the web, and will not use the one installed on the host operating system, causing SSL Certificate validation to always fail.

After reviewing the discussion on #84559, the issue is not this. It is the fact that the Dotnet Runtime cannot locate libssl.so, in order to access the functions needed to do any SSL cryptography, or Communication. Which is an issue that is entirely different. This is the first release of Godot 4.x tree, that has been able to be exported to Android, so there are still going to be outstanding issues with that. But make sure you look through the bug reports, to understand what specifically is going on.

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.