Avoid multiple detect from single12way rotate

[zorro2055]

Hi @mahoneyt944 , I'm copying and pasting the comments I just put in the mame2003-plus pull request that are used for this for the most part.

its been a while as it definitely took longer than expected and other things came up, but I believe it is done (it appears to work as intended with my 12-way rotary joystick) and ready to merge! It definitely uses more code than I expected.

See the corresponding RetroArch pull request that is needed for this to work that I request shortly.

First, some key points about implementation on the mame2003-plus and RetroArch side) after how this ended up (hopefully, it all looks good to you):

1. When mame2003-plus uses the input callback, this is now "intercepted" to filter select saved poll data. For this pull, it filters the analog increment and deincrement to the effect described earlier in this discussion. I was looking at actually intercepting the poll data as it was taken during the polling, but the poll data appears to be stored differently with each input device driver. And there was no modify poll data API that I could see. Intercepting the input callback functions essentially the same though.
2. I decided to add the 2 new analog controls options (X-Way Joystick to enable or disable this filter, Lockout (ms) setting the time the button will ignore a new button press). to all games. First, this allows any game to use this option even though it may be unlikely they need it. X-Way Joystick defaults to disabled, so it will not effect any games unless specifically enabled. Lockout (ms) defaults to 21 which is just longer than 1/50 of a second. When X-Way Joystick is disabled, Lockout does not do anything. Second, this meant I only had to modify two definitions for ANALOG and ANALOGX instead of 15ish local definitions for games that use this x-way joystick.
3. I successfully tested this with the updated RetroArch on Linux, Windows 11, and PS Vita (it runs, no way to use rotary joysticks on that of course). Just in case this comes up against something it will not run on, I did put in a toggle option that will allow this to not be compiled in. Basically, just uncomment the NO_FILTERED_POLL=1 in the Makefile
   https://github.com/zorro2055/mame2003-plus-libretro/blob/8c57fd7d81f48d7e6a1f9a53ad1aa6c2b01ce529/Makefile#L5-L7

Note I just had to sync in 2 merges from other on RetroArch just now, so technically, that is not tested.

I successfully tested this with this option enabled on the above mentioned platforms as well.
4. This adds the new files filtered_poll.c and filtered_poll.h. Only filtered_poll.h is used in mame2003-plus, but I have filtered_poll.c in mame2003-plus as it seems to be done with other libretro files as well. Both of these files are used in RetroArch. I threw in a copyright header from another file for both of these, but let me know if that should change in any way.
5. The intercept requested saved poll data is implemented in filtered_poll.c and filtered_poll.h. Filtered_poll.c is compiled in RetroArch where this functionality resides. This also keeps to the original intent of not modifying any existing RetroArch source code files. I have one timeout filter currently which is used for my use case. I have basic comments showing where 3 more other filters could be defined in the future if there is ever a use for that.
https://github.com/zorro2055/RetroArch/blob/8d25800544f356ea261546320739803d48aa84fa/libretro-common/filtered_poll/filtered_poll.c#L151C1-L164C15
https://github.com/zorro2055/RetroArch/blob/8d25800544f356ea261546320739803d48aa84fa/libretro-common/include/filtered_poll.h#L39-L43
6. Any other core could use the RETRO_API functions defined in filtered_poll.h for their own purposes.

That is all I can think of at the moment. Let me know if you have any questions of course. Hopefully, it is ready to merge off the bat.

Thanks!

Link to mame2003-plus pull request that uses these changes:
libretro/mame2003-plus-libretro#1652

Guidelines

1. Rebase before opening a pull request
2. If you are sending several unrelated fixes or features, use a branch and a separate pull request for each
3. If possible try squashing everything in a single commit. This is particularly beneficial in the case of feature merges since it allows easy bisecting when a problem arises

Description

[Description of the pull request, detail any issues you are fixing or any features you are implementing]

Related Issues

[Any issues this pull request may be addressing]

Related Pull Requests

[Any other PRs from related repositories that might be needed for this pull request to work]

Reviewers

[If possible @mention all the people that should review your pull request]

[zorro2055]

