dirkwhoffmann / virtualc64

VirtualC64 is a cycle-accurate C64 emulator for macOS
https://dirkwhoffmann.github.io/virtualc64
Other
356 stars 33 forks source link

Error with Commodore Key. #629

Closed rauno-palosaari closed 3 years ago

rauno-palosaari commented 3 years ago

Virtual C64 4.0b3

Error with Commodore Key. Release without press

Commodore + key1 -> Nothing

_pressRowCol(7,5)
releaseRowCol(7,0)
releaseRowCol(7,5)

Commodore + key2 -> _

_pressRowCol(7,5)
_pressRowCol(5,6)
releaseRowCol(5,6)
releaseRowCol(7,5)

Commondore key have other strage keys -> small cap, 4

_pressRowCol(7,5)
_pressRowCol(1,3)
_pressRowCol(1,7)
releaseRowCol(1,3)
releaseRowCol(1,7)
releaseRowCol(7,5)

Control ok

Control + key1 -> Black

_pressRowCol(7,2)
_pressRowCol(7,0)
releaseRowCol(7,0)
releaseRowCol(7,2)

Regard,

Rauno Palosaari

rauno-palosaari commented 3 years ago

Commondore ok with 3.4

dirkwhoffmann commented 3 years ago

Hi Rauno. You pressed RunStop on the MacBook Pro Touch Bar, right?

rauno-palosaari commented 3 years ago

Hi,

I used Mac Mini (x86_64), Big Sur, Mac Mac keyboard (Swedish), only physical keyboard.

No RunStop, no MacBook Pro Touch Bar. Only Option + 1( and Option + others).

I’ll try to tomorrow (tried yesterday) from rebuild: git reset git stash git stash drop

Would could if to see the log for Option + 1, or other C64Configure.h it: static const int KBD_DEBUG = 1; // Keyboard

option(C= + 1..8)

If it still I have error, I’ll see if when I’ll try to debug if I have a problem.

Regards, Rauno

On 26 Jan 2021, at 16:01, Dirk Hoffmann notifications@github.com wrote:

Hi Rauno. You pressed RunStop on the MacBook Pro Touch Bar, right?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

rauno-palosaari commented 3 years ago

Strange. Option + 1 works with the Magic Keyboard with Numeric Keypad.

Option + Numeric Keypad 4

_pressRowCol(7,5)
_pressRowCol(7,0)
releaseRowCol(7,0)
releaseRowCol(7,5)

Option + 4

_pressRowCol(7,5)
releaseRowCol(7,0)
releaseRowCol(7,5)
dirkwhoffmann commented 3 years ago

I can confirm that there is a bug. After adding some more debug messages, I can see the following after pressing OPT + 4 + 4 in positional key mapping mode:

_press(49)
_pressRowCol(7,5)
KeyboardController.82::keyDown(with:): keyDown: Optional("¢") 21
KeyboardController.88::keyUp(with:): keyUp: Optional("¢") 21
KeyboardController.82::keyDown(with:): keyDown: Optional("¢") 21
KeyboardController.88::keyUp(with:): keyUp: Optional("¢") 21
_release(49)
_releaseRowCol(7,5)

The Commodore key (7,5) is pressed and released on the C64 side which is good. But the '4' gets lost (it is recognized on the Swift side, but there are no corresponding key events on the C64 side). I think it's because I use the MacKey class as the lookup key for the positional key map in Swift. The modifier flags (which are also stored in the MacKey class) are likely to interfere:

 let joyKeyMap1: [MacKey: Int]

If I remember correctly, I had a similar issue in vAmiga. I'll look into it in more detail tomorrow...

dirkwhoffmann commented 3 years ago

I’ve identified two (maybe three) issues:

  1. As expected, the emulator failed to find the mapped C64 key in the positional key map if a modifier flag was present.

    I’ve fixed that by providing a custom hash function:

    func hash(into hasher: inout Hasher) {
    
        hasher.combine(keyCode)
    }

    An even easier method would be to use the key code as map index and not the object itself.

  2. The standard positional key map had duplicates (two distinct Mac keys assigned to the same C64 key)

    MacKey.curRight: C64Key.curLeftRight,
    MacKey.curLeft: C64Key.curLeftRight,
    MacKey.curDown: C64Key.curUpDown,
    MacKey.curUp: C64Key.curUpDown,

    As a result, pressing „cursor left“ on the Mac pressed the horizontal cursor key on the C64 keyboard. This moved the cursor into the wrong direction if the shift key was not pressed at the same time. I agree that this is totally confusing to the user. To cope with this issue, I’ve removed the double mappings from the default key map. Furthermore, when the key map is being loaded from the user defaults, the emulator checks for double mappings and filters them out. This has two implications though:

    • Pressing "cursor left" or "cursor up" on the Mac keyboard has no effect in positional key mapping mode any more. To move the cursor up, you need to press Shift+Cursor down, just like on the original C64 keyboard.
    • The key map in the user defaults storage might not work as expected any more (because all duplicates are filtered out). This can be solved by restoring the standard settings in the Keyboard preferences dialog (it’ll save the new standard positional key map to the user defaults storage which has no duplicates any more).
  3. One of the logs above shows a key release event, but no key press event for the option key. I was not able to reproduce this on my machine, but I remember that I had similar problems with older macOS releases (many years ago actually). To cope with this, I used the following code at that time:

   /* Checks if the internal values are consistent with the provides flags.
     * There should never be an insonsistency. But if there is, we release the
     * suspicous key. Otherwise, we risk to permanently block the C64's
     * keyboard matrix
     */
    func checkConsistency(withEvent event: NSEvent) {

        let flags = event.modifierFlags

        if (leftShift || rightShift) != flags.contains(NSEvent.ModifierFlags.shift) {
            keyUp(with: MacKey.shift)
            keyUp(with: MacKey.rightShift)
            Swift.print("*** SHIFT inconsistency detected *** \(leftShift) \(rightShift)")
        }
        if control != flags.contains(NSEvent.ModifierFlags.control) {
            keyUp(with: MacKey.control)
            Swift.print("*** CONTROL inconsistency *** \(control)")
        }
        if option != flags.contains(NSEvent.ModifierFlags.option) {
            keyUp(with: MacKey.option)
            Swift.print("*** OPTION inconsistency *** \(option)")
        }
    }

At some point, I stopped calling this function, because the issue went away from some OS upgrade to another (on my machine). If the problem still shows up in the next release, I might have to resurrect this function.

dirkwhoffmann commented 3 years ago

Hopefully fixed in v4.0b5.