stella-emu / stella

A multi-platform Atari 2600 Emulator
https://stella-emu.github.io
GNU General Public License v2.0
626 stars 111 forks source link

California Games not advancing past BMX event #858

Open sa666666 opened 2 years ago

sa666666 commented 2 years ago

Bug has apparently been present since Stella 3.9.3, and is still present in master!

ROM: California Games (1987) (Epyx).zip State file from 6.5.3 (submitted by bug reporter): California Games (1987) (Epyx)_state_6.5.3.zip State file from master: California Games (1987) (Epyx)_state_6.7pre.zip

This was reported by PM to AtariAge; I will include the relevant info here.


When playing California Games in normal mode (meaning that you're progressing from completing one event then moving on to the next), the game will not go past the BMX downhill event once complete. It will just stay at the screen with the bike at the finish line and never move to the next screen. It should display the team with the high score, then move on to the Surfing event. I tried with other emulators and it behaves as expected, but when playing with Stella, it "hangs" after the BMX event. Is this a known issue?

This save state0 will put you right before the last screen on the BMX event. Once you cross the finish line, you won’t be able to continue. It’s supposed to show the event winner screen and then go to the surfing event when you press the button.

I also added a save state for the end of the footbag event (state1). You'll see that when you press the button, it behaves as it should for this event.


I tried in Stella 6.5.3, and confirmed it there. And also in latest master (as of Dec. 29), with the same results. I set emulation speed to 1000%, to get to the desired point very quickly.

Some oddities:

thrust26 commented 2 years ago

Adding @DirtyHairy because the fact that it is working at other speeds than 100% makes me wondering.

sa666666 commented 2 years ago

Yeah, that makes sense.

thrust26 commented 2 years ago

Ideas: ~The code (non intentionally?) also reads INPT4 via PLA multiple times during kernel display. I wonder if that affects Stella.~ The code checks the fire button twice per frame, once before and once after the kernel. It checks if the buttons is pressed and released (or vice versa). The button press is only detected if the button is pressed when the 2nd check happens. Does Stella maybe update the button state not frequently enough? Somehow this reminds me at #689.

thrust26 commented 2 years ago

Attached is a simple test program, which checks the fire button twice per frame just like CG does. The background color should change when the button is pressed or released. This does not happen in Stella but works for other emulators.

test_button.zip

sa666666 commented 2 years ago

It doesn't work in MAME or z26 either, but does in Stellerator, gopher2600 and clksignal.

thrust26 commented 2 years ago

Findings:

test_button_v2.zip

Some thoughts:

Ideas for solutions:

  1. We could "slow down" emulation, e.g. by emulating only fractions of a frame each step, poll events, sleep and then continue with the next step. Each step should take about the same amount of time. For CG two steps per frame would be sufficient, and I cannot think of any realistic scenario which would require more.
  2. We could poll events parallel to the EmulationWorker and cross our fingers, hoping that the first (INPT4) state check happens before we have polled new events. (tested, this works in debug mode on my machine)
  3. We could move the beginning of a frame e.g. to the start of the display kernel. So the VBlank code reads the result from the current, and Overscan the result of the next poll.
  4. ...?
thrust26 commented 2 years ago

@DirtyHairy @sa666666 How shall we continue here?

sa666666 commented 2 years ago

How about, in addition to the normal polling, we poll at each read of INPTx too? Would this cause too much slowdown, I wonder?

If there are no pending events, this is essentially a small check (check returns false, so method returns immediately).

thrust26 commented 2 years ago

We are doing this for paddle and trackball games anyway, no?

sa666666 commented 2 years ago

No, currently all controllers are querying myEvent, which is essentially a cached state of the events from the last time polling was done in EventHandler. What I am proposing is to directly call EventHandler::pollEvent at each INPTx read. If there are no pending events from SDL, then this is a simple check (while(SDL_PollEvent(&myEvent))) that exits immediately. Otherwise it will get the pending events and put them in myEvent, from which the current code can then read it and continue as it is now.

thrust26 commented 2 years ago

Hm, paddles are read multiple times per frame. So here Stella must update multiple times per frame. We should do the same with joystick controllers.

sa666666 commented 2 years ago

Events are only grabbed from SDL once per frame. If the paddles are checking multiple times per frame, they are checking a cached event stored in myEvent. They are not polling SDL directly multiple times per frame. The latter is what I'm proposing.

thrust26 commented 2 years ago

Got you. Polling with each INPTx might become quite slow though. Maybe we should define a minimum interval between polls, e.g 1/2 frame.

sa666666 commented 2 years ago

Well, first I would try it on every read, to see if it even fixes the issue. Then if that works, we can further optimize it.

thrust26 commented 2 years ago

If we limit the change to INPT4/5, then we should be fine. Makes no sense for analog input anyway.

thrust26 commented 2 years ago

@sa666666 Do you have an idea how to add this extra polling? Usually the controller code has no access to event handling.

sa666666 commented 2 years ago

It's going to require some refactoring. New method(s) need to be added to EventHandler, and then the object needs to be accessible from the Controller class. I suggest whatever we do, get it test "quick and dirty" first, to see if it will even help. Then we can worry about doing it cleanly.

thrust26 commented 2 years ago

Wouldn't a change in OSystem::mainLoop() be much less invasive and more versatile, too?

sa666666 commented 2 years ago

I don't know. And I guess we need to experiment. I'm not even entirely sure my suggestion will fix the problem. I think this work needs to be done in a branch first, since it has the potential to be very invasive either way.

DirtyHairy commented 2 years ago

Be careful with this. Afaik accessing input is only legal on the main thread on some platforms, and emulation runs on thread.

thrust26 commented 2 years ago

How about a separate thread which only polls the input multiple times per frame?

Have you seen SDL_AddTimer?

thrust26 commented 2 years ago

Be careful with this. Afaik accessing input is only legal on the main thread on some platforms, and emulation runs on thread.

Two ideas here:

  1. interrupt emulation when INPT4/5 are accessed, sleep*, poll and continue emulation
  2. sleep* emulation when INPT4/5 are accessed, poll multiple times per frame in a separate thread

* sleep until current real time

DirtyHairy commented 2 years ago

If I understand (1) correctly (terminate timeslice prematurely -> return to main thread -> poll -> go for next timeslice), this should work.

I don't think (2) is viable; afaik polling has to happen on the main thread in order to be safe across platforms --- this is not just about race conditions, but about assumptions made by the API of windowing systems.

thrust26 commented 2 years ago

If I understand (1) correctly (terminate timeslice prematurely -> return to main thread -> poll -> go for next timeslice), this should work.

Not quite. After polling, the current timeslice has to be continued before we go to the next one.

I don't think (2) is viable; afaik polling has to happen on the main thread in order to be safe across platforms --- this is not just about race conditions, but about assumptions made by the API of windowing systems.

Then it must be made possible in the main thread to continue running (and polling) while emulation is dispatched.

thrust26 commented 1 year ago

@DirtyHairy @sa666666 How should we proceed here?

How would you interrupt emulation? I suppose this has to be triggered in TIA class? Since I still have problems to follow the code here, I suggest someone how knows the code, should do it.

thrust26 commented 1 year ago

@DirtyHairy @sa666666 Bump. IMO we should fix this for 7.0.

thrust26 commented 10 months ago

@DirtyHairy @sa666666 2nd Bump