I see one check so far was not successful (some tests still running). The changes I made to the main Makefile probably would need to be done with the Makefiles for these builds as well. Worse case, this feature would be compiled out with NO_FILTERED_POLL set to equal 1.

[mahoneyt944]

Most of these issues are declaration related. move declarations to the beginning of a code block instead of in the for loop for example.

[zorro2055]

Ah, look at the details tab. Okay, these issues seem easy enough to fix. Can you give me feedback on anything else (hopefully nothing) so I can fix everything together?

[LibretroAdmin]

There seems to be a C89 build failure that has to be fixed.

[mahoneyt944]

Ah, look at the details tab. Okay, these issues seem easy enough to fix. Can you give me feedback on anything else (hopefully nothing) so I can fix everything together?

Just move the declarations to the beginning of the scope of each function or code block, that should cover most of what I see. C89 is strict on this.

[zorro2055]

Almost got it. I just need to fix some text in a comment to now pass the PS4 build. I should get that fixed within the hour I assume or less.

[mahoneyt944]

@zorro2055 @LibretroAdmin
Looks good now. 👍

[zorro2055]

Thanks all. I moved the declarations to the top and fixed the comment errors. As you can see, everything compiles now. I'll do the same shortly for mame2003-plus.

[LibretroAdmin]

OK so looking at this I have some concerns:

1. The way this is implemented, is this going to be enforced for EVERY core ? If so, I really don't think that is a good idea. A feature like this should be opt-in on a per-core basis since it will likely induce quite a bit of overhead on the input side.
2. Why is this really needed? Is this needed for mame 2003 plus? If so, see point 1, why does it have to be enforced for every core in RetroArch like this? I think if you need it for MAME 2003 Plus and that alone then you should only enable it for that core and not have it affect everything else.

[zorro2055]

Hi @LibretroAdmin , thanks for looking this over. Below are the answers to your questions.

1. This is NOT enforced for EVERY core. A core can opt in if it wants to use it. The way this implemented is these new defined functions in filtered_poll.c are published from the main executable retroarch. The shared library core can then call that function. This is the opposite of how main executables and shared libraries typically work, but Linux ELF files do support it. For windows, the shared library dynamically finds the filtered_poll.c functions as shown here in the also proposed change for mame2003-plus:
   https://github.com/zorro2055/mame2003-plus-libretro/blob/2a5aa80d792b1849713817d735a61516e6a568cb/src/mame2003/mame2003.c#L825-L839

I just double checked this by running retroarch with this patch with mame2003-plus without the proposed patch essentially (uncommented NO_FILTERED_POLL=1 in mame2003-plus Makefile and recompiled) and it runs fine. In this case, a core that does not call these filtered_poll.c functions does not lose performance in anyway since these functions are never run.

Good question on the overhead. I can say the game Ikari Warriors that I tested on the PS Vita at 60 fps fine. The performance seems the same as without the patch. The patch is written to minimize intercepting. If there is an easy way to benchmark performance, let me know. If a core is not using this feature, there is no performance impact. No code was introduced into the existing retroarch code files. Thus not complicating backporting the latest MAME features back to retroarch.

2. This started as an issue specific to a 12 way rotary joystick for games that use it (most famously Ikari Warriors) to keep one single step twist from sometimes turning the game character 2 steps. After discussion with @mahoneyt944 , it seems to make sense to generalize this so other cores could use this functionality, locking out a button for a certain time after it is pressed, if desired.

More detail: the rotary joystick twists and "snaps" every 30 degrees. This rotates a character on the screen by 30 or 45 degrees depending on the game. The 12 way rotary joystick that I have:
https://www.ultimarc.com/arcade-controls/joystick-accessories/ikari-12-way-rotary-upgrade-for-servostik-j-stik/
This joystick simulates a unique button press when the joystick is rotated left or right (in mame2003-plus, these two buttons are set by the "Dial" input). The joystick firmware time that the simulated button can be held down can be set to 4 different settings. I have it set to be just longer than 1/60 of a second. This guarantees that when the joystick is twisted 30 degrees, it will detected as retroarch / mame2003-plus poll for input every 1/60 of a second. The issue is while the twist is guaranteed to be detected, sometimes, it might be detected twice in a row since the "button press" is just over 1/60 of a second. So this patch gives a general saved poll request data intercept input callback function to lockout another button press for some variable amount of time to block this second "false" twist step that can happen sometimes. I do plan to hopefully use this in mame2003 as well. The way I implemented these functions in filtered_poll.c, up to 3 other saved poll data requests filters could be defined if they were ever needed just in case.

