Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added measures to eliminate when occasionally, two character rotations steps occur when rotary joystick is twisted one position. #1652

Merged

Conversation

zorro2055
Copy link
Contributor

@zorro2055 zorro2055 commented Nov 5, 2023

Hi, this pull request will likely require discussion and maybe a change before being merged.

Explanation of issue:
Libretro appears to use a poll based system to get input from joysticks, keyboards, etc. This means once every frame, an input is polled at that moment to see what it state is. So, for an X-way rotary joystick single twist to guarantee that it is detected, it has to simulate a button press that lasts at least one entire frame. However, when the button "press" is held that long, one since joystick twist can sometimes be detected twice, once at the beginning of a frame and once at the end of a frame.

Here is a text graphic to kind of show this ( continuous dashed line is when one twist "presses" a button for a certain time, and P is when libretro is polling for input ):

                 ------------------
                  P               P               P

Solution to this issue in this pull request:
If a twist is detected, rotate character one position as usual. Then, however, block any more character twists for 24 milliseconds after the initial twist is detected. 24 milliseconds is 1.44 frames @ 60fps and 1.2 frames @ 50fps. So the next twist that could be detected would happen two frames after the previous detection.

Cons with this method:

  • Would only continuously detect twist every 2 frames instead of every frame. So if game runs at 60 fps, new twists would be detected at 30 fps. It is doubtful a rotary joystick would be twisted faster than 30 times a second though.
  • Some of the games such as Blasteroids (like Asteroids) use IPT_DIAL (also used with 12 way rotary joysticks) as their primary way to spin the spaceship mapping to left and right buttons. With this pull request, the ship does rotate at half the speed as without this patch.

Alternative / possible code additions to minimize / eliminate some of the cons:
Option 1 - In games such as Blasteroids, the presets for the sensitivity (under Analog Controls under the Config Menu) could be doubled to allow the ship to spin at its original speed.
Option 2 - Change libretro to use events to gather input rather than polling. Then theoretically, multiple button presses could event be detected between frames. udev on Linux supports events, I'm not sure if all other systems supported by libretro do. However, this option must likely would need a huge overall of libretro, so it seems not worth pursuing.
Option 3 - In the Analog Controls under the Config Menu, two options could be added. The first option could be called possibly "Use rotary joystick". Setting this option to "yes" would turn on this pull request. The second option could be "Lockout time (ms)" which could be set to the number of milliseconds before detecting twists during input polling.
Option 4 - Something else?

Let me know what y'all think. If option 3 is favored, it would take time for me to search through the code to determine how to do that.

Once this PR, or some version of it, is hopefully approved, I can submit it to mame2003 as well where I originally developed it.

Thanks!

Thank you wanting to make a contribution to this project!

Please note that by contributing code or other intellectual to this project you are allowing the project to make unlimited use of your contribution. As with the rest of the project, new contributions will be made available freely under the classic MAME Non-Commercial License.

This license can be viewed at https://raw.githubusercontent.com/libretro/mame2003-plus-libretro/master/LICENSE.md.

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Nov 5, 2023

I think if you're looking to alter how inputs are polled you'd best do this at the input driver / frontend level instead of here. The input drivers already behave differently between platforms as it is. I was under the impression that you could only read a polled input once before having to poll again for most input drivers though I haven't explicitly tried this. Would this be the case where a game driver is reading the input last polled twice in a frame?

@grant2258 has worked on the inputs here in the past, locally and the input drivers. Maybe he would offer up his opinion on this request.

@grant2258
Copy link
Contributor

grant2258 commented Nov 6, 2023

There are some overall issues here, a simplistic explanation is there are two main types of rotary encoder: absolute and incremental. I will assume from your explanation your are going for the incremental this would have side effects on an absolute encoder output.

The output of an absolute encoder indicates the current shaft position, making it an angle transducer. . The output of an incremental encoder provides information about the motion of the shaft, which typically is processed elsewhere into information such as position, speed and distance.

Now there is another issue your going by the emulation input poll rate when the game reads the polls itself it could be it doesnt read every frame on a particular game or it`s read as absolute rather than incremental.

The encoder will read the amount of travel in incremental for mame dials are treated as analog so the analog menu is available.

If you are using the dial as a button (digital) you would need to use the joy/key speed and use sensitivity for the real analog reads. I would need to check the code path for the dials to see its they are read abs or rel. Then enters another issue if your dial is reading rel or abs in RA itself. Its something that can be fixed in the analog menu though. If the defaults arent good for dial feel free to change them.

The logical part of me says this isint a the right solution its more likely just a fix for a specific game perhaps? At saying that I dont really use the core so don't feel strongly about it to say yes or no though. Its worth remembering even buttons have bounce time after letting go of them. If its moving too fast it more than likely the sensitivity and (if using digital key press is too high or sensitivity) as mame tries to make a digital button press count up rather than just on using there two settings.

@zorro2055
Copy link
Contributor Author

zorro2055 commented Nov 6, 2023

Hi, thanks for your responses. Ok, I'll try to respond to some of the comments / questions now. And I'll circle back on others later depending on your feedback from these answers, etc....

There are some overall issues here, a simplistic explanation is there are two main types of rotary encoder: absolute and incremental. I will assume from your explanation your are going for the incremental this would have side effects on an absolute encoder output.

Yes, I am going for incremental, types IPT_DIAL and IPT_DIAL_V in the code. A keyboard or joystick button is assigned to rotate clockwise and another button is assigned to rotate counterclockwise. These are named "Dial" in the input selection menu. I would guess that the types IPT_PADDLE and IPT_PADDLE_V are absolute, but did not check that. My pull request only applies to IPT_DIAL and IPT_DIAL_V.

Now there is another issue your going by the emulation input poll rate when the game reads the polls itself it could be it doesnt read every frame on a particular game or it`s read as absolute rather than incremental.

Ah, any easy way to tell if this is happening? The games I have tested so far do not seem to do that, but I have not looked for that specifically....

If you are using the dial as a button (digital) you would need to use the joy/key speed and use sensitivity for the real analog reads. I would need to check the code path for the dials to see its they are read abs or rel. Then enters another issue if your dial is reading rel or abs in RA itself. Its something that can be fixed in the analog menu though. If the defaults arent good for dial feel free to change them.

Yeah, at least for the couple of these types of games that I tried, I needed to set joy/key speed at 16 and sensitivity to 100%. That basically turns the character exactly one step.

The logical part of me says this isint a the right solution its more likely just a fix for a specific game perhaps? At saying that I dont really use the core so don't feel strongly about it to say yes or no though. Its worth remembering even buttons have bounce time after letting go of them. If its moving too fast it more than likely the sensitivity and (if using digital key press is too high or sensitivity) as mame tries to make a digital button press count up rather than just on using there two settings.

There are not a lot of games that have this particular stepped rotary action (usually, the steps are 30 or 45 degrees), so a fix for just those games might be possible. The games I am aware of that use this are (there could be more, but I would not think a lot more if there are):
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

Per my option 3, if code is not added only for specific games, having additional options in the Analog Controls menu to disable this feature for other games that use IPT_DIAL and IPT_DIAL_V games that have much smaller steps than 30 or 45 degrees like the above games do could be a way to avoid unintended effects with those other games.

Just for reference to see the style of game these larger step rotary joysticks were used for, here is a picture of the Ikari Warriors actual arcade cabinet. You can see the joysticks which actually twist in steps of 30 or 45 degrees (I can't remember which) to turn the character 45 degrees:
https://www.vintagearcade.net/wp-content/uploads/2015/02/Ikari-Warriors-4.jpg

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

I have this specific arcade controller that is a 12 way rotary joystick:
https://www.ultimarc.com/arcade-controls/joystick-accessories/ikari-12-way-rotary-upgrade-for-servostik-j-stik/

Most of these games have similar type gameplay / concept of use as Ikari Warriors.

With this particular joystick, there is button "debounce" to control how long one rotary step (twist) emulates a keyboard or joystick button being pressed down. With a Windows program that changes the setting the the joystick's firmware on its PCB, I can change the length of time that the "button" is "pressed". Since I am running Retropie in linux which has udev which has events, not polling, I was able to determine the time the "button" was held down for each. That info is:

Button debounce setting              Time button "held" down (milliseconds)
None                                 4
Short                                8, hit 12 once after trying about 15 times
Standard                             20, hit 16 once after trying about 15 times
Long                                 40, hit 36 and 44 about every 1 in 10 times

I have mine set at Standard which holds the "button" down for typically a little more than one full frame (at 60 fps).

I think if you're looking to alter how inputs are polled you'd best do this at the input driver / frontend level instead of here. The input drivers already behave differently between platforms as it is. I was under the impression that you could only read a polled input once before having to poll again for most input drivers though I haven't explicitly tried this. Would this be the case where a game driver is reading the input last polled twice in a frame?

If it makes sense to put this more in the input drivers, I can look at that. It might be something like a new function is added that calls the polling function in the driver, but adds the ability to monitor the last time the joystick was twisted to filter out the character rotating two steps from one step of the rotary joystick.

Hopefully, that was not too much info!

@mahoneyt944
Copy link
Collaborator

If it makes sense to put this more in the input drivers, I can look at that. It might be something like a new function is added that calls the polling function in the driver, but adds the ability to monitor the last time the joystick was twisted to filter out the character rotating two steps from one step of the rotary joystick.

This seems like a more appropriate direction in my opinion, also would benefit any core not just ours.

@zorro2055
Copy link
Contributor Author

Ok, from that info, here is my potential plan then. I'll wait for comments over the next day or two to see if y'all largely agree with this so I'm being the most efficient time wise. Some things might change a little as I have not dug deep yet into everything.

  1. Add a new function in libretro that calls the polling function, but adds algorithm used in this original pull request to eliminate one rotary joystick step from rotating the character two steps. This keeps the current polling function as is, but also allows other cores to access this new function if needed. This will be done in a separate PR for libretro.
  2. Put the following pseudo code in mame2003-plus at lowest level that I can which is probably the inptport.c file:
if (((type == IPT_DIAL) || ( type == IPT_DIAL_V )) && (useRotaryJoystick))
     newFunctionThatCallsRegularPollingFunctionAndEliminatesDoubleRotate();
else
     regularPollingFunction();

3a. To avoid disturbing other games that use very small steps for IPT_DIAL and IPT_DIAL_V, I do basically option 3 that I mentioned before. I add two new options to the Analog Controls menu, useRotaryJoystick (calls new function from libretro if set to yes), and Lockout time (how long to not allow new rotary input).
3b. Then, (this is a little different than my original option 3 idea) change large rotary step games to have the new preset PORT_ANALOGJOY instead of typically PORT_ANALOGX. PORT_ANALOGJOY would set the two new settings to Yes and 24 (milliseconds). PORT_ANALOG and PORT_ANALOGX would automatically have these settings set to No and 0.

So, if this is the current PORT_ANALOGX line and definition for SNK games from a certain time including Ikari Warriors:

src/drivers/snk.c:      PORT_ANALOGX( 0xf0, 0x00, IPT_DIAL, 25, 10, 0, 0, KEYCODE_Z, KEYCODE_X, IP_JOY_NONE, IP_JOY_NONE )
src/inptport.h:#define PORT_ANALOGX(mask,default,type,sensitivity,delta,min,max,keydec,keyinc,joydec,joyinc)

then this would be that line updated to PORT_ANALOGJOY and the PORT_ANALOGJOY definition added.

src/drivers/snk.c:      PORT_ANALOGX( 0xf0, 0x00, IPT_DIAL, 25, 10, 0, 0, 1, 24, KEYCODE_Z, KEYCODE_X, IP_JOY_NONE, IP_JOY_NONE )
src/inptport.h:#define PORT_ANALOGJOY(mask,default,type,sensitivity,delta,min,max,userotaryjoystick,lockouttime,keydec,keyinc,joydec,joyinc)

Doing it that way means I do have to touch the 241 different PORT_ANALOG and PORT_ANALOGX lines, just the 10 to 15ish lines of games that use the large step rotary joystick.

I would modify this PR to do items 2-3b after item 1 is completed with some testing of item 1 with item 2 to verify item 1 is working properly.

This is obviously going to take some time to do. I would think I would have item 1 done in 1-3 weeks.

Let me know if this is a plan that largely makes sense.

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Nov 7, 2023

Per driver edits here would be difficult to maintain, since backporting from newer mame would not have this. A thought on future proof design.... It seems to me that your goal is to adjust the polling rate to ignore a reading from these input types. I think it might be easier to create an option in the frontend solely. This reminds me that "polling behavior" is already an option under retroarch input options. Have you experimented with early vs late polling? If this doesn't help the situation, it maybe a place to add a new option, "delayed polling" with a secondary option to set a specific delay.

@grant2258
Copy link
Contributor

grant2258 commented Nov 7, 2023

I dont really agree with the direction of this on a logical basis then again I dont have any objections either way as long as your device works. Your handling of buttons should treating it as a rotary switch counter rather than a analogue dial device.

Think of it im terms rotary encoder/rotary switch scenario(digital vs analog). If the device has an analogue mode consider using that as a dial device. A button press for example is around 100ms between pressing and reaction/movement time for a human and that is being generous. No way can a human react in a time frame of 4ms so there really is little need for that kind of accuracy.

A lot of people try using frameskip for this kind of thing but the reality is your taking out the human reaction time. There really is no need for the pole at all. If you just catch the input as buttons in a 12 way counter with negative and positive direction the reaction time of devices 4ms wouldnt matter because it cant be physically done in that time scale. Just treat it as normal two button counter up and down rather than a dial. Since mame has a key speed this can be adjust to work already via the analog menu settings.

@mahoneyt944 I cant see how this can be put in the front end. You would need 12 inputs for position tracking and its seems to be a negative/positive two output on the device in question. RA also would need 12 inputs for a 12 way rotary switch and way to map it as well. There is also no clear way to map this on a RA as we would need two more inputs for this device as far as the front end goes.

A better solution would be a position(number switch) with sensitivity settings on how fast the button reacts for the sake of ra it would take digital buttons and analogue in mouse and joystick helpers. Since all these inputs are available you this could also be done on a core level. If Ra is doing it though there would also need to be a options for the separate input mapping for each input available for a real device. It would certainly need some thought if done through RA to be honest. I cant see anything beyond arcade emulators needing this though and each core would need updated to handle the new position code anyway.

@zorro2055 this is mainly between you and @mahoneyt944 inputs discussions can get heated easily and it just my perspective on how I would deal with it if i was to do it. So just take its as something to consider or ignore rather than any kind of request as im not affiliated with libretro or any of its projects, I just put commits in now and then. Someone that isusing the core will have there own perspective on how things should be done. If someone want something done a way you dont agree with, they probably should be doing it themselves in the way they want it,if it differs from the way you want to do it that is. I think the changes that would be required on RA would probably get a strong no go but I could be wrong.

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Nov 8, 2023

A core level solution would be okay, but if it requires hundreds of drivers to be modified, that maybe problematic in respect to future backports and such, so there would need to be care given in this regard.
As a side note, I've had some experience testing a few different spinners, and the internal pulse rate of these spinners can greatly change the analog settings needed to get an accurate feel. I personally ran tests on Trons spinner (a machine a actually own) to ultimarc's spinner, USB button hole spinner, some cheaper ebay spinners, and a dozen or so mice. All in which act very different. I find I have the most accurate control with the USB button hole spinner, which uses a lower pulse rate (I think around 300 - 600), but seems to play nice with most games. From memory, most of the other popular options out there use a pulse rate of 1200. It's of my opinion that these don't translate as well under certain situations such as quick changes or abnormally quick spins. I suspect this is due to the actual count of real hardware vs the extreme precision of these newer spinners, it seems you have to drown out the sensitivity to try to match them, which makes them less accurate.

@zorro2055
Copy link
Contributor Author

Thanks for comments so far. I was thinking and testing some of them. I should have enough time tomorrow to read through all of them and respond.

@zorro2055
Copy link
Contributor Author

zorro2055 commented Nov 9, 2023

Okay, first, I'll just clarify how the current Ultimarc 12 way rotary device works. When the joystick is twisted, it snaps a 12-way rotary switch from one position to the next. So the joy stick basically snaps from one position to the next position every 30 degrees. With every twist to the next position, the PCB that the 12 way switch is attached to simulates a button press that lasts a specific time, either about 4, 8, 20 or 40 milliseconds depending on the firmware setting. I have mine set at 20 milliseconds (Standard setting). There is a unique simulated button press for clockwise and counter clockwise rotation.

The simulated button press for clockwise and counterclockwise can be mapped to any keyboard, joystick, or mouse button. This is no option to have this 12 way rotary joystick report as a mouse axis (ie rotary encoder). This is the only aftermarket device that I have found that simulates the control style for Ikari Warriors and similar games.

Until that is when I saw this rotary joystick:
https://thunderstickstudio.com/products/grs-super-joystick
Note that it is available for pre-order only. It supports mapping to buttons for clockwise and counterclockwise rotation like my current Ultimarc 12-way rotary. However, it also supports looking like a spinner / mouse where the "distance", really rotation, that it travels is adjustable, for each 30 or 45 degree step I am guessing. So this might be the more ideal controller for these types of games since it can give rotation positionally, and probably still has fixed 30 or 45 degree steps that it snaps to (description does not clarify if it does snap in mouse mode). However, it is not shipping yet, and no one knows how well it actually works.

So yes, I'm interested in getting my current Ultimarc 12-way rotary to work with the best accuracy with this PR or similar.

Understood @grant2258 that this conversation is basically between @mahoneyt944 and me, but I think it makes sense to answer your questions where I can just so also @mahoneyt944 has the most information to help us resolve this.

@mahoneyt944 i did try the late polling option in RA and did again and it does not help. I think that is because if all polling points are later, then it is basically the same situation from a time point of view for potentially having one rotary step rotate the character two steps.

So if regular polling looks like this with the dashed lines being one rotary step (20 milliseconds long per my Standard setting) and P standing for polling times:

                 ------------------
                  P               P               P

then late polling is the same, but everything is shifted slightly later:
                    ------------------
                     P               P               P

As you can see, adding a setting for potentially more delay would only shift the second option further to the left and still have the same problem. That is why it seems the only option (with the control scheme that the ultimarc 12-way rotary joystick uses) is to ignore the next poll only if a rotary step was detected in the previous poll only per the algorithm of the PR (implemented by effectively locking out any more character rotation for 24 milliseconds after detecting a rotary step). Also, it does seem that the frontend setting applies to all games, but maybe it is possible to only target specific games. The way MAME is currently setup, it does seem to push for using the clockwise and counterclockwise button mappings in Dial which is probably why the Ultimarc 12-way rotary joystick is designed for that. You can see that for the PORT_ANALOGX pre-sets these clockwise and counterclockwise buttons right off the bat (the KEYCODE_Z and KEYCODE_X in the preset below from one line that I grepped):

src/drivers/snk.c:	PORT_ANALOGX( 0xf0, 0x00, IPT_DIAL, 25, 10, 0, 0, KEYCODE_Z, KEYCODE_X, IP_JOY_NONE, IP_JOY_NONE ) \

@grant2258 i agree that the ideal device would probably have a dedicated mapped button for every angled position of a 12-way rotary with that mapped button basically always "virtually" held down when the rotary joystick was in that particular position. Both this control scheme and the emulated mouse axis (rotary encoder) control scheme eliminates the need to basically skip polling for a frame when a rotary step is detected as this PR's algorithm is doing. Obviously, no hardware exists that supports this 12 virtual button control scheme at the moment. Although, perhaps it will show up in the future at some point. Also, this 12 virtual button control scheme might be the best, if hardware for it existed, because as @mahoneyt944 pointed out, rotary encoders can be a little squirrelly.

Anyway, so what to do now with the currently existing 12-way ultimarc rotary joystick?

Potential option 1 - If most of this PR effectively goes into RA / libretro per my proposed Step 1 (as originally suggested by @mahoneyt944 ), but it goes into a separate new C file so that makes reverse porting from later versions of MAME easier? As long as the original polling function name stays the same during reverse porting, then the new polling function in the new separate C file calling the original polling function, should not be affected. I would have to verify this could be done of course. I only really looked at the udev_input.c driver before in RA. I'd have to look at it more to see how that whole driver system in RA works before I could verify this would definitely work....

Potential option 2 - keep it in the core mame2003-plus. This is basically what this PR currently does right now.

For either of these options, again, to keep this algorithm change from affecting all analog ports (only needed with games that use the analog ports for large angle steps, such as Ikari Warriors, Time Soldiers, etc), I believe I would need to change to a new preset analog type for 10-15 games per my step 3a and 3b in an earlier comment. I would have to verify that I could easily have the other present analog types PORT_ANALOG and PORT_ANALOGX automatically set the 2 new analog menu items to values to not use this algorithm so that other games do not use this algorithm.

So, if option 1 is preferred, I would first research that it could be implemented as described and would report what I found here. I would also research and make sure that new analog menu items could have a default setting easily assigned in one function if that preset does not set that new analog menu item. If I reported here that these two things could be implemented, then I would think that option 1 is possible.

If option 2 is preferred, then I would research and make sure that new analog menu items could have a default setting easily assigned in one function if that preset does not set that new analog menu item. If I reported here that this one thing could be implemented, then I would think that option 2 is possible.

Well, hopefully that was clear enough. Let me know what you think.

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Nov 9, 2023

@zorro2055 I think it's worth looking into how and if the frontend could handle this polling, Option 1 as you mentioned.

I suppose for option 2, we could create a new analog type which would mirror PORT_ANALOGX but catch the polling in a way which works for your case use then update the few games that would use it. Might be worth looking into.

/* analog input */
#define PORT_ANALOG(mask,default,type,sensitivity,delta,min,max) \
PORT_BIT(mask, default, type) \
{ min, max, IPT_EXTENSION | IPF_SENSITIVITY(sensitivity) | IPF_DELTA(delta), IP_NAME_DEFAULT },
#define PORT_ANALOGX(mask,default,type,sensitivity,delta,min,max,keydec,keyinc,joydec,joyinc) \
PORT_BIT(mask, default, type) \
{ min, max, IPT_EXTENSION | IPF_SENSITIVITY(sensitivity) | IPF_DELTA(delta), IP_NAME_DEFAULT }, \
PORT_CODE(keydec,joydec) \
PORT_CODE(keyinc,joyinc)

@zorro2055
Copy link
Contributor Author

zorro2055 commented Nov 9, 2023

@mahoneyt944 sounds good. I'll start looking at the code to see if option 1 is possible like we are thinking and will report back.

FYI, from a quick check of the latest version of MAME, it did appear to have the same issues. So, if everything is as we think it might be, maybe I would submit to fix there issue also in the latest version of MAME which could be back ported to here. And then this could be updated to reference that. But there are a lot of twists and turns on that path which might lead to a dead end, so I'll continue with our tentative plan that you just outlined as discussed.

@zorro2055
Copy link
Contributor Author

zorro2055 commented Nov 13, 2023

@mahoneyt944 , ok, here is the research I have done so far on option 1. It seems possible, and fairly clean, but let me know what you think.

First, here are some of the relevant lines of code relating to implementing option 1. There are definitely multiple steps.

First, the function pointer "poll_cb" is set to point to the polling function.
Declaration of "poll_cb" in mame2003-plus:

static retro_input_poll_t poll_cb = NULL;

The type, "retro_input_poll_t" for "poll_cb" is defined in libretro.h (located in mame2003-plus and RA):
typedef void (RETRO_CALLCONV *retro_input_poll_t)(void);

"poll_cb" is actually set by the function "retro_set_input_poll" declared in libretro.h:
RETRO_API void retro_set_input_poll(retro_input_poll_t);

This function is defined in mame2003-plus:
void retro_set_input_poll(retro_input_poll_t cb) { poll_cb = cb; }

The "retro_set_input_poll" function is called in RA and selects "core_input_state_poll_maybe", defined in RA, as the polling function:
https://github.com/libretro/RetroArch/blob/f091b5a9e9475255e5efaded5f95c9750fdfe15e/runloop.c#L4569
Then things are set up for "poll_cb" to be called every frame it looks like:
poll_cb(); /* execute input callback */

So, here is roughly how I think a second poll function could be set for the games that used the 12-way rotary joystick in the arcades, and hopefully not get in the way of backporting features from later version of MAME.

First, two new files would need to be created in Retroarch (names are just placeholders):
new_filtered_analog_poll_function.h - declares new poll function and helper function to manage variables, would also exist in mame2003-plus so mame2003-plus can choose this polling function for the games that need it.
new_filtered_analog_poll_function.c - defines the new poll function and helper function. contains static variable that allows helper function to pass type (IPT_DIAL and IPT_DIAL_V) and milliseconds to lockout (24 ms right now) to new poll function, "new_filtered_analog_poll", and the pointer passed to it of the old poll function.

To select the new polling function if needed, more code would be added to the function definition shown above in mame2003-plus for "retro_set_input_poll". It would check to see a new setting in the Analog Menu, called say "Use CW/CCW Rotary Joystick".

If this setting is false, then the function sets poll_cb to the old poll function.
If this setting is true, the helper function is called and assigns to static variable defined in new C file, type to filter polls, ms of lockout, and pointer to old poll function. Then poll_cb is set to the new poll function.

The new poll function calls the old poll function, then filters the polled data that it needs to (types IPT_DIAL and IPT_DIAL_V).

I glossed over some details, but hopefully you get the idea. Feel free to ask questions if anything is not clear. If this concept seems okay to you, I would start coding it in to the point where I could test it out. If there are things that might make this better, please let me know that as well.

This option 1 would most likely also need an new port_analog type as well for the games that used a rotary joystick. I would work on that after I got code in place for the concept details I went through above.

@grant2258 This would not happen right now, but I was thinking about your concept of using 12 virtual buttons for the 12-way rotary joystick. I think that could be reduced to 6 virtual buttons in total. Then hitting each button is a position 60degs apart from each other. To select the 30deg positions between the 6 buttons, 2 buttons next to each other would be pressed. That would represent the "position" between these two buttons. Maybe someone will make hardware that supports that someday....

@mahoneyt944
Copy link
Collaborator

I don't see any immediate issues with your proposal, it maybe worth verifying that the analog menu option is initialized prior to polling if it's to be referenced when the core initializes.

@zorro2055
Copy link
Contributor Author

Great, sounds good! Good point, I assumed the analog menu option is initialized prior to poll with some of the functions I saw. But now I just followed the relevant functions, and this does seems to be the case.

In RA in function runloop_event_init_core,
https://github.com/libretro/RetroArch/blob/f091b5a9e9475255e5efaded5f95c9750fdfe15e/runloop.c#L4603
it appears that the analog (and all ports) are initialized multiple functions down into function event_init_content:
https://github.com/libretro/RetroArch/blob/f091b5a9e9475255e5efaded5f95c9750fdfe15e/runloop.c#L4744

Then it appears 10 lines down from function runloop_event_init_core, the polling function is selected going down multiple functions from function runloop_event_load_core:
https://github.com/libretro/RetroArch/blob/f091b5a9e9475255e5efaded5f95c9750fdfe15e/runloop.c#L4754C3-L4754C3

So hopefully that is right.

If all goes well, I should have this done in a couple of weeks. If I have questions along the way, I'll ask here. When I'm done, I assume I'll post a PR for Retroarch and update the code in this PR. If you are interested in seeing code along the way, let me know. A good half way point for that would be when I have all the code in RA and minimal code in mame2003-plus just to test the code in so far....

@zorro2055
Copy link
Contributor Author

Hello @mahoneyt944 , this is taking longer than anticipated, but I am making progress. Calling function definitions in the main executable (RA) from the library (mame2003-plus) is not done often, so it took some research and testing.

Now I have a question about which way to go. Let me know what you think.:
Scenario 1: No modification to existing RA source files (favored by me, but additional trickiness discussed below)
or
Scenario 2: adding 2 lines to RA source files.

In both scenarios, I have added the two new files, filtered_poll.h and filtered_poll.c, to RA. I also copied over filtered_poll.h to mame2003-plus. Also, in both scenarios, one line is added in Makefile.common in RA to add filtered.o to the OBJ list to compile.

Scenario 1: NO modification to existing RA source files at all, some modification to the RA build files.

For operating systems that support the ELF format, like linux, I was able to export the two symbols of the functions defined in filtered_poll.c for RA (the main executable). I did this by:

  • adding the following line in Makefile: LDFLAGS += -Wl,---dynamic-list=link-T
  • adding the file link-T:
{
        extern "C"
        {
                retro_core_input_state_filtered_poll_maybe;
                retro_set_filtered_poll_variables;
        };
};

For Windows based systems, I typedef'd the two functions, then from mame2003-plus, was able to "probe" the main executable for the function definition pointers of the two functions defined in RA, then assign these function pointers to local typedef'd variables to call those functions.

I was able to support both methods by using #ifdef, #else, #endif statements.

I was able to get this to work in linux and in Windows 11 using MinGW64. I do prefer this method, but the only thing that worries me is since this method does not seem very common, does it work on all the other platforms that RA supports? Is there a semi-easy way to check that without having all the hardware?

Scenario 2: Define a third function that is called from RA and defined in mame2003-plus. This function can pass the function pointers of the two functions in filtered_poll.c to mame2003-plus. From there mame2003-plus can call the functions. This scheme is used by RA currently multiple times. For instance, RA passes the polling function pointer to mame2003-plus by using retro_set_input_poll:
https://github.com/libretro/RetroArch/blob/1fab694b763b7b053ecbf5f89d0df2e4084b2b15/runloop.c#L4566
which calls the definition in mame2003-plus:
https://github.com/libretro/mame2003-libretro/blob/be247427a8a68f8401ce40c830e2d8767d000f84/src/mame2003/mame2003.c#L734

So this method has been proven to already work on all RA's platforms, but this way does have to add a call to a third function that would be declared in filtered_poll.h and a #include <filtered_poll.h> line to one of the current RA sources files, probably runloop.c.

So let me know what you think. Below is the code for my early filtered_poll.h, filtered_poll.c and modified retro_set_input_poll definition in mame2003.c for reference that demonstrates Scenario 1:
filtered_poll.h

#ifndef FILTERED_POLL_H__
#define FILTERED_POLL_H__

#include <libretro.h>

/* typedefs and #if statements with matching #else and #end if statment are commented out
 * when in the RetroArch project, but are uncommented when used in a core module. */

// #if defined(_WIN32) || defined(__CYGWIN__) || defined(__MINGW32__)
/* Setup typedef as function pointer template to get access to function in main executables
 * in Windows environments. */
// typedef void (*retro_core_input_state_filtered_poll_maybe_FUNC)(void);
// typedef void (*retro_set_filtered_poll_variables_FUNC)(void *filteredPorts, int timeLockout,
//                                                            retro_input_poll_t defaultPoll);
// #else
/* Just set up header which is supported on other OSs */
RETRO_API void retro_core_input_state_filtered_poll_maybe(void);
RETRO_API void retro_set_filtered_poll_variables(void *filteredPorts, int timeLockout,
                                                            retro_input_poll_t defaultPoll);
// #endif

#endif

filtered_poll.c

#include <stdio.h>
#include <filtered_poll.h>

static void *filtered_ports               = NULL;
static int time_lockout                   = 0;         /* milliseconds */
static retro_input_poll_t default_poll    = NULL;

void retro_core_input_state_filtered_poll_maybe(void)
{
   printf("Before polling in filtered_poll_maybe.\n");
   default_poll();
   printf("After polling in filtered_poll_maybe.\n");
}

void retro_set_filtered_poll_variables(void *filteredPorts, int timeLockout,
                                                         retro_input_poll_t defaultPoll)
{
   filtered_ports = filteredPorts;
   time_lockout= timeLockout;
   default_poll = defaultPoll;
}

modified retro_set_input_poll in mame2003.c

void retro_set_input_poll(retro_input_poll_t cb)
{
  int *ftrPorts = 0;
  int tLock = 24;

#if defined(_WIN32) || defined(__CYGWIN__) || defined(__MINGW32__)
  /* Get the module handle from the executing EXE */
  HMODULE module = GetModuleHandleW(NULL);
  /* Type-cast function pointer to proper typedef */
  retro_set_filtered_poll_variables_FUNC retro_set_filtered_poll_variables =
          (retro_set_filtered_poll_variables_FUNC)GetProcAddress(module,
          "retro_set_filtered_poll_variables");
  retro_core_input_state_filtered_poll_maybe_FUNC retro_core_input_state_filtered_poll_maybe =
          (retro_core_input_state_filtered_poll_maybe_FUNC)GetProcAddress(module,
          "retro_core_input_state_filtered_poll_maybe");
#endif
  retro_set_filtered_poll_variables((void *)ftrPorts, tLock, cb);
  poll_cb = retro_core_input_state_filtered_poll_maybe;
}

@zorro2055
Copy link
Contributor Author

Actually, after grepping RA, and a little more googling, it does look like the methods in Scenario 1 are already used in RA, so that seems to prove they would work across all RA build targets.

On the linux side, switch to using typedefs now as well like with windows, but with dlopen and dlsym function calls. This is already used in dylib.c:
https://github.com/libretro/RetroArch/blob/1fab694b763b7b053ecbf5f89d0df2e4084b2b15/libretro-common/dynamic/dylib.c#L187-L192

On the windows side, this method with GetModuleHandle and GetProcAddress already appear to be used in dylib.c:
https://github.com/libretro/RetroArch/blob/1fab694b763b7b053ecbf5f89d0df2e4084b2b15/libretro-common/dynamic/dylib.c#L162-L169

So, it seems I could go forward with Scenario 1, but switch to using typedefs, dlopen and dlsym for linux. Let me know if you see any issues with that, or if you are okay with me proceeding forward with Scenario 1.

@zorro2055
Copy link
Contributor Author

Further info I discovered on the windows side of Scenario 1: From these lines in lylib.c in RA, it looks like GetModuleHandle is not available in UWP, which seems to be what XBox uses? Perhaps if that is case, it could be compiled statically for UWP?
https://github.com/libretro/RetroArch/blob/93e5566b9a5c4505da0257aa62fe8080f6f0b875/libretro-common/dynamic/dylib.c#L155-L160

@mahoneyt944
Copy link
Collaborator

Sounds alright to me. @arcadez2003 might be able to answer the Xbox case for you. Not sure on that.

@zorro2055
Copy link
Contributor Author

zorro2055 commented Dec 5, 2023

Thanks @mahoneyt944 . If @arcadez2003 has any feedback, great. But I did do some googling and I found these instructions to compile RA for UWP / Xbox 1: link

It does look like the core is statically built into RA. Two package files are created, but one of them looks to only contain Visual C libraries. So, since I just discovered that I have a old PS Vita which still works (and also is statically linked when compiling RA for it), I will check to see if my code compiles and runs on that. If yes, then I'll check to see if it compiles for the Xbox 1. If yes, then, I would feel pretty confident that it would work on the Xbox 1, but I would not be able to test that. I'll go ahead on this, and report back with my results. If you see any issues with this though, let me know.

@zorro2055
Copy link
Contributor Author

Ok, I was able to successfully compile and run RetroArch with the mame2003-plus core per the code I gave above on PS Vita. This was with my added filtered_poll.h, filtered_poll.c and modified mame2003.c file. The dlsym and dlopen function calls that I was going to use instead for linux (non-windows) were not found, so I assume that library does not exist for PS Vita. But useing the code as I had it above (exporting the two functions from RA, then the linker links the declared functions from the core) worked. So I guess I'll stick with that.

Also, using the compile instructions from the link I posted before to compile a UWP for Xbox 1, Series S, or Series worked as well. I do not have the hardware to test running it. That targets x64. Maybe only UWP for ARM doesn't support the GetModuleHandle function call.

So, my plan is to continue on with more of the functionality per discussion in previous posts on here from a couple of weeks ago. Also, I'm thinking just in case there are some RA platforms out there that would not compile well with this (which I have not seen yet), I would wrap #ifdef (NO_FILTERED_POLL), #else, and #endifs around the code I add. That way, worse case, someone could define this variable in a Makefile to not include the added filtered poll code. It would just be the original poll function then.

Let me know if there are any comments or questions on this, otherwise, I plan to just continue on.

…Used to prevent single rotary step rotating character twice
…ctFromSingle12wayRotate

Sync this branch with changes that have occurred in the upstream master.
…nput, or saved poll data request filter) to be used to lockout analog rotation button events for a certain time to prevent one analog rotation click (simulated button) to rotate character more than once.
@zorro2055 zorro2055 force-pushed the avoidMultipleDetectFromSingle12wayRotate branch from a7801f5 to cbcc653 Compare March 28, 2024 17:50
@zorro2055
Copy link
Contributor Author

Hi @mahoneyt944 , 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. This pull request now reflects this latest code.

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
    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.

I'll post a link to the RetroArch pull request in another comment once I post that which should be shortly.

@zorro2055
Copy link
Contributor Author

Link to RetroArch pull request that is needed to for this pull request to work:
libretro/RetroArch#16389

@zorro2055
Copy link
Contributor Author

Good news @mahoneyt944 ! I just saw your comment about you should be able to let this through. Yes, I did try to think it out as best as I could.

@mahoneyt944
Copy link
Collaborator

Is this pretty much to fix issues with this code below reading twice per frame? Would it be adequate to just ignore additional reads if it was already read in that frame? As in 1 read per frame?

static int snk_rot12( int which ){
/*
This routine converts a 4 bit (16 directional) analog input to the 12
directional input that many SNK games require.
*/
const int dial_12[13] = {
0xb0,0xa0,0x90,0x80,0x70,0x60,
0xf0,
/* 0xf0 isn't a valid direction, but avoids the "joystick error"
protection
** in Guerilla War which happens when direction changes directly from
** 0x50<->0x60 8 times.
*/
0x50,0x40,0x30,0x20,0x10,0x00
};
int value = readinputport(which+1);
int joydir = value>>4;
static int old_joydir[2];
static int old_dial_select[2];
static int dial_select[2];
/* added to compensate for Guerilla War bug fix noted above.
** When dial_select is used to point to invalid 0xf0 (when set to 6),
** in the frame after dial select was set to 6,
** move to the next valid setting in the rotation
** the character was supposed to rotate to, 5 or 7
** which is used to point to 0x50 or 0x60 in dial_12.
*/
if ( dial_select[which] == 6 ){
if ( old_dial_select[which] < dial_select[which] ){
old_dial_select[which] = dial_select[which];
dial_select[which]++;
}
else {
old_dial_select[which] = dial_select[which];
dial_select[which]--;
}
}
else {
int delta = (joydir - old_joydir[which])&0xf;
old_joydir[which] = joydir;
if( delta<=7 && delta>=1 ){
if( dial_select[which]==12 ) dial_select[which] = 0;
else {
old_dial_select[which] = dial_select[which];
dial_select[which]++;
}
}
else if( delta > 8 ){
if( dial_select[which]==0 ) dial_select[which] = 12;
else {
old_dial_select[which] = dial_select[which];
dial_select[which]--;
}
}
}
return (value&0xf) | dial_12[dial_select[which]];
}

@zorro2055
Copy link
Contributor Author

zorro2055 commented Apr 5, 2024

Ah, the PR I submitted for that function fixed an issue when the 12-way rotates to 0xf0 which is not a valid position to get around a joystick error protection in Guerilla War according to the comments. My patch just held that position of 0xf0 for one frame, then went onto the next number so the character appears to rotate once rather than no times when the 12-way joystick rotates to 0xf0.

I never looked at where it got the input at that time. Looking now, it appears that it:

  • Gets the absolute value and the previous absolute value from two variable locally saved within mame2003-plus
  • Calculates the relative value from those two locally saved values
  • Scales that relative value by the sensitivity set in the analog controls menu and by the amount of time that has passed since the last rendered frame.
  • Add the previous absolute value to this relative scaled value and use that for the input.

The locally saved within mame2003-plus absolute value and previous absolute value look to be stored by the update_analog_port function (was between the functions I put in to check processing time in the my retroarch patch currently proposed) when it fetches the saved poll data from retroarch.

So it seems the work of fully processing the raw analog input data is split into two stages, so I don’t think only one call could be made. Of course, there does seem to be a lot going on, so maybe there is a way to cut down code somewhere, but it would definitely take some investigation trying to figure out all the interactions.

Hopefully, that somewhat answers your question.

