Lua: Show a future frame#4598
Conversation
What if the other PR is merged first?
An event sounds good, tho I'd wait for a request to add it, because I can't imagine a case when it's needed. |
|
Looked at the code and I absolutely prefer this approach, great idea! Just please mention in the docs that the function we pass will be automatically passed current frame as an argument (or whatever its argument is, but that's what I understood by looking at it). Also mention that actual current frame remains the same, the only thing that changes is visuals that are taken from the target future frame... and possibly what it's used for, like we hack memory and then make that hacked frame appear. Because fake frame advancing now happens under the hood, so this info needs to be explicit. |
It should not be merged at all. I think this one is better and since you agree (and no one has disagreed) I will go ahead and close the other. I can directly commit the unpause fix to master.
Agreed. Besides, the callback that already exists for checking if the desired future frame has been reached would likely be able to serve that purpose. |
|
|
||
| public Func<int, bool>/*?*/ PreFutureFrameCallback { get; set; } | ||
|
|
||
| public int MaxFutureFrames { get; set; } |
There was a problem hiding this comment.
No. Nobody would want a value larger than int.MaxValue.
I do not recall considering what happens if a negative value is given, but I think the best thing to do in that case is either (a) not actually show a future frame, or (b) error.
Perhaps making it unsigned would fit with negative values being an error? But I'd rather not make it so easy to accidentally have a negative value get cast to >2 billion.
There was a problem hiding this comment.
If you make the API only accept non-negative integers then you won't have to worry about that possibility. There is no implicit int-->uint cast.
You could also reduce the width (to byte) to prevent extremely large values, but really you should be checking the user-provided value ASAP and throwing ArgumentOutOfRangeException.
There was a problem hiding this comment.
It doesn't matter that there's no implicit int -> uint cast. Someone will make an explicit cast without necessarily thinking about the implications. (Should they be aware? Yes. But I don't think making that change would help anyone be aware.)
Reducing it to byte might be a good idea, but I don't know a good way to determine what a hard upper limit should be. I wouldn't want an ArgumentOutOfRangeException preventing legitimate uses. (Still, 255 seems plenty high enough for what this feature is meant for. And low enough it won't force a user who accidentally triggers it to kill the process.)
There was a problem hiding this comment.
1 byte might be not enough for insane sonic situations. Reminder: in genesis sonic you can't force the camera to an arbitrary position, you have to gradually scroll it, otherwise things break. So depending on level width (romhacks exist too) we may have to scroll the camera really far, taking many frames.
There was a problem hiding this comment.
I do not recall considering what happens if a negative value is given, but I think the best thing to do in that case is either (a) not actually show a future frame, or (b) error. Perhaps making it unsigned would fit with negative values being an error? But I'd rather not make it so easy to accidentally have a negative value get cast to >2 billion.
I like the idea of an error on negative numbers. As for the limit, 65535 is probably a good one, so the type could just be a short (even signed short would give a decent limit).
There was a problem hiding this comment.
It is now a short. For Lua, it is long since lua only has 1 integer type. Making the C# side be short results in an implicit cast so instead I made it check that it fits in a short.
YoshiRulz
added
App: EmuHawk
Any news on this one? |
|
I tested this with sonic advance camhack in tastudio and it feels seamless! Video in the encode works as intended too. Audio is unusable but it wasn't usable before either, and we don't exactly needed it anyway because normal encode's audio could be used. Fixing the audio may be tricky too because it can't have pauses in the dump anymore, so that bit would have to be fixed first. Low priority overall. |
The "fix" I had intended to make at that time was somewhat problematic. I went with updating the documentation instead, telling users why the behavior is what it is and how to get what they want. That's still not totally ideal, since it won't work from a callback. But that is not related to this PR. Re-open #4591 if you want something done about that.
I was wondering about that, it looked wrong. But I don't plan on fixing that since it was already broken. |
vadosnaprimer
dismissed
YoshiRulz’s stale review
Suggestions were either implemented or not considered worthwhile.
|
conflicts appeared :( |
…pecifically for the use case they were made for. This significantly reduces the shenanigans and hacky feel of the code. Fixes various issues with invisible emulation.
SuuperW
force-pushed
the
show_future
branch
2 times, most recently
from
3c79a1f to
69a6a35
Compare
And were resolved. I'll go ahead and merge it for you. |
3 participants
This is an alternative to the invisible emulation changes made in #4592. (If this is merged, the unpausing thing from that PR still needs to be done.)
The old seekframe and invisibleemulation API methods have been removed, in favor of
show_future.Tool updates do not run during future emulation. Tools used to update during invisible emulation, which caused problems. But Lua would not update during
seekframe(except that input and memory callbacks would run), so this matches old Lua behavior.It is possible that a user would want Lua to run during future emulation (or in old terms, invisible emulation) but we can't know that. If we want to support this, it should be something such that scripts must opt-in to running during future emulation since most things should not run there. Maybe a new type of event.
Check if completed: