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

ConfigManager: Drop SkipIdle option. #3806

Merged
merged 2 commits into from
Oct 4, 2016
Merged

Conversation

degasus
Copy link
Member

@degasus degasus commented Apr 30, 2016

This option is safe + deterministic, so let's always enable it.


This change is Reviewable

@@ -119,7 +119,7 @@ void Jit64::lXXx(UGeckoInstruction inst)
// ... maybe the throttle one already do that :p
// TODO: We shouldn't use a debug read here. It should be possible to get
// the following instructions out of the JIT state.
if (SConfig::GetInstance().bSkipIdle &&

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@JMC47
Copy link
Contributor

JMC47 commented Apr 30, 2016

@RisingFog people say to disable idle skipping for TASing. Is that not true any more?

@degasus
Copy link
Member Author

degasus commented Apr 30, 2016

@JMC47 Idle skipping has some timing variances of a few cycles. So it must be consisent to sync on TAS, but it should not matter for syncing with the real hardware.

@RisingFog
Copy link
Member

@JMC47 I mainly turned it on because it was recommended, without questioning why. If removing the options is safe and deterministic, I don't see why we shouldn't remove it.

@JMC47
Copy link
Contributor

JMC47 commented Apr 30, 2016

Idle skipping is deterministic (proven by netplay) but...

Is it deterministic with savestates?

@magumagu
Copy link
Contributor

SkipIdle is completely deterministic and safe in single-core mode, but it has some funny interactions with dual-core mode; it works as a proxy for SyncOnSkipIdle, and also causes some CPU timing differences which might have unpredictable effects.

That's not really an argument for keeping the option, but it might make sense to wait until after 5.0 (along with #3804).

@JMC47
Copy link
Contributor

JMC47 commented Apr 30, 2016

Oh yeah, tons of people turn off idle skipping because it's slower. Such as in Rogue Squadron 3

@degasus
Copy link
Member Author

degasus commented Apr 30, 2016

@magumagu Never ever in 5.0, of course.

@Fallcrest
Copy link

@JMC47 How much of a performance difference is there in Rogue Squadron 2 (or any other game that this affects)? I think removing this hack is likely worth it now that 5.0 has been released, unless it really has bad behaviors with Dual Core mode.

@degasus
Copy link
Member Author

degasus commented Sep 7, 2016

@Fallcrest I don't want to drop the hack, neither idle skipping. I want to drop the option and to force enable idle skipping.

@degasus
Copy link
Member Author

degasus commented Sep 17, 2016

Rebased. And I've dropped this very outdated comment. Ready to merge IMO

@@ -1432,7 +1426,7 @@ void SaveRecording(const std::string& filename)
header.recordingStartTime = s_recordingStartTime;

header.bSaveConfig = true;
header.bSkipIdle = s_bSkipIdle;
header.bSkipIdle = true;

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@shuffle2 shuffle2 merged commit ab01dec into dolphin-emu:master Oct 4, 2016
@JMC47
Copy link
Contributor

JMC47 commented Oct 4, 2016

Could we replace the Idleskipping option in the menu with "Sync on IdleSkip?"

@ghost
Copy link

ghost commented Jul 31, 2017

@degasus

Please reconsider in re-instating this option to Dolphin again since games like Resident Evil 0 depends on turning "Enable Idle Skipping" OFF which you can't do anymore.

You could circumvent the crashes by choosing JITIL Recompiler in older builds but since it's gone in Dolphin too there is absolutely no way in making progress in Resident Evil 0 at all with newer builds (check this issue)

A lot have happened and have been removed from Dolphin which is fine by me but when you have no other option but to stick with ancient builds just to get a game going that's where it stops to being fun for me.

Thanks in advance!

@JosJuice
Copy link
Member

@Sigmavirus Absolutely no way? Are you sure that disabling sync on idle skip (which still is an option, albeit INI-only) doesn't help?

@ghost
Copy link

ghost commented Jul 31, 2017

@JosJuice
Hmm "Disable Sync on Idle Skip" through .ini, is that a new option?
How do I try it out, what's it called in the .ini?

I'm willing to test it out right now.

@JosJuice
Copy link
Member

[Core]
SyncOnSkipIdle = False

It was added in 4.0-4837, so it's not especially new.

@ghost
Copy link

ghost commented Jul 31, 2017

OK, never seen that option before but I'll try it out as fast as I can because I have to start a new game and progress all the way to the point where you have to use the Panel opener on the train to trigger the cutscene where Dolphin always crashes.

If it still crashes with this option and in case they aren't related, I hope you'll consider re-instating "Enable Idle Skipping" otherwise Resident Evil 0 is useless because it's not just crashing on the train but other places later on in the game as well.

@ghost
Copy link

ghost commented Jul 31, 2017

@degasus @JosJuice

I have now tested it out with

[Core]
SyncOnSkipIdle = False

in the GBZ.ini and with JIT Recompiler selected, still crashes the game at the exact same spot as it always have.

It's all up to you if you want to listen to my plea, but as of right now I'll have to stay with 5.0-3318 where JITIL Recompiler still exists so I can bypass these crashes for just this game.

Re-instating JITIL Recompiler is not an option I know, but "Enable Idle Skipping" might be?
5.0-910 with "Enable Idle Skipping" OFF and JIT Recompiler works perfectly fine.

@bb010g
Copy link
Contributor

bb010g commented Aug 10, 2017

You may want to make an issue on the tracker for the RE0 issue.

@JosJuice
Copy link
Member

There is already one, and SigmaVirus included a link to it in their first comment on this PR.

@degasus degasus deleted the idle branch February 3, 2019 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
9 participants