Simplify SGB related UIs and make it possible to use SGB under Gambatte link

[Fortranm]

This is a revision of #3283 made to work on BizHawk 2.9.

https://en.wikipedia.org/wiki/Super_Game_Boy#Super_Game_Boy_2

The Super Game Boy 2 was released exclusively in Japan in 1998. Additions included a link port to allow a user to access two-player mode via the link cable...

Currently, the SGB mode of Gambatte has to be turned on via Config > Cores > GB in SGB instead of in the sync setting and cannot be used in GB Link as a result.

This PR does three things related to SGB:

1. SGB is added (or rather, merged in) as a Console Mode option for Gambatte.
   

2. "GB in SGB" check box and SGB core picker are removed and the BSNES cores are added under the GB core picket (legacy BSNES core not included). GB/GBC games will load under SGB mode of the corresponding BSNES core if one of those options are chosen.
   

3. As a result of the first change (with addition adjustments), it is now possible to use SGB mode in Gambatte Link too.
   
   
   
   
   Each SGB core can have up to 4 controllers linked to it. The Power button for each SGB core is included on the first Player tab of the core for the time being.
   Note that the tab parsing process is currently hardcoded to categorize every item that starts with "Px " under the tab "Player x" and everything else under "Console". There have been suggestions to use "Console x" for a core in link mode but that requires enough refactoring to be considered a separate task IMO.

Note: The SGB sound is not implemented yet but that can be done along with the MBC sound fix for Gambatte Link altogether later.

resolves #2289

[TiKevin83]

approving from a code perspective, haven't done a QA of the functionality yet

[Morilli]

Haven't tested the gambatte changes extensively but I think this is solid enough to get in. The "gb as sgb" UX flow was pretty bad imo so this is a definite improvement.

[YoshiRulz]

I'm curious of what @CasualPokePlayer has to say, specifically about the video buffer stuff. But this now LGTM in terms of code. (I think you deserve some kind of trophy for hacking together the button mapping by doing it in the wrong direction and with 3 nested loops.)
As for functionality, opening a 4x bundle and immediately closing it results in:

stacktrace

System.NullReferenceException: Object reference not set to an instance of an object
  at BizHawk.Emulation.Common.LinkedSaveRam.CloneSaveRam () [0x0002f] in <f3a4da15026e4f05ab7072e9a135677c>:0
  at BizHawk.Client.EmuHawk.MainForm.FlushSaveRAM (System.Boolean autosave) [0x000f0] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at (wrapper remoting-invoke-with-check) BizHawk.Client.EmuHawk.MainForm.FlushSaveRAM(bool)
  at BizHawk.Client.EmuHawk.MainForm.CloseGame (System.Boolean clearSram) [0x0006f] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at BizHawk.Client.EmuHawk.MainForm.CloseRom (System.Boolean clearSram) [0x00010] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at (wrapper remoting-invoke-with-check) BizHawk.Client.EmuHawk.MainForm.CloseRom(bool)
  at BizHawk.Client.EmuHawk.MainForm.CheckHotkey (System.String trigger) [0x01588] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at BizHawk.Client.EmuHawk.MainForm.<ProcessInput>b__143_0 (System.Boolean current, System.String trigger) [0x00000] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at System.Linq.Enumerable.Aggregate[TSource,TAccumulate] (System.Collections.Generic.IEnumerable`1[T] source, TAccumulate seed, System.Func`3[T1,T2,TResult] func) [0x0002e] in <89a8dc5bc33447a4a98f23a2d52e8aee>:0
  at BizHawk.Client.EmuHawk.MainForm.ProcessInput (BizHawk.Client.Common.InputCoalescer hotkeyCoalescer, BizHawk.Client.Common.ControllerInputCoalescer finalHostController, System.Func`2[T,TResult] searchHotkeyBindings, System.Func`2[T,TResult] activeControllerHasBinding) [0x00135] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at BizHawk.Client.EmuHawk.MainForm.ProgramRunLoop () [0x0009c] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at (wrapper remoting-invoke-with-check) BizHawk.Client.EmuHawk.MainForm.ProgramRunLoop()
  at BizHawk.Client.EmuHawk.Program.SubMain (System.String[] args) [0x0042b] in <5e7f3991f9b246bb80e8d9725eda76e1>:0

[YoshiRulz]

I still don't have a nice repro, but I found the culprit with print-debugging, and it wasn't related to this PR. Pushed 64db1fe.
In working on that, I've seen that SGB in GambatteLink is working, so I won't block this any further. @CasualPokePlayer?

[CasualPokePlayer]

Why is SGB being removed here? BSNES/BSNESv115 still have it.

[Morilli]

Because for core choosing, the system is only set to SGB when the core is bsnes:

BizHawk/src/BizHawk.Client.Common/RomLoader.cs

Lines 485 to 490 in 9ee3830

	static bool IsPreferredCoreSGB(Config config)
	=> config.PreferredCores[VSystemID.Raw.GB] is CoreNames.Bsnes or CoreNames.Bsnes115 or CoreNames.SubBsnes115;
	if (IsPreferredCoreSGB(_config))
	{
	game.System = VSystemID.Raw.SGB;
	}

Thinking about it, that's probably logic that could get removed too...

[CasualPokePlayer]

How does this exactly end up working for say, loading a GBC only ROM into BSNES. Does it proceed to fail completely due to not finding a fallback with the SGB system ID ctor, or does it end up still picking Gambatte with the GBC system ID?

[Morilli]

That's a good point. It does NOT work currently for that exact reason and instead runs into this code which then no longer makes sense:

BizHawk/src/BizHawk.Client.Common/RomLoader.cs

Line 859 in 9ee3830

DoLoadErrorCallback("Failed to load a GB rom in SGB mode. You might try disabling SGB Mode.", system);

[Morilli]

There's more to this, and there is already broken behavior currently. I'll make an issue later.

[Fortranm]

#3960
The new issue in question for reference. The way this currently is makes sense with that proposed change.
Resolving.

[YoshiRulz]

Not resolved.

Outdated

Outdated

[YoshiRulz]

Keep this attribute...

[YoshiRulz]

...and here...

[YoshiRulz]

...and here...

[YoshiRulz]

...and here.

[Morilli]

At the very least this one is questionable as the core doesn't ever actually sets its SystemID to SGB. I don't really mind this too much, but I do think we should be able to agree on what this attribute is supposed to mean.

[YoshiRulz]

Semantically, it indicates which constructor to use for a given RomGame.
Also, the set across all of the core's constructors doubles as the list of sysIDs which that core can emulate. We could split that out to the [Core] attr instead, but I think a single-source-of-truth is better.

BizHawk/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SNES/LibsnesCore.cs

Lines 112 to 115 in dbc6acc

	if (game.System == VSystemID.Raw.SGB)
	{
	IsSGB = true;
	SystemId = VSystemID.Raw.SNES;

^ This is clearly incorrect. I imagine it was a hack to get the frontend to show the right menus/tools.

[Morilli]

the set across all of the core's constructors doubles as the list of sysIDs which that core can emulate.

This is precisely the issue; I don't think this invariant necessarily needs to hold true (it doesn't even right now with that old bsnes core), and even with your suggested change bsnes* core cannot provide the GB/C SystemID, even though they have a corresponding CoreConstructor attribute.

That attribute is currently entirely internal and only used for the internal selection process of which cores to consider for a given rom, so putting semantic stuff here that is supposed to communicate meaning to the reader while doing nothing for the core or romloader that's using it is misguided imho.
It's fine to communicate this SGB stuff with a comment, but the attribute itself doesn't need to be here.

The conceptual question here is whether it is acceptable for a core to load a rom of type X and declare a SystemID of type Y != X using that loaded rom, and I say yes.

[YoshiRulz]

How about this:

• RomLoader never sets SGB on any rom (since any GB rom could be used with SGB, they should all be detected as the same, and GB/GBC is the better choice)
• There is no core preference for SGB, only GB and maybe GBC
• Cores are perfectly honest with setting IEmulator.SystemId (not necessarily matching the one passed in, since a sync setting can choose between GB/GBC/SGB)
• SystemId always matches a [CoreConstructor]—if it doesn't make sense to have that ctor, like with SGB which RomLoader will never use, it can simply be private and call something like this(default)

I think that would give the consistency I'm after while being simpler for regular users.

[Morilli]

The first three should already be present here.

If you feel like adding dummy constructors with the SGB attribute just to signal intent, fine.

[YoshiRulz]

The first 3 are not already implemented. This PR addresses some of them, but I'm saying we should clean it up on master first.

[Fortranm]

The first 2 definitely are included in this the PR.
The only one that doesn't currently fit 3 is the legacy BSNES core. Actually modifying that core to use the SGB sysID should be a separate task imo (and hopefully the core is retired before that has to be done, maybe)