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

do not cache lightgun input #1067

Merged
merged 28 commits into from
Jun 11, 2021
Merged

do not cache lightgun input #1067

merged 28 commits into from
Jun 11, 2021

Conversation

markwkidd
Copy link
Collaborator

I have tested this locally and it's working as it should.

I would like to trim away the other input caching eventually, but the lightgun seemed like a good place to start rather than biting off the whole thing at once the way I did with my previous PR #1044

@mahoneyt944
Copy link
Collaborator

This has the same touch screen issues we discussed before. In the current implementation, the "X-Y Device" core option determines what's appropriate to poll (mouse, pointer, lightgun, or none). We will need to preserve that feature for touch screens to work properly.

@mahoneyt944
Copy link
Collaborator

Tested touchscreen after adding the core option check. Seems to work fine now. Can't test the light gun since I don't own one ha. Btw what gun do you use?

@mahoneyt944
Copy link
Collaborator

Found something interesting,

  • I select the lightgun as my xy device.
  • Open mame menu
  • Select a input to map so it's highlighted waiting for a button press, then double click the trigger (double tap the touch screen)

It will select other triggers for other players?
Screenshot_20210603-172513

@markwkidd
Copy link
Collaborator Author

markwkidd commented Jun 3, 2021

Could you take a look at this issue to see if it sounds familiar to yours? libretro/RetroArch#12417

One of my headaches is that the RetroArch input code itself is buggy and not many cores are using the new lightgun API yet. Not sure how my code could do this, although of course that's the definition of a bug I guess :)

@markwkidd
Copy link
Collaborator Author

I use my laptop trackpoint for quick testing, but to actually play I've been using a wiimote.

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Jun 3, 2021

Hmm I'm not sure if the flycast bug is related or not. Hard to tell because it could easily be a port issue or polling issue in that core too.

As far as this core, I can only test the actual lightgun code with my touchscreen. I would test a real lightgun in the mame menu to see if we can reproduce the bug I saw.

Basically....

  • Set xy device to lightgun
  • Open mame menu
  • Click on any input so it's in waiting for a input to be pressed
  • Double click the lightgun trigger
  • see what mame maps

Edit: as far as the possible bug, the mouse doesn't have this issue if selected instead.
Let me try the lightgun bug before these commits to see if it's new or not.

@mahoneyt944
Copy link
Collaborator

Tested this bug with the current build and it doesn't exist. So it's a new big in this PR.

@markwkidd
Copy link
Collaborator Author

The MAME menu doesn't map those additional lightgun buttons when I test the PR locally.

I wish it would go wrong on my end to make this simpler to investigate

@mahoneyt944
Copy link
Collaborator

The MAME menu doesn't map those additional lightgun buttons when I test the PR locally.

I wish it would go wrong on my end to make this simpler to investigate

I just use my smartphone. Mines a motorola Z3. Works very well. Easy to setup and test.

@markwkidd
Copy link
Collaborator Author

I have a Google Pixel I could use, but I have never installed any apps other than through the Play Store. Kind of embarrassing. I assume you are downloading and installing RA manually on your phone?

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Jun 3, 2021

RA is on the play store too but it's delayed from live updates. This option installs like anything else. There are 2 options to choose from based of your Android version.

Personally, I installed the official version from retroarch.com instead - download stable from the home page. You will have to enable 3rd party installs on your phone under settings for your phone to install it though. It's updated live.

To install beta builds of mame, download them from the pipeline. Extract the zip it comes in. Then manually install it in the ra menu.

@markwkidd
Copy link
Collaborator Author

thanks for pushing me to finally get RA set up in android for the first time. I may not be able to spend much more time right now on this but I'm in a better position for sure.

is there any convenient way to read logs in android that you can recommend?

@mahoneyt944
Copy link
Collaborator

  • Click on the gear, then logging. Enable logging verbosity and log to file. Then select the level of core logging you want.
  • load the core, load the content.
  • close content, close Ra.
  • navigate to your retroarch directory on your device. Retroarch -> logs -> retroarch.log

I typically view it in a text editor app or on a online txt editor. Sometimes you have to rename the file extensions to ".txt" first.

Screenshot_20210603-194450
Screenshot_20210603-194517
Screenshot_20210603-194532

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Jun 4, 2021

I think the issue is in this return. Not sure why at the moment. I'm guessing input_cb returns 0 or 1?

return input_cb(port, RETRO_DEVICE_LIGHTGUN, 0, retro_code);

@markwkidd
Copy link
Collaborator Author

In the case of the trigger, I trace input_cb(port, RETRO_DEVICE_LIGHTGUN, 0, retro_code); to https://github.com/libretro/RetroArch/blob/master/input/drivers/android_input.c#L1455

That in turn leads me to consider https://github.com/libretro/RetroArch/blob/master/input/drivers/android_input.c#L565-L579

android_check_quick_tap is an intriguing function name.

@markwkidd
Copy link
Collaborator Author

I have yet to explore how the touch overlay fits into this scheme.

@mahoneyt944
Copy link
Collaborator

With our current build the lightgun works with the touch screen as it should. So by moving polling into osd_is_joy_pressed seems to cause this odd issue in this PR. I've tested a bunch of things and I can't see why it starts mapping multiple lightgun triggers. I started organizing the polling into its own function for simplicity.

https://github.com/libretro/mame2003-plus-libretro/compare/poll

It's come to my attention that poll_cb is called from retro_run and is important to be in sync for each frame. This maybe part of the issue here.

@ghost
Copy link

ghost commented Jun 4, 2021

From the android input code mark posted seems to me the multiple tab menu entry may be each player mouse index is set to zero and the lightgun is mapping to all player triggers to mouse index 0 because of that.

https://github.com/libretro/RetroArch/blob/master/input/drivers/android_input.c#L1455

try setting a different index for each player in input port settings for the mouse

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Jun 4, 2021

Our current build doesn't have the issue though. It came from this PR. I'm guessing because he's calling input_cb from osd_is_joy_pressed out of frame?

I also have my Android set to only 1 player port too. So the others are disabled. I'll test though

@ghost
Copy link

ghost commented Jun 4, 2021

I would really need to look at the code and what been changed. Im trying to stay away from input stuff libretro side of things

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Jun 4, 2021

Just to be sure I tested by changing the mouse index as suggested for the other ports and it still maps multiple gun triggers. So I think it's just a frame issue if moving the call to input_cb?

@markwkidd
Copy link
Collaborator Author

I don't see how the input callback should ever have junk data no matter when you are checking it.

From my understanding, polling data should be from the last time the callback was polled, even if that was many frames ago.

@ghost
Copy link

ghost commented Jun 4, 2021

code here

if (options.mouse_device == RETRO_DEVICE_LIGHTGUN)
{
retro_code = get_retrogun_code(osd_code);
if(retro_code != INT_MAX)
{
if(retro_code == RETRO_DEVICE_ID_LIGHTGUN_TRIGGER)
{
if(input_cb(port, RETRO_DEVICE_LIGHTGUN, 0, RETRO_DEVICE_ID_LIGHTGUN_RELOAD))
return 1; /* lightgun reload hack, report trigger as being pressed no matter what */
}
return input_cb(port, RETRO_DEVICE_LIGHTGUN, 0, retro_code);
}
}

you should log through mameui or retroarch to see whats triggering the reload .

@markwkidd
Copy link
Collaborator Author

I'm making some additions to one of the libretro test cores in order to make it easier to see what input is being polling, without the complication of the MAME code.

I just need to work on it a little more 🔢 then I'm hoping it will help narrow this down.

@markwkidd
Copy link
Collaborator Author

I could uncomment and modify this line:

/*log_cb(RETRO_LOG_DEBUG, "MAME is polling joysticks -- joycode: %i player_number: %i osd_code: %i\n", joycode, player_number, osd_code); */

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Jun 10, 2021

my thought is if the user doesn't want to use it why poll it? Idk lol.

Inptport.c

/* update mouse/trackball position */
if(options.mouse_device == MOUSE || options.mouse_device == POINTER)
  osd_xy_device_read (i, &(mouse_delta_axis[i])[X_AXIS], &(mouse_delta_axis[i])[Y_AXIS]);

/* update lightgun position, if any */
elseif(options.mouse_device == LIGHTGUN)
   osd_xy_device_read (i, &(lightgun_delta_axis[i])[X_AXIS], &(lightgun_delta_axis[i])[Y_AXIS]);

@markwkidd
Copy link
Collaborator Author

markwkidd commented Jun 10, 2021 via email

@markwkidd
Copy link
Collaborator Author

markwkidd commented Jun 10, 2021 via email

@mahoneyt944
Copy link
Collaborator

mahoneyt944 commented Jun 10, 2021

@markwkidd I threw some commits at it to lighten the load. Feel free to remove, add, edit, etc anything you wish. Here's where I stopped.

  • osd_is_joy_pressed needs work then optimized
  • osd_xy_device_read needs notes added.
  • pointer stuff

@mahoneyt944
Copy link
Collaborator

@markwkidd I'm not sure why, but when I tried to add the standard retropad in. It was crashing the core so I reverted that one. Otherwise it seems to be working on my Android phone so far. Needs more testing.

@markwkidd
Copy link
Collaborator Author

markwkidd commented Jun 10, 2021 via email

@mahoneyt944
Copy link
Collaborator

@markwkidd ok. I used the flag check from retro_run in osd_is_joy_pressed and that clears up the 1st frame crash. It's working now. I'd really like to optimize the heck out of osd_is_joy_pressed though. This seems to be a downside of this new method.

I'm wondering how much this even does.

The current implementation

  • in sync with the polling
  • the cached states are returned faster in osd_is_joy_pressed
  • cached states could be slower though with the extra layer?

This pr

  • not in sync. At least a frame off or more?
  • osd_is_joy_pressed now has to identify the input to return which means more actions are taken to return the state.
  • direct states should be faster though?

Seems to me we are trading off where the slow down is. While we save time not caching we lose time identifying the return and polling out of sync.

The real question is, which is actually faster? We'd have to setup some kind of controlled test between the two and log the delay.

@mahoneyt944 mahoneyt944 merged commit 0f11bc7 into master Jun 11, 2021
@mahoneyt944 mahoneyt944 deleted the lightgun branch June 11, 2021 10:04
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.

2 participants