More details on the retroarch -> mame2003-plus absolute values input and mame2003-plus -> previous absolute + scaled relative value snk game input.

retroarch→mame2003-plus absolute values

update_analog_port function:

update_analog_port(i);

locally stored previous analog input absolute value (input_analog_previous_value[]) within update_analog_port function:

input_analog_previous_value[port] = input_analog_current_value[port];

seq_pressed (in update_analog_port) drills down to eventually call input_cb to get saved poll data from retroarch for deincrement port (“button”):

if (seq_pressed(decseq)) delta -= keydelta;

seq_pressed (in update_analog_port) drills down to eventually call input_cb to get saved poll data from retroarch for increment port (“button”):

if (seq_pressed(incseq)) delta += keydelta;

after processing at end of update_analog_port, locally store current analog input absolute value (input_analog_current_value[]):

input_analog_current_value[port] = current;

mame2003-plus → prevous absolute + scaled relative value snk game input

readinputport function in snk_rot12 function:

int value = readinputport(which+1);

scale_analog_port function (sensitivity scaling and time since last frame scaling happens here) in readinputport

scale_analog_port(port);

input_analog_current_value[] and input_analog_previous_value[] processed here within scale_analog_port function:

delta = input_analog_current_value[port] - input_analog_previous_value[port];

