Multi script drawing

[SuuperW]

This pull request fixes #2600.

The first commit (add a reference to BizHawk.Client.EmuHawk) requires removing use of init setters in BizHawk.Client.EmuHawk. I've never encountered this sort of issue before, so I might be missing a better solution.

[YoshiRulz]

[...] PolySharp [...]

We do not use PolySharp, it seems to be a transitive dependency accidentally added to SharpGen (which is a dep of Vortice). Opened SharpGenTools/SharpGenTools#236. We may be able to downgrade Vortice as a workaround.
edit: We can't upgrade Vortice because we're already using the final 2.x version, and 3.x does not target .NET Standard 2.0. Opened amerkoleci/Vortice.Windows#415.

Add a reference to BizHawk.Client.EmuHawk [...]

You can't do this. The test project is .NET Core and EmuHawk is .NET Framework.

I'm also not seeing what part of your MockDisplayManager references something from EmuHawk. I suggest you move my SimpleGDIPDisplayManager (and FakeGraphicsControl) to BizHawk.Client.Common and use those.

[Testing Lua drawing edge cases with a unit test that doesn't involve Lua]

If you can't see what's wrong with this, nothing I say can help you.

[SuuperW]

Thanks for your feedback.
I had tried to figure out how PolySharp was getting into the project, but couldn't find it.

Add a reference to BizHawk.Client.EmuHawk [...]

You can't do this. The test project is .NET Core and EmuHawk is .NET Framework.
I'm also not seeing what part of your MockDisplayManager references something from EmuHawk. I suggest you move my SimpleGDIPDisplayManager (and FakeGraphicsControl) to BizHawk.Client.Common and use those.

It did seem less than ideal to add that reference. (I can do it, it will build and run, but calling certain pieces of code does of course crash.)
Anyway, I originally added it because I was doing something different. I should have revisited that before pushing, though. The only remaining use of it was for a simple extension method on the IGL interface. I can remove that project reference and add BizHawk.Bizware.Graphics with a small code change.

I'll look into moving the classes as you suggest. Thanks for pointing it out, as I hadn't seen it since the project isn't part of the solution. It seems strange to move code written for tests outside of test projects, though. Might it be better to move them to BizHawk.Tests? BizHawk.Tests.Testroms.GB could add a reference to BizHawk.Tests.

[Testing Lua drawing edge cases with a unit test that doesn't involve Lua]

If you can't see what's wrong with this, nothing I say can help you.

I recognize this isn't really testing Lua even though the motivation was to fix an issue with drawing through Lua scripts. But I don't believe I ever claimed I was testing Lua, I just said the fix fixes a Lua-related issue. It tests code used by Lua but is not an exhaustive set of tests by any means. Of course it's better to have tests of Lua than to not, but do you think there needs to be tests that involve Lua in order for this to be worthwhile? (EDIT: Yeah, okay, my tests aren't good.)

As for making tests that actually use Lua scripts, I do want to do this but am currently unsure how. (Still looking, though.) If you know a good way to do this I would appreciate your input. One of the problems is that, as the code is written right now, we cannot instantiate BizHawk.Client.EmuHawk.LuaLibraries since it requires a reference to a Windows Form. An actual Windows Form, not just stuff on MainForm, for the FormsLuaLibrary (and, of course, a project reference to BizHawk.Client.EmuHawk). I may end up creating a new test project that targets windows.

I noticed there's a folder output/Lua/UnitTests. But these don't appear to have any automation. (and don't all even work)

[SuuperW]

I've force-pushed with new commits, this time Lua scripts are run in tests.

The first several commits are all refactoring so that the code needed for the test can be moved to BizHawk.Client.Common. There should not be any behavior changes except for commit 0c8ce93.

Regarding the SimpleGDIPDisplayManager in BizHawk.Tests.Testroms.GB, that implementation is outdated and does not compile. So I decided not to use it.

[SuuperW]

I realized that this broke drawing via external tools (which already wasn't working as semi-documented anyway, but another undocumented way). My new commits address this issue by allowing external tools to use the new BeginFrame and EndFrame methods without having to worry if Lua is also running. External tools + LuaConsole tool can now draw on the same frame.

It looks like none of the external tools in /ExternalToolProjects used the GuiApi. However one of the ones linked in https://github.com/TASEmulators/BizHawk-ExternalTools/wiki/Catalog does, but it uses the obsolete (and now deleted) DrawNew/DrawFinish methods so hasn't worked for a couple years now. It might be a good idea to update documentation to include a working example of drawing (which may be as simple as forking Ecco_BizHawkTool and replacing the obsolete methods with the new ones and linking to that instead).

[YoshiRulz]

Here's one of my in-development projects which uses IGuiApi: RetrOCR_wip202309.zip

[CasualPokePlayer]

https://github.com/CasualPokePlayer/PokemonGBTASTool
Another project which uses the IGuiApi

[SuuperW]

Thanks for sharing those. So, I guess we want to keep the old WithSurface behavior to preserve compatibility with these tools and any others people may have made?

I just looked at the Lua library and it does not expose the WithSurface method, meaning it's only available to external tools right now. So we should be able to preserve compatibility by putting calls to BeginFrame and EndFrame inside WithSurface, without breaking anything. We could even give it an optional parameter to control that functionality so external tools could still control the calls to BeginFrame and EndFrame (when LuaConsole isn't open at least).

@CasualPokePlayer Correct me if I missed something, but the only use of IGuiApi I see in that Pokemon tool is using Gui.Text. That is not relevant here since it goes to the OSD instead of rendering like other methods do.

[YoshiRulz]

IMO, if you're going to change it, it should be to add a with_surface to Lua (or something equivalent, point being to replace the gui library, drawing only being available via the callback's parameter†). I would have done that but adelikat complained that old scripts which were 'just drawing' should be supported.

† Which I haven't implemented for ApiHawk yet, but plan to.

[SuuperW]

Okay, new force push reverts the changes to IGuiApi. There are now no changes to how external tools draw.

To summarize, the behavior changes in this PR are:

1. Drawing with multiple Lua scripts on the same frame now works. This is done by replacing the LuaAutolockHack and LuaAutoUnlockHack of the GuiApi implementation with BeginFrame and EndFrame methods (which are not on the IGuiApi interface), since the auto hack was fundamentally incompatible with having multiple scripts drawing on the same frame.
2. Drawing with external tools and Lua at the same time now works. This is done by making the new GuiApi methods that LuaLibraries calls recognize when another GuiApi instance has locked/unlocked surfaces.

There is also a fair bit of moving code around and refactoring in order to support the new tests, but this should not have any impact on behavior.

Note that drawing from multiple external tools still does not work. All external tools share API instances and the GuiApi class doesn't know who's calling it, so we cannot support multiple external tools without either changing the API usage or giving each external tool its own GuiApi instance.

I tested a Lua script making thousands of calls to drawPixel per frame and got the same FPS on both master and this branch so performance seems unaffected.

[Morilli]

Seeing as #2600 has been resolved, can this be closed?