For reference on games with this discrete rotary joystick, here is a picture of Ikari Warriors:
https://www.vintagearcade.net/wp-content/uploads/2015/02/Ikari-Warriors-4.jpg

And video of Ikari Warriors gameplay:
https://www.youtube.com/watch?v=8ECiZUWoz8c

These are all the games (on MAME) that I know of offhand that use this type of 8 or 12 way rotary joystick:
Bermuda Triangle
Caliber 50
Downtown
Guerrilla War
Heavy Barrel
Ikari Warriors
Victory Road
Ikari III - The Rescue
Midnight Resistance
SAR - Search and Rescue
Time Soldiers
Touchdown Fever
Touchdown Fever 2

Hopefully that answers your questions. Let me know what you think!

[zorro2055]

There is also the option to compile this filtered poll functionality out of RetroArch by uncommenting the "NO_FILTERED_POLL=1" line in the RetroArch Makefile for whatever reason might come up.

[zorro2055]

And actually, here is a basic over view of how it works both without and with patch (retroarch and mame2003-plus). This should help with what it affects.

Currently without patch (or NO_FILTERED_POLL=1 uncommented in proposed mame2003-plus in Makefile):

During initialization:

• Register default input callback function (callback is used to check state of saved poll data for a given specific input). Default input callback function itself is defined in retroarch.
  This is done by calling from retroarch:
  

  RetroArch/runloop.c

  Line 4584 in fcd546b

  runloop_st->current_core.retro_set_input_poll(core_input_state_poll_maybe);

  
  and defined in the core (mame2003-plus from proposed patch shown below):
  https://github.com/zorro2055/mame2003-plus-libretro/blob/2a5aa80d792b1849713817d735a61516e6a568cb/src/mame2003/mame2003.c#L818-L820

During game loop:

• Call default input callback to get input state of one button, etc (shown in mame2003-plus. Note that there are 4 arguments, but the input callback definition has no arguments, but with some crazy syntax and typedef, the function under it uses these arguments. This is how mame2003-plus is and other cores are I assume.):
  https://github.com/zorro2055/mame2003-plus-libretro/blob/2a5aa80d792b1849713817d735a61516e6a568cb/src/mame2003/mame2003.c#L1399
• Input callback returns saved poll state of button or whatever was queried.

With proposed retroarch (and mame2003-plus ) PR. Other cores could use this optionally.

During initialization:

• Register custom input callback function (callback is used to check state of saved poll data for a given specific input). Custom input callback function itself is defined in new file filtered_poll.c in retroarch.
  This is done by calling from retroarch:
  

  RetroArch/runloop.c

  Line 4584 in fcd546b

  runloop_st->current_core.retro_set_input_poll(core_input_state_poll_maybe);

  
  and defined in the core (mame2003-plus from proposed patch shown below):
  https://github.com/zorro2055/mame2003-plus-libretro/blob/2a5aa80d792b1849713817d735a61516e6a568cb/src/mame2003/mame2003.c#L849

During game loop:

• Check if any active filters. If yes, enable filter checking, otherwise do not.
• Call custom input callback to get input state of one button, etc (shown in mame2003-plus. Note that there are 4 arguments, but the input callback definition has no arguments, but with some crazy syntax and typdef, the function under it uses these arguments. This is how mame2003-plus is and other cores are I assume.):
  https://github.com/zorro2055/mame2003-plus-libretro/blob/2a5aa80d792b1849713817d735a61516e6a568cb/src/mame2003/mame2003.c#L1399
