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

Provide a convenient way to await thread finish. #69796

Conversation

Daylily-Zeleen
Copy link
Contributor

Implement #5918 and instead of #5893

@Daylily-Zeleen Daylily-Zeleen requested review from a team as code owners December 9, 2022 05:43
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/privide_convenient_way_to_await_thread_finish branch from 02d98d8 to 5672e47 Compare December 9, 2022 05:46
@Chaosus Chaosus added this to the 4.x milestone Dec 9, 2022
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/privide_convenient_way_to_await_thread_finish branch from 5672e47 to 46b2bd1 Compare December 9, 2022 06:13
@Daylily-Zeleen
Copy link
Contributor Author

Well......how can I handle this error, I have not develope experience of mono module.

@raulsntos
Copy link
Member

I think the await_to_finish method you add in this PR is the first one in Godot's API that returns a Signal, the .NET bindings generator seems unable to handle this situation since we never had this case in the API before and generates invalid C#.

Also, is this new API really needed? Can't you just await the signal?

var thread := Thread.run(do_something_heavy.bind(count))
var result = await thread.finished

@Daylily-Zeleen
Copy link
Contributor Author

I think the await_to_finish method you add in this PR is the first one in Godot's API that returns a Signal, the .NET bindings generator seems unable to handle this situation since we never had this case in the API before and generates invalid C#.

Also, is this new API really needed? Can't you just await the signal?

var thread := Thread.run(do_something_heavy.bind(count))
var result = await thread.finished

In order to not hurt the origin feature, a way to start async process is needed.

Of course, we can avoid to return a signal to deal this problem.
For example, use func start_await() -> Thread instead of func await_to_finish() -> Signal, and use it like this

var result := await Thread.run(do_something_heavy.bind(count)).start_await().finished

But Signal is type of Godot Variant, for completeness of functionality, we should provide the ability of handle Signal for C#( I don't know the detail of C# in Godot). Even if we limit ourselves avoid this situation, we can't restrict developers to return a Signal.

@neikeq
Copy link
Contributor

neikeq commented Dec 10, 2022

We do have support for Signal in the C# bindings generator, but this is the first time it's used and there seems to be an error. That can be easily fixed. It would take a bit more work to turn the C# type into an awaitable.

Regarding this new API, since this is the first time it's done this way, we need to decide whether it's the right way.

It seems the returned signal acts as a Future. I'm not a fan of it, mainly because Signal currently lacks typing support (Signal<ParamType>).

Additionally, it could be great to have a more specific API for this (futures and awaitables), ideally one that would work in C++ as well with co_await. This could always be introduced in the future, though, so it doesn't worry me as much.

@neikeq
Copy link
Contributor

neikeq commented Dec 11, 2022

This fixes the C# bindings:

itype.c_out = "%5return " C_METHOD_MANAGED_FROM_SIGNAL "(&%1);\n";

-	itype.c_out = "%5return " C_METHOD_MANAGED_FROM_SIGNAL "(&%1);\n"; 
+	itype.c_out = "%5return " C_METHOD_MANAGED_FROM_SIGNAL "(in %1);\n"; 

The C# type needs improvement (rename it from SignalInfo to Signal and make it awaitable), but it's already usable as is.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/privide_convenient_way_to_await_thread_finish branch 2 times, most recently from a4c4ce6 to 08aac77 Compare December 19, 2022 15:10
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/privide_convenient_way_to_await_thread_finish branch from 08aac77 to cf093f8 Compare December 19, 2022 15:12
@Daylily-Zeleen Daylily-Zeleen deleted the daylily-zeleen/privide_convenient_way_to_await_thread_finish branch December 19, 2022 15:13
@Daylily-Zeleen Daylily-Zeleen restored the daylily-zeleen/privide_convenient_way_to_await_thread_finish branch December 19, 2022 15:13
@Daylily-Zeleen Daylily-Zeleen deleted the daylily-zeleen/privide_convenient_way_to_await_thread_finish branch December 19, 2022 15:13
@Daylily-Zeleen
Copy link
Contributor Author

Replaced by #70299.

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.

6 participants