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

Movie recording and IEmulator.DeterministicEmulation #1290

Closed
Asnivor opened this issue Aug 20, 2018 · 14 comments
Closed

Movie recording and IEmulator.DeterministicEmulation #1290

Asnivor opened this issue Aug 20, 2018 · 14 comments
Labels
App: EmuHawk Relating to EmuHawk frontend Meta Relating to code organisation or to things that aren't code

Comments

@Asnivor
Copy link
Contributor

Asnivor commented Aug 20, 2018

Currently DeterministicEmulation is ignored when starting to record a movie.

Unless anyone has any objections, we really should be enforcing DE for movie recording.

@Asnivor Asnivor added the App: EmuHawk Relating to EmuHawk frontend label Aug 20, 2018
@vadosnaprimer
Copy link
Contributor

Ignored for all the cores? O_O

@Asnivor
Copy link
Contributor Author

Asnivor commented Aug 20, 2018

I'm assuming so. I dont see anything here: https://github.com/TASEmulators/BizHawk/blob/7ff4cea6c7fb2e133ab9a233ae6fb1fadf9a1788/BizHawk.Client.EmuHawk/movie/RecordMovie.cs

I have tried recording a movie with a core that exposes DeterministicEmulation as a SyncSetting (ZXHawk in this case) and the movie recording process starts up when DE==false

Unless I am missing a piece of the puzzle, I would expect other cores to behave the same way (the ones that have this setting available to change that is)

@Asnivor
Copy link
Contributor Author

Asnivor commented Aug 21, 2018

So speaking with @adelikat and @zeromus in IRC, the general consensus is that DE should be forced TRUE whenever a movie is recorded. However, it seems like ZXHawk is the first core to actually present DE as a SyncSetting.

Other cores that support it have this bool passed in directly through their constructors. As far as I can tell, this usually comes from a LoadRomArgs object before-hand where DE is null in every case I can see (although I might be missing somewhere where this is actually not the case).

When a movie is started, the following line effectively forces DE on:

bool deterministic = args.Deterministic ?? Global.MovieSession.QueuedMovie != null;

..because args.Deterministic is usually null. The RomLoader object is then created with this DE setting. If a core were to be launched where DE is not null in the LoadRomArgs then it looks like the DE setting would be respected (even if it was false) when a movie is started.

TLDR: Deterministic Emulation is implemented in IEmulator, but barely any cores make use of it (and if they did, it would mean that DE could be false when a movie runs).

It seems to me that the simple solution to this is:

  • Ensure ZXHawk accepts an additional DE bool? in its core constructor (as other cores do) that (if not null) overrides whatever DE value is stored within SyncSettings for the core.
  • Modify the linked code above to behave properly with respect to movies if the LoadRomArgs.Deterministic value passed in is not null (forcing it to true if Global.MovieSession.QueuedMove != null).

Unless I am missing something, this should take care of all current cases, and future proof things slightly for the future.

@vadosnaprimer
Copy link
Contributor

I don't know when it should be false (maybe some cores have casual gaming profiles), but I don't think DE should be a sync setting. SyncSettings is only a thing for movies, and for movies DE must be always true, so it can't change as long as we're (re)recording. And yes, if a core even allows to skip some measures that actually make it deterministic, for such cores we must force DE. If it basically changes nothing, then it doesn't matter, but this also means it should always be set to true for them.

@Asnivor
Copy link
Contributor Author

Asnivor commented Aug 21, 2018

hmm, so how would I go about letting the user select deterministic emulation for a core if it's not stored in SyncSettings? Unless I have missed it, there doesnt appear to be a way to do this already...

@Asnivor
Copy link
Contributor Author

Asnivor commented Aug 21, 2018

And yes, if a core even allows to skip some measures that actually make it deterministic, for such cores we must force DE. If it basically changes nothing, then it doesn't matter, but this also means it should always be set to true for them

This is effectively what the PR does. Even if the user is running ZXHawk and DE == false in syncsettings...

@vadosnaprimer
Copy link
Contributor

I don't think the user should be able to decide whether they would like to lose their leg or not. I'll have to look up which cores even depend on this thing, and when they want it to be false.

@Asnivor
Copy link
Contributor Author

Asnivor commented Aug 21, 2018

Oh, I just re-read what you wrote.

So DE is pretty much controlled via user profile selection at the moment?

If that is the case, then I should probably just remove this as a syncsetting completely in zxhawk...

@Asnivor
Copy link
Contributor Author

Asnivor commented Aug 21, 2018

I'll have to look up which cores even depend on this thing, and when they want it to be false

Makes sense. Unless there are cores that by default expect a null value to be passed in and a false value generated as a result (which seems highly irregular but I guess might be that way for reasons), the PR will still work.

Otherwise the PR works in exactly the same way (i.e. if a non-null value is passed in that is used unless a movie is queued).

@zeromus
Copy link
Contributor

zeromus commented Aug 21, 2018

I would like for DeterministicEmulation to be a sync setting. It is. Whether we expose that to the user is a separate question. I don't mind special logic curating what we show the user in the presentation. Buried in the inner logics, it's too complicated. Maybe that means we filter DeterministicEmulation out of a property list for showing to the user. Maybe that means we use reflection to see if a DeterministicEmulation property is available, and set it to true if it is.

@Asnivor
Copy link
Contributor Author

Asnivor commented Aug 23, 2018

I'm not sure where I would start with that (or even if I understand correctly).

You want it for all cores to be stored within the core's SyncSettings, and for this to be forced ON whenever a movie is queued?

@zeromus
Copy link
Contributor

zeromus commented Aug 25, 2018

Yes, and yes. You don't have to do all that. That's just what makes the most sense.

It's weird for DeterministicEmulation to be privileged above other options. There are similar things that need to be setup when recording a movie. For instance, one could put all the RTC emulation types in SyncSettings (choose to follow system time, choose t=0). The frontend would have to know to do the right thing in that case too.

One possibility: we could have is way for the cores to proffer an alternate set of default sync settings for movies (or perhaps the DeterministicEmulation flag could be renamed to InitSyncSettingsForMovie). However, remember in some cases there may be several options for how to run (all deterministic, just different) which are suitable for movies. That's why I prefer to put this stuff in the frontend.

If there's any core service concerning determinism, maybe it could just be something that asserts that the SyncSettings are deterministic. That way, if a core was ever changed so that a new sync setting was added which has implications on determinism, and for some reason it was opt-in to determinism, the frontend would be automatically running its old sync settings through that validator and the core would catch it--then some programmer would know he has to setup the movie preparation logic which sets sensible sync settings for movies.

@Asnivor
Copy link
Contributor Author

Asnivor commented Dec 5, 2018

So I'm not entirely sure whats going on here now. @adelikat merged the PR a while back. Do we close this issue out?

@Asnivor
Copy link
Contributor Author

Asnivor commented Feb 16, 2019

This issue is pointless.

The issue it addressed was non-existent, and the subsequent fix broke stuff that then had to be fixed again.

As it is, DE is currently OFF unless the core manages its own sync settings. Unless there is a movie running, then DE is always on.

This is probably how it should be.

@Asnivor Asnivor closed this as completed Feb 16, 2019
@YoshiRulz YoshiRulz added the Meta Relating to code organisation or to things that aren't code label Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App: EmuHawk Relating to EmuHawk frontend Meta Relating to code organisation or to things that aren't code
Projects
None yet
Development

No branches or pull requests

4 participants