• Custom input callback calls default input callback to get actual current state of button, etc:
  https://github.com/zorro2055/RetroArch/blob/e20b0f42e5c7fd2ba6b2d2323388af39daa73e9a/libretro-common/filtered_poll/filtered_poll.c#L56
• Check if filters are enabled
• If yes, check if queried button, etc should be filtered and if yes, change value of necessary. If no, do not check this.
• Custom input callback returns saved (or modified) poll state of button or whatever was queried.

So, any core out there right now is set to the default (or maybe some other input callback). This retroarch PR does not be default, change the behavior of any core. A core would specifically have to be modified (and code other logic) to set the custom input callback function in filtered_poll.c in order to have its behavior modified.

Hopefully, that makes sense.

[zorro2055]

Hi, ok, I did some benchmarking by setting a timer at the being and end of the update analog ports section in mame2003-plus running Ikari Warriors which uses this and the mame2003-plus PR to filter the analog port representing the twist for the 12-way rotary joystick. I ran this on a AMD Ryzen 9 7940HS. That appears to have a clock speed of 4 - 5.2 GHz.

Summary of results using average of last 10 time measurements before unload game message:

Scenario 1 (retroarch default input callback function):
Mame2003-plus with uncommented "NO_FILTERED_POLL=1", so basically the current standard:
10.9 microseconds

Scenario 2 (If a core were modfied to use custom input callback all the time, but no filtering happening):
Mame2003-plus with "NO_FILTERED_POLL = 1" commented out, but filtered analog port feature turned off in settings. So the custom input is being used, but it has been set to not check button to see if they need filtering:
10.5 microseconds

Scenario 3 (If a core were modified to use custom input callback and filtering was happening):
Mame2003-plus with "NO_FILTERED_POLL = 1" commented out and filtered analog port feature turned on in settings:
13.7 microseconds

So at least on the computer I have (which has more performance than a Raspberry Pi of course), Scenario 1 and 2 are basically the same. Scenario 3 is 3.2 microseconds longer which is something, but seems pretty small to me unless I am missing something? Maybe that translates to 15 microseconds on a Raspberry Pi?

For reference

Added code in src/inptport.c in mame2003_plus for time benchmark:

Time measurements for scenario 1:

Time measurements for scenario 2:

Time measurements for scenario 3:

[zorro2055]

Hi @LibretroAdmin

Just checking in on this. I obviously posted a bunch of info before, but here are more direct answers to your questions. Let me know from these answers if it makes sense to merge this. Or if I should just go back and only implement this in Mame2003-Plus. Thanks!

The way this is implemented, is this going to be enforced for EVERY core ? If so, I really don't think that is a good idea. A feature like this should be opt-in on a per-core basis since it will likely induce quite a bit of overhead on the input side.

No, no core would be affected by this unless it explicitly changed the retro_set_input_state function defined in the core (and declared and called in RA). This function sets the input callback function. The custom input callback function defined in the new proposed file filtered_poll.c would have to be explicitly referenced in the retro_set_input_state function definition in the core.

That retro_set_input_state function (declared in RA) shown defined in the unpatched Mame2003-Plus for reference:
https://github.com/libretro/mame2003-plus-libretro/blob/ab725a7f30a133551742b400089e8fffdf29d84a/src/mame2003/mame2003.c#L605C1-L605C70

Here is that same area in my patched Mame2003-Plus PR:
https://github.com/zorro2055/mame2003-plus-libretro/blob/2a5aa80d792b1849713817d735a61516e6a568cb/src/mame2003/mame2003.c#L818-L851

Yeah, it is slower by about 3.2 microseconds on an AMD Ryzen 9 7940HS with a clockspeed of 4 - 5.2 GHz.

Why is this really needed? Is this needed for mame 2003 plus? If so, see point 1, why does it have to be enforced for every core in RetroArch like this? I think if you need it for MAME 2003 Plus and that alone then you should only enable it for that core and not have it affect everything else.

The advantage is it provides a generic system for other cores to use this filtering as well which they can opt into if desired as explained above. It seemed handy to have that, but I have no idea how often other cores would have use for it. Ie, would they have a similar problem that I am trying to solve for Mame2003-Plus?