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

Beginning of AsyncGlk implementation #39

Draft
wants to merge 4 commits into
base: garglk
Choose a base branch
from

Conversation

curiousdannii
Copy link
Contributor

@curiousdannii curiousdannii commented Feb 26, 2023

This was meant to be a draft, but I can't see how to change it to a draft.

@curiousdannii curiousdannii changed the base branch from garglk to master February 26, 2023 06:33
@curiousdannii curiousdannii changed the base branch from master to garglk February 26, 2023 06:34
@curiousdannii curiousdannii changed the title AsyncGlk implementation Beginning of AsyncGlk implementation Feb 26, 2023
@awlck awlck marked this pull request as draft February 26, 2023 08:43
@curiousdannii
Copy link
Contributor Author

curiousdannii commented Apr 16, 2023

I'm now getting this error: Cannot wait on monitors on this runtime.

Which is from the code

Task t = AsyncGlk_imports.glk_select();
t.Wait(-1);

So I think I can't wait for a task like that, instead I have to use await. Which means turning glk_select and all the functions in the call tree into async functions.

@awlck
Copy link
Owner

awlck commented Apr 16, 2023

JS and .NET-in-WASM are inherently single-threaded, unfortunately. Task.Wait literally just means "block this thread until that task is done", but since there is no other thread available that could ever execute the task itself, we're not allowed to do this.

Really, this is the reason why I wasn't too keen on making the JS/WASM interface a reality. The Adrift runner was built on WinForms' 'old-school' model of synchronous calls with nested event loops, and the existing standalone FrankenDrift apps use the Eto.Forms library specifically because it supports that model. I'll have to see whether it's possible to just introduce async/await everywhere without comprehensively destroying the existing frontends, or if we have to break this out into a whole separate "AsyncFD" project.

(Either way, should Campbell Wild ever decide to make any changes to the Adrift code, this change means integrating them into FD will be even more of a nightmare than I imagine it already is.)

Side note: how do the other wasm-compiled interpreters (tads, git, scare, etc.) handle this? I'd expect them to face similar issues...

@curiousdannii
Copy link
Contributor Author

Looking at how the Glue API was designed, I think the async can be restricted to GlkRunner. I don't think the Adrift core or the Eto Forms runner will be impacted. If I'm wrong then we'll have to reconsider...

The Emglken terps use Asyncify to run because the C Glk API is blocking. I don't think that's an issue with the dotnet browser-wasm because C# does have async/await. It's just one particular way of using async that won't work.

Browser-wasm seems to run a .net interpreter compiled with Emscripten. So that means any C# async shouldn't be an issue despite the browser running single threaded, as it's being interpreted rather than running directly.

@curiousdannii
Copy link
Contributor Author

curiousdannii commented Apr 17, 2023

So the changes required actually seem quite minor. So far I've only had to change these functions to be async:

  • AppendHTML
  • GetCharInput
  • GetLineInput
  • Run

I also changed it from directly calling glk_select to a helper function GetEvent, which just makes it so that it returns an event rather than taking one in, as async functions can't have ref arguments.

        public Task<Event> GetEvent() {
            Event ev = new() { type = EventType.None };
            Garglk_Pinvoke.glk_select(ref ev);
            return Task.FromResult(ev);
        }

I ran into one hiccup which is that you can't await in an unsafe context, meaning the fixed part of GetLineInput. But I found that I can directly make a pinned array with GC.AllocateArray, which seems a little cleaner than using fixed and pointers. I don't know if this matters, but the functions don't need to be marked as unsafe any more!

I keep finding places where _output isn't created, and so have to add code to

if (_output is null) {
    _output = new GlkHtmlWin(GlkApi);
}

From what I can tell, the windows are delayed only because of SetBackgroundColour. But I suspect with the Gargoyle Glk extensions this isn't necessary, we should be able to set the colour via calling garglk_set_zcolors and then clearing the window. All the Glk implementations we're currently thinking about for GlkRunner support the Gargoyle Glk extensions so it's fine to depend on them. If we set colours that way then we can create the windows at the beginning and not have to keep checking if they're null or not.

@curiousdannii
Copy link
Contributor Author

Looking at how the Glue API was designed, I think the async can be restricted to GlkRunner. I don't think the Adrift core or the Eto Forms runner will be impacted. If I'm wrong then we'll have to reconsider...

<waitkey> being something that can just be printed means I'm wrong and this will be much more messy than I was hoping. But maybe I can salvage it...

@curiousdannii
Copy link
Contributor Author

Actually it turns out threads are supported in browser-wasm, but it's looking possibly more difficult than making things async.

@curiousdannii
Copy link
Contributor Author

curiousdannii commented Apr 17, 2023

Okay, so I have basic line and character input working, and I only needed to asyncify these extra functions:

  • RunnerSession.Display
  • RunnerSession.OpenAdventure
  • SharedModule.Source2HTML

What I've done is duplicate Display into DisplayWaiting. If Display is called with <waitkey> then it shows an error. This means that we can restrict converting the calls to DisplayWaiting for only the instances where it might actually result in a <waitkey>. And error and stacktrace will be generated so that we can see which functions need to be converted.

But I'm actually cheating... Restart calls OpenAdventure, but I didn't make Restart async as SystemTasks calls Restart, and therefore the whole of the parser would need to be async.

It feels fragile. Probably the solution will need everything to be converted to async.

I really wish printing couldn't result in IO events! Why do so many IF platforms support it?!

@awlck
Copy link
Owner

awlck commented Apr 17, 2023

I really wish printing couldn't result in IO events! Why do so many IF platforms support it?!

Then again, with the graphical nature of the Adrift editor I'm not sure where else you could stick the option to achieve similar flexibility.

Also, it's almost a tradition for Adrift5 games (especially those by Larry Horsfield) to make their opening stanza a sort of title screen with lots of key-waiting; not supporting it from within OpenAdventure will probably make everyone mad at us 😅

@curiousdannii
Copy link
Contributor Author

Maybe it wouldn't really make a difference, but if the printing functions only ever just printed text and there were distinct functions that did the IO stuff then it would feel simpler! ;)

So I've made most of the parser async now, and while a bit repetitive, it feels much safer to me. We are increasing the difference from upstream Adrift, but it is isolated to just a few files. I think it will be worth it.

@curiousdannii
Copy link
Contributor Author

curiousdannii commented Apr 21, 2023

I tried doing it properly and making all the callers async, and it's just never ending. The code is so entwined and everything calls everything else. I think almost all of it would end up having to be converted. So I've given up. I pushed curiousdannii@b89b075 which is where I stopped.

I'm going to go back to the threads option, see if I can work out how to get it working.

Also VB not having destructuring is annoying!

@curiousdannii
Copy link
Contributor Author

curiousdannii commented Apr 21, 2023

I think we need to wait for dotnet/runtime#68162 before the threading option is viable. Which means no Adrift 5 in JS for a while! So I'm going to shift my focus to adding some polish to the general GlkRunner implementation.

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

Successfully merging this pull request may close these issues.

2 participants