which leads to setting the local variable input_port_value[] to the previous absolute + scaled relative:

input_port_value[port] &= ~in->mask;
input_port_value[port] |= current & in->mask;

This input_port_value[] is returned to snk_rot12 function in readinputport function:

return input_port_value[port];

@zorro2055
Copy link
Contributor Author

zorro2055 commented Apr 5, 2024

And I think I misunderstood your first question. This PR here keeps one rotation step of a 12 way rotary joystick from occasionally being detected over two frames. This is because one rotation step of the rotary joystick is simulated as a button press. This "button" must be "pressed" for slightly longer than a frame to guarantee it is detected. But that means one rotation step will be detected over two frames rotating the character two steps instead of one.
My first post goes over this in more detail.

Hmm, a memory (static variable) could be built into that function to ignore input if input was received on the last frame. So that would work I think. Although, the function does not know about time implicitly or what frame it is. So that would have to be built in. Edit: actually, i believe this function just runs every frame; my PR from before took advantage of that. So time does not have to be kept track of.

However, some of the games using 12 or 8 way rotary joysticks use a different driver, not snk.c. Such as snk68.c and probably others.

Also, notice even in snk.c, there are other rot functions such as snk_rot8 and snk_rotx. Those might be used by some of the games.

So if this method were implemented, all relevant rot functions in all the relevant drivers would have to be found and modified....

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Apr 5, 2024

Although, the function does not know about time implicitly or what frame it is.

This can be used to grab the frame at anytime

int cpu_getcurrentframe(void)

You can also use it to manipulate time to a degree, like I did here to hide crosshairs after 15 seconds of inactivity.

if(inactive_xy[player_number-1][2] + (Machine->drv->frames_per_second * 15) < cpu_getcurrentframe())

I suppose you could do something similar, flag when a frame hits in a static variable and lock out for recurring hits on that frame. Or buffer a number of frames.

@zorro2055
Copy link
Contributor Author

Ah, good to know about those two items. Okay, that could work, and I imagine is a lot faster. If we assume the game always runs at its native frame rate, then just tracking frames works the best. If the game ran slower than its native frame rate (not good for the game in general), then tracking time is better to not lockout rotation for too long.

This could also be implemented in just the update_analog_port for simplicity.

Basically, if the analog port is a IPT_DIAL or IPT_DIAL_V type, and a static variable says rotation happened last frame, then don't run these lines:

if (seq_pressed(decseq)) delta -= keydelta;

if (seq_pressed(incseq)) delta += keydelta;

This can be turned on and off with the X-Way Joystick setting I put in the Analog Controls menu as well. If we assume things will always run at the games native frame rate, then a time setting that I added in Analog Controls is not needed.

Are we exploring this because it is faster? Obviously, the current retroarch and mame2003-plus PRs I have proposed are slower, but are more general. Or will it depend on how the discussion with the retroarch PR goes?

@mahoneyt944
Copy link
Collaborator

I don't have authority over what the frontend accepts or rejects. So this could be another solution, core side only, if they decide against your PR at least. Good to have options. I know slowing anything down for the sake of one core probably isn't their first choice...but not sure what they will do

@zorro2055
Copy link
Contributor Author

Got it. Thanks @mahoneyt944 , that makes sense.

@mahoneyt944
Copy link
Collaborator

@zorro2055 this seems like a lot of overhead for a dozen or so games and likely only this core would be using this filtered polling.... As grant also pointed out this could effect other input types absolute vs relative too with how they are overlaid here. The latency is also a concern, although minimal is worth noting..Maybe there is a more simplistic approach or perhaps this might be a case where you want to branch this to your personal repo and use that core for these games only? I don't like to see your efforts go to waste, as I can tell a lot went into this, but I don't think we're ready to adopt this as it is today.

@zorro2055
Copy link
Contributor Author

Hi @mahoneyt944 , thanks for the feedback. It sounds like you talked with Libroretroadmin and grant.

Yeah, I did like the concept of it having the filtered polling function accessible with any core, but it did get more complex than perhaps initially imagined. If it is unlikely any other core would use filtered polling, then I am fine with getting rid of that. That would get rid of a lot of code in the just mame2003-plus PR. The filtered_poll files would go. The large function in mame2003.c would go as well as other stuff. The #ifdef NO_FILTERED_POLL statements would be removed.

It was a lot of work, but sometimes its hard to see the results before the work is done. With the results in hand, this new decision can be made. On the positive side, I definitely learned some stuff while doing it!

I would like to still put something in as this would support anyone that bought that 12-way rotary joystick. Plus then I don't have to keep a separate patched version.

I can rework this PR to just compensate for the one rotation of the joystick causing the character to rotate twice sometimes issue, no general "filtered polling". I was thinking about your using the frame count comment, and I think that always makes sense. No matter what the frame rate is, the "hold time" of the virtual dial button that a rotation of the joystick triggers must be a little longer than a frame, but not longer than two frames. So just using frame count works. So I could implement that.

Then, I just need the one added setting for each player, "X-Way Joystick", in Analog Controls instead of two. This is by default off so the behavior of the dial buttons is not changed. Everything acts the same as before this PR with this setting. Only when X-Way Joystick is set to on is a pressed dial button locked out for one frame after it is pressed. So this would be set to off (default setting) if a person were using a keyboard or perhaps other devices with these 15 or so games. If a person had the rotary joystick, they would set this to on for the particular game.

Here is a picture of that setting circled. Ignore the crossed out Lockout setting which I would remove:
image

Pseudo code for what I would do so this only affects the dial buttons (when X-Way Joystick setting is set to ON) in the area I mentioned on April 5th is:

