search

libretro / 81-libretro

A port of the EightyOne ZX81 Emulator to libretro
GNU General Public License v3.0
20 stars 30 forks source link

Update keybovl.c #25

Closed roo1234 closed 3 years ago

roo1234 commented 3 years ago

Slows down the keyboard overlay polling. Update once each 8 loops. Tries to address this issue: https://github.com/libretro/81-libretro/issues/15

leiradel commented 3 years ago

Looks very arbitrary to me, shouldn't have been merged without investigation.

inactive123 commented 3 years ago

Are you going to lead the charge with those investigations then I take it?

I will revert it in the meantime assuming good faith that both you and @roo1234 will try getting to the bottom of fixing the issue at hand.

leiradel commented 3 years ago

Unfortunately I'm not in a position to contribute code right now, but I felt the fix was quite arbitrary. Only update the keyboard 1 out of 8 times? Sounds like the DOS era all over again, when you had to install TSR helpers to slow down the machine and make games playable. It also uses an uninitialized variable.

inactive123 commented 3 years ago

At the end of the day we still need a solution to this -

https://github.com/libretro/81-libretro/issues/15

So regardless of your code preferences, something has to be worked out regardless. Do you have a better solution or a better idea? Something has to give here.

leiradel commented 3 years ago

It's not code preferences, the logic is arbitrary. It feels like a hack that may cause problems in other platforms. The virtual keyboard navigation should wait for the controller button to be released and pressed again before advancing to the next key, if this is not happening it's this part of the code that should be fixed.

inactive123 commented 3 years ago

@roo1234 Can you do anything with that feedback?

roo1234 commented 3 years ago

Hi, This is indeed a hack in order to fix the issue, I cannot see another more elegant solution given the way the polling is implemented. I was hoping someone would suggest improvements or at least test it. I tested it on win64. The var initialization cannot be done as suggested because it's a static, the routine relies on the last stored value from the last function call.

inactive123 commented 3 years ago

@roo1234 Are there any negative consequences with this hack or did you encounter none?

roo1234 commented 3 years ago

From my tests no ill effects. I have tried other approaches but this is the best I could come up with to solve the issue at hand. But then again I have only built and tested x86_64. @twinaphex, are you the main developer of this port?

leiradel commented 3 years ago

It doesn't address the real issue, and wasn't tested in the device where the problem happens. I can't write any code for this, but adding a hack to the codebase is not a good step forward.

inactive123 commented 3 years ago

Honestly @leiradel we have to be able to move forward here, so if you don't have a better way of informing him how to implement the desired output, then I see no real reason to block his efforts here and am more inclined to just merge his current efforts. It might be a hack but the current issue this tries to address is obviously not desirable either, so something has to give here.

leiradel commented 3 years ago

The best move forward here is to do nothing. The hack wasn't even tested on the device where the issue seems to happen.

roo1234 commented 3 years ago

The issue also happens in windows, when the overlay keyboard is enabled. Thing is you probably don't need an overlay keyboard on a keyboarded computer.

roo1234 commented 3 years ago

Testing the released version again on windows (10, 64bit), when using the real keyboard the overlay does not repeat (must press the key again). When using the joystick, the issue reappears (fast polling, repeating - same as in RPI3). Given that, I think it's best to ignore this commit to have a nicer solution in the future.

inactive123 commented 3 years ago

Alright, just don't let this prevent you from submitting PRs that you think are good to have in the future. We won't shoot down any good idea unnecessarily.