New extra trailing `|` in input LogEntry for handheld consoles.

[RetroEdit]

Preface/TLDR

This issue itself isn't significant, but I think it shows some other issues that could be addressed.

But basically, it at least gives me peace of mind that this schema change is known.

Details

I recently noticed for certain cores like mGBA and Gambatte, there's an extra trailing | in each input string. For example in Gambatte, before a blank entry was |.........|, and after, it was: |.........||

Probably only single-player handheld cores are affected; i.e., any core where this code is run:

https://github.com/TASEmulators/BizHawk/blob/4fc9e207892278b7713aae53f458f2d38243ab8c/src/BizHawk.Emulation.Common/Base%20Implementations/ControllerDefinition.cs#L162-L163

Cause

I bisected to: a0800862b3678cbfeaad1290418a17c2b3e51fea (2.10 dev)

CreateLogEntry is the proximate cause, because the second group in source.Definition.ControlsOrdered is empty. Specifically, prior to the above commit, Bk2LogEntryGenerator has an internal field _controlsOrdered that was initialized to _source.Definition.ControlsOrdered.Where(static c => c.Count is not 0).ToList(); and reused; afterwards, the filter c => c.Count is not 0 was no longer applied.

Arguably the root cause here is that GenOrderedControls inits with a list of length PlayerCount + 1.

Resolution

A few things could be improved here:

• [ ] Code: ControllerDefinition could probably have an boolean (or maybe enum?) internal field that identifies consoles like GB/GBA/TI-83, etc.
  • IsHandheld maybe?
  • [ ] It's also worth considering whether the LogEntry schema should be reverted/altered for these cases.
    • Note though the specific change in schema here isn't necessarily a huge issue; based on some recent TASVideos submissions I've seen, old movies still can be imported sensibly.
• [ ] Documentation: the remark before GenOrderedControls mentions a "player 0" without further context. Above, ControlsOrdered does mention the list has sublists of console buttons followed by player buttons. I'd like to see consistent verbiage around console buttons, and clear specification of how cases like GB are encoded.
• [ ] Testing: should catch schema changes like this. LogGeneratorTests does test generically test logs, but there doesn't appear to be core-specific tests.
  • [ ] At the very least, there should be a consistency check for each core (with default sync settings) comparing the blank frame to a hardcoded value. This is not to say the schema should never change, but it should be intentional, to consider the effects on importing movies from prior versions of BizHawk.
  • [ ] Maybe somehow test whether trailing or extraneous | or lack thereof are loaded in a backwards compatible manner?
  • [ ] Maybe somehow test per-button whether the button mnemonic/ordering corresponds to what it was previously?

See also

A semi-related commit and issue I found: b0913448966d65cc5c19a7b72168cea0d540fa5e (commit comment: "fixes a8368849a9bb03da0ed2183b4fb68caabb53af49 and 3384ce8629f1ef03b4b9296c311b33d0ea3d685c"), #4132

[Morilli]

To be honest this isn't really an issue, as | are ignored completely when parsing, they only serve semantic purpose. So test whether trailing or extraneous | or lack thereof are loaded in a backwards compatible manner isn't necessary either.

See https://github.com/TASEmulators/BizHawk/blob/bb7e5bc02c5a44e4e8d481cd165b5afa54f7c903/src/BizHawk.Client.Common/movie/bk2/Bk2Controller.cs#L63

I do wonder what the point of that "hack" in PlayerCount is because I don't see any reliance on that code or differentiation elsewhere. Just removing that part would probably "fix" this case. You would still get empty lists for empty non-0 player numbers, although I'm unsure if that can ever happen or if that would even make sense. After all, what is a "player" without controls?

Initializing the list in GenOrderedControls with PlayerCount + 1 makes sense because index 0 is used for "console buttons" (like reset and power) and everything beyond is for individual player controls. In the case of systems like the gameboy there is no differentiation because there are no different players or controls; the system is the controller.

[RetroEdit]

Tests don't matter until they do. If ignoring extraneous | is allowed behavior, then the tests should document that.

But otherwise I agree.

[YoshiRulz]

To reframe the problem: | is semantically a field separator (fencepost-style). There are 2 fields, player 0 and player 1, while we only have a player 0 at runtime. Therefore the extra | is erroneous.

(IMO our parser is too lax.)