if ((X-Way Joystick) && ( --cpu_getcurrentframe != frame_button_hit )
{
  if (seq_pressed(decseq))
     {
         delta -= keydelta;
         frame_button_hit = current_frame;
     }
} 

Does that basic plan sound good? I'm not sure how you mean by affecting the other absolute and relative types, but I think being able to turn this on and off with X-Way Joystick setting addresses that? Let me know if it doesn't though. Or maybe clarify what that means....

@mahoneyt944
Copy link
Collaborator

I think that sounds much simpler overall, @grant2258 what do you think?

@grant2258
Copy link
Contributor

Sounds good to me, the only discussion on my part was on here. It could be possible that the input offsets might need checked on other games since the structure size changed. Ill need to check the code it probably sets things to default if its different havent checked, even if it does its as simple as deleting your config files.

@zorro2055
Copy link
Contributor Author

Sounds good! It will probably take me a week or two to make this update.

For the defaults, I do know that when I went between testing this PR as is compiling with and without NO_FILTERED_POLL set (thus removing and adding the X-Way Joystick and Lockout settings), all input settings that I looked at reset. I had to set new dial keys, and Analog settings each time I did that including setting X-Way Joystick to Yes when NO_FILTERED_POLL was not enabled. But yeah, that behavior can definitely be double checked once I update this PR.

@zorro2055
Copy link
Contributor Author

Ok, that went faster than anticipated. I've updated per what we talked about . I tested it, and indeed, the character never turns two rotations (90 degrees) when the rotary joystick is rotated one step when X-Way Joystick is set to ON. When X-Way Joystick is set to OFF, then the character will occasionally rotate two steps (90 degrees) when the rotary joystick is just rotated one step.

Here is a screenshot when I first ran RetroArch with the updated mame2003-plus shared library. The values did reset and you can see what they are:
image

I had to change the pseudocode a little to make it work. I put the additional if statement within the if (seq_pressed(decseq)) since not all the analog buttons are IPT_DIAL and IPT_DIAL_V. There are a fair number of different analog types.

So the code for deincrement looks like this now:

if (seq_pressed(decseq))
    /* Don't register button press if xwayjoy is on, is DIAL(_V) type, and button was pressed last frame */
          if (  !(    (xwayjoy == IPF_XWAYJOY)
             && ((type == IPT_DIAL) || (type == IPT_DIAL_V))
             && (last_frame == last_frame_dec)   )  )
    {
      delta -= keydelta;
      last_frame_dec = last_frame + 1;
    }

So it says, basically, that if a button in the analog section is pressed, then don't register the button (skip the if statement body) if xwayjoy is on, input type is IPT_DIAL or IPT_DIAL_V, and the button was pushed in the last frame.

The same is done for the increment button.

Let me know if this looks good. If you need me to compress all the commits into one commit, I can figure out how to do that.

@mahoneyt944
Copy link
Collaborator

That looks good to me, are you ready to merge?

@zorro2055
Copy link
Contributor Author

Great, yes, I'm ready to merge. I assume once this is merged, it is okay to port this to mame2003?

@mahoneyt944
Copy link
Collaborator

Great, yes, I'm ready to merge. I assume once this is merged, it is okay to port this to mame2003?

Yes, you're welcome to port it there too if you want. Though you might want to let it simmer here for a little in case there's any bugs to work out.

@mahoneyt944 mahoneyt944 merged commit 1b56b20 into libretro:master May 21, 2024
@zorro2055
Copy link
Contributor Author

Ah, that makes sense. I'll do that. Thanks.

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented May 21, 2024

@zorro2055 Seems ok to me, since this is for IPT_DIAL / IPT_DIAL_V. It might be sensible to omitt the the x-way options from showing in the mame menu for all other analog types. Since they can't be used with those types anyhow. Such as lightgun etc
jpark-240521-051654

@zorro2055
Copy link
Contributor Author

zorro2055 commented May 22, 2024

@mahoneyt944 Good to hear it is working for you.

Ah, I see what you mean about showing up with the lightgun and others.

I guess there are two options for this, the second which you mentioned:

  1. Allow the X-Way Joystick option to be used with the other analog options. No idea if that would be useful.
  2. It looks like the X-Way Joystick option could probably be hidden in non Dial types.
    a. Easier way: the entries are hardcoded in with a #define ENTRIES 4 line that the rest of the menu system refers to. I could create a new variable that is set to ENTRIES when the analog device is a Dial and ENTRIES - 1 when the analog device is not a Dial. Then I would replace all instances of ENTRIES in the menu system with this variable. Since X-Way Joystick is the last entry, just using an if statement checking if a Dial device is used to display it. From inspection, it looks like this could work.
    Some code from that #define ENTRIES for reference:
    #define ENTRIES 4
    total2 = total * ENTRIES;
    menu_item[total2] = ui_getstring (UI_returntomain);
    menu_item[total2 + 1] = 0; /* terminate array */
    total2++;
    arrowize = 0;
    for (i = 0;i < total2;i++)
    {
    if (i < total2 - 1)
    {
    int sensitivity,delta;
    int reverse;
    int xwayjoy; /* Toggle lock out analog (de)increment for one frame if dial(-v) button just pressed */
    strcpy (label[i], input_port_name(entry[i/ENTRIES]));
    sensitivity = IP_GET_SENSITIVITY(entry[i/ENTRIES]);
    delta = IP_GET_DELTA(entry[i/ENTRIES]);
    reverse = (entry[i/ENTRIES]->type & IPF_REVERSE);
    xwayjoy = ((entry[i/ENTRIES]+2)->type & IPF_XWAYJOY);

    b. Harder way: similar to easier way, but would support if someone wanted an entry not at the end hidden. Have two #define lines, #define ENTRIES and #define ENTRIES_DIAL_DIAL_V. Define new variable that is set to ENTRIES if analog device, otherwise ENTRIES_DIAL_DIAL_V. Replace all other ENTRIES in menu system with this variable. Put if statement to populate X-Way Joystick if Dial device. Change for loop to not loop for every menu item, but to loop once per player. Do if else statements instead of case statements within that. If if statement is triggered, increase i with i++ statement. There are probably things I am missing there, but hopefully, you get the idea.
    That for loop for reference:
    for (i = 0;i < total2;i++)
    {
    if (i < total2 - 1)
    {
    int sensitivity,delta;
    int reverse;
    int xwayjoy; /* Toggle lock out analog (de)increment for one frame if dial(-v) button just pressed */
    strcpy (label[i], input_port_name(entry[i/ENTRIES]));
    sensitivity = IP_GET_SENSITIVITY(entry[i/ENTRIES]);
    delta = IP_GET_DELTA(entry[i/ENTRIES]);
    reverse = (entry[i/ENTRIES]->type & IPF_REVERSE);
    xwayjoy = ((entry[i/ENTRIES]+2)->type & IPF_XWAYJOY);
    strcat (label[i], " ");
    switch (i%ENTRIES)
    {
    case 0:
    strcat (label[i], ui_getstring (UI_keyjoyspeed));
    sprintf(setting[i],"%d",delta);
    if (i == sel) arrowize = 3;
    break;
    case 1:
    strcat (label[i], ui_getstring (UI_reverse));
    if (reverse)
    strcpy(setting[i],ui_getstring (UI_on));
    else
    strcpy(setting[i],ui_getstring (UI_off));
    if (i == sel) arrowize = 3;
    break;
    case 2:
    strcat (label[i], ui_getstring (UI_sensitivity));
    sprintf(setting[i],"%3d%%",sensitivity);
    if (i == sel) arrowize = 3;
    break;
    case 3:
    strcat (label[i], ui_getstring (UI_xwayjoy));
    if (xwayjoy)
    strcpy(setting[i],ui_getstring (UI_on));
    else
    strcpy(setting[i],ui_getstring (UI_off));
    if (i == sel) arrowize = 3;
    break;
    }

Let me know what you think!

Note: I just quickly checked about option 2a and 2b, so it might be more complex then I think, but hopefully not.

@mahoneyt944
Copy link
Collaborator

Being that adding options isn't a common practice, 2a might be ok... though I haven't looked at the code to know if changing the entries size will alter the config sizes when they save ( which might not be ideal either). I'm surprised there isn't a common parameter to hide the visibility of an entry without resizing the config?

It's not a major deal to leave it visible, as the toggle isn't functional for the other types but it definitely fills up the menu considerably. More of a visual thing.

grant2258 pushed a commit to grant2258/mame2003-plus-libretro that referenced this pull request May 22, 2024
…s steps occur when rotary joystick is twisted one position. (libretro#1652)
@zorro2055
Copy link
Contributor Author

zorro2055 commented May 22, 2024

Got it. Yeah, I skimmed all of the usrintrf.c file for some sort of disable feature and searched all of mame2003-plus for the words disable, grey and gray. Nothing came up. And it looks like only these first two variables are used for the menu, no obvious third variable for status such as enable or disable. So, I'm thinking it is pretty unlikely that feature exists.

menu_item below is the name such as "X-Way Joystick". And menu_subitem is the setting such as UI_on or UI_off. The other variables do not seem to relate... label and setting are temporary variables which are then assigned to menu_item and menu_subitem respectively

const char *menu_item[40];
const char *menu_subitem[40];
struct InputPort *entry[40];
int i,sel;
struct InputPort *in;
int total,total2;
int arrowize;
char label[30][40];
char setting[30][40];

From a UI point of view, it would be nice to get rid of those X-Way Joystick entries in the analog devices where they will not register a change. Let me try to put together a working 2a concept. I realized it will be slightly more complex because multiple analog devices can be used (and thus listed in this menu) for one game. For instance, Hard Drivin has analog petals and wheel, three separate analog devices. That will make it a little more complex, but I think it can be done. It will probably take me a week to get it together.

Right, the saving thing could be an issue. But it might be ok since different games can have different amounts of analog devices, so a different number of lines in the menu. So length differences between games may not trigger a reset unless the number of analog menu line items changes in a particular game. We'll see!

@zorro2055
Copy link
Contributor Author

zorro2055 commented May 23, 2024

Hi @mahoneyt944 ,
Okay, again, that went faster than I thought it would. I was able to not display the X-Way Joystick option in analog devices that were not IPT_DIAL and not IPT_DIAL_V. The game seemed to keep the same saved values even when I went back to mame2003-plus shared library file that always shows X-Way Joystick and back again.
From the screenshot from Super Speed Race Jr that uses an analog Dial and analog pedal, you can see the X-Way Joystick option is visible for Dial, but not for the Pedal as it should be.
image

So, it seems to be working pretty well. Should I open a new PR for that code for you to review? Or should I add that code to the branch that was for this PR and update that branch in github? Thanks.

@mahoneyt944
Copy link
Collaborator

Yes open a new pr. Nice work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants