ThunderAxe31 commented 1 year ago

I tried the latest devbuil (10ba45d4) and I noticed that the fix for issue #3014 wasn't what I was looking for, but I see that it was my fault for not understanding the actual nature of the bug. Here is a more exact description of the problem.

Version: from 2.6 to latest devbuild

Repro:

Open any game

Launch this script:

gui.pixelText(50, 50, "TEST")
while true do
emu.frameadvance()
end

You will see the word "TEST" remaining on screen despite the script drawing it only for 1 frame.

Expected behavior: The word "TEST" should disappear after advancing 1 frame from the moment of executing the script. Note that it was the behavior in all BizHawk versions prior to 2.6.

Suggested solution: When a Lua drawing function is used, have the Lua surface getting updated also on the next frame.

vadosnaprimer commented 1 year ago

Yeah, unless the drawing is inside a regular callback or a loop, it shouldn't persist.

CasualPokePlayer commented 1 year ago

If you want old behavior:

gui.pixelText(50, 50, "TEST")
while true do
    emu.frameadvance()
    gui.clearGraphics()
end

Persisting graphics is a feature, not a bug. This is for scripts seeking the performance of not drawing everything (in software!) every frame. If you don't call any gui draw functions nor explicitly clearGraphics, everything will persist. If you call gui draw functions, the previous frame's contents will be clobbered. If you want every frame to have graphics cleared, put gui.clearGraphics() in your while true loop.

ThunderAxe31 commented 1 year ago

Persisting graphics is a feature, not a bug. This is for scripts seeking the performance of not drawing everything (in software!) every frame. If you don't call any gui draw functions nor explicitly clearGraphics, everything will persist. If you call gui draw functions, the previous frame's contents will be clobbered. If you want every frame to have graphics cleared, put gui.clearGraphics() in your while true loop.

First off, my proposed solution doesn't affect performance because it would update the the Lua surface only one extra time, on the subsequent frame in which a drawing function was called, and only in the case that it isn't already called due to a new drawing.

Second, even if we wanted to keep this behavior, its design does also defeat its own purpose. If you need the drawings to remain on screen, when why should they disappear when a script draws something different entirely during a later frame? See example below.

gui.pixelText(50, 50, "This text will be lost like tears in rain.")
emu.frameadvance()
gui.drawLine(0, 0, 100, 100)
while true do
    emu.frameadvance()
end

vadosnaprimer commented 1 year ago

What if I need some graphics to go away (when some variable is false) and other graphics to stay? How many people have been requesting for graphics to stay on the new frames without calling draw functions?

YoshiRulz commented 1 year ago

Second, even if we wanted to keep this behavior, its design does also defeat its own purpose. If you need the drawings to remain on screen, when why should they disappear when a script draws something different entirely during a later frame? See example below.

That's just #2501 again.

CasualPokePlayer commented 1 year ago

First off, my proposed solution doesn't affect performance because it would update the the Lua surface only one extra time, on the subsequent frame in which a drawing function was called, and only in the case that it isn't already called due to a new drawing.

"Update the lua surface" means "clearing the lua surface." Which itself has a performance consequence if you want to lazy draw. If you don't want to lazy draw, just draw every frame (in practice, that's what happens if people don't try to lazy draw, not like your unrealistic example). If you really want to clear the surface explicitly every frame, you can call gui.clearGraphics to "update" it.

Second, even if we wanted to keep this behavior, its design does also defeat its own purpose. If you need the drawings to remain on screen, when why should they disappear when a script draws something different entirely during a later frame? See example below.

2501 is why. The faulty code is the same. We would need some way to copy everything back (with hardware acceleration, otherwise it's stuck with slow software rendering). For now, if you want lazy drawing, you have to batch it all in one frame (in theory that's probably not so hard to do with a table of lua functions to call, but that's quite annoying).

ThunderAxe31 commented 1 year ago

Ok, so was this change introduced just to save CPU usage. Is there even one example of a script that would benefit from this? The point of using Lua for drawing is to display stuff that tend to change, a lot. Like for watching RAM values, display a minimap, statistics, etc. I can't even begin to imagine a hypothetical script that would need to display unchanging stuff, except something really mundane like a streamer logo or a picture frame. Which obviously would be the exception by far, so it would make much more sense to make a Lua function for having this behavior only when explicitly asked, like gui.makeDrawingsPersistent(boolean) or something. Because right now the default behavior is really unfriendly and unintuitive, requiring a coder to slap a custom workaround in every single Lua script they use. A program is supposed to prevent the user from having to add hacky code, not force them to do so. There is already at least one more person pointing out this problem (#3202), on the other hand how many people have been requested the behavior to become like this?

YoshiRulz commented 1 year ago

I can't even begin to imagine a hypothetical script that would need to display unchanging stuff

Rando item tracker for one.

When you say "this change was introduced just to save CPU usage", I assume you're talking about https://github.com/TASEmulators/BizHawk/commit/7749d02382d1c9e682cbd28ff3dd3240e5b91227? It wasn't, it was necessitated by something else. I can't remember exactly what, but it was probably to fix #2571, which was a regression introduced by another necessary change.

CasualPokePlayer commented 1 year ago

There is already at least one more person pointing out this problem (#3202)

That other person is pointing out that disabling a lua script didn't clear out the surface. That was fixed. This current issue is about Lua surface not being cleared every single frame. Please do not conflate these two different issues.

Because right now the default behavior is really unfriendly and unintuitive, requiring a coder to slap a custom workaround in every single Lua script they use.

Do you have an example real world lua script that breaks under this change?

requiring a coder to slap a custom workaround in every single Lua script they use. A program is supposed to prevent the user from having to add hacky code, not force them to do so.

To be clear, are you saying adding gui.clearGraphics in your while true loop is a custom hacky workaround?

I can't even begin to imagine a hypothetical script that would need to display unchanging stuff

"Slow to change" stuff also applies.

ThunderAxe31 commented 1 year ago

I can't even begin to imagine a hypothetical script that would need to display unchanging stuff

Rando item tracker for one.

What game? For me, two games come to my mind and they both have a random item tracker that draws changes on every frame (Crash Team Racing and Rolan's Curse)

When you say "this change was introduced just to save CPU usage", I assume you're talking about 7749d02? It wasn't, it was necessetated by something else. I can't remember exactly what, but it was probably to fix #2571, which was a regression introduced by another necessary change.

If the change was introduced to fix something unrelated, then there shouldn't be any problems in reintroduced this specific old behavior.

There is already at least one more person pointing out this problem (#3202)

That other person is pointing out that disabling a lua script didn't clear out the surface. That was fixed. This current issue is about Lua surface not being cleared every single frame.

What makes you so sure that's what that user what talking about? You can "pause", "stop", or "remove" a script. There is no "disable" command.

Because right now the default behavior is really unfriendly and unintuitive, requiring a coder to slap a custom workaround in every single Lua script they use.

Do you have an example real world lua script that breaks under this change?

ThunderAxe31 commented 1 year ago

To be clear, are you saying adding gui.clearGraphics in your while true loop is a custom hacky workaround?

Yes, because it basically needs to be added in every single script that draws on screen.

But more importantly, where should I put it exactly? If I put that line before emu.frameadvance(), if effectively deletes any drawin before are even displayed. If I put it after that, there will be no difference at all (drawings will remain on screen after stopping or pausing the script).

I can't even begin to imagine a hypothetical script that would need to display unchanging stuff

"Slow to change" stuff also applies.

Which would get even more hacky, because it would require to code some extra logic for detecting when the script is drawing something new or not.

YoshiRulz commented 1 year ago

Do you have an example real world lua script that breaks under this change?

I literally mentioned them in my comment, but have here some actual files:

Grabbing some of those scripts at random, you didn't use DrawNew/DrawFinish. If you had, you would have gotten warnings when updating to 2.6, and eventually you would have figured out that you needed to replace gui.DrawNew("emu") with gui.clearGraphics("emu") (or gui.clearGraphics() as I believe there's an implicit gui.use_surface("emu")). As for clearing drawings when removing the script, see https://github.com/TASEmulators/BizHawk/issues/3014#issuecomment-1335385342.

I can't even begin to imagine a hypothetical script that would need to display unchanging stuff

"Slow to change" stuff also applies.

Which would get even more hacky, because it would require to code some extra logic for detecting when the script is drawing something new or not.

EmuHawk shouldn't need to record draw calls and detect changes or anything like that, because a script that's not constantly clearing the canvas only needs to make draw calls when something changes, drawing over the top. Not that it matters because, again, that feature doesn't work.

CasualPokePlayer commented 1 year ago

What makes you so sure that's what that user what talking about? You can "pause", "stop", or "remove" a script. There is no "remove" command.

I apologize, reading further the user does explicitly say "disable" rather than "remove" and the fix was when removing a script. Even so, stopping a lua should itself not cause constant clearing, otherwise someone using registered memory callbacks (which might not occur every frame) to draw will find their script's drawings disappear. Note registered callbacks may occur after the lua script has "stopped" (or moreso reached the end, which is regarded the same way internally). This style is more prominant for other emulators with lua support to note. Although perhaps then pausing should clear them out? Granted that would be Yoshi's call here.

To be clear, are you saying adding gui.clearGraphics in your while true loop is a custom hacky workaround?

Yes, because it basically needs to be added in every single script that draws on screen.

But more importantly, where should I put it exactly? If I put that line before emu.frameadvance(), if effectively deletes any drawin before are even displayed. If I put it after that, there will be no difference at all (drawings will remain on screen after stopping or pausing the script).

Ok this is describing a different issue to be clear: you want lua stuff to be cleared when the script is paused or stopped. That is different from asking it to be cleared every frame. In the current BizHawk code, you would need to use event.onexit for that.

The example scripts you provided too will work fine under the assumption you are not pausing or stopping, which is what I assumed, as this issue and repro script did not specify any semantics regarding pausing or stopping.

Which would get even more hacky, because it would require to code some extra logic for detecting when the script is drawing something new or not.

It's not particularly hacky to do so, but this "hacky" code is needed if you want to lazy draw again due to #2501. If you don't want to lazy draw I will clarify, you do not have to do this, just draw every frame.

vadosnaprimer commented 1 year ago

I guess I'll have to repeat my questions.

What if I need some graphics to go away (until some variable is true again) and other graphics to stay?

How many people have been requesting for graphics to stay on the new frames without calling draw functions?

CasualPokePlayer commented 1 year ago

What if I need some graphics to go away (until some variable is true again) and other graphics to stay?

2501 prevents that from working.

How many people have been requesting for graphics to stay on the new frames without calling draw functions?

Dunno, this is just what the current code seems to be intending.

vadosnaprimer commented 1 year ago

Doesn't really sound like a feature then if it breaks every script with stuff it only draws on certain frames, and some stuff always.

vadosnaprimer commented 1 year ago

Since this behavior was not requested by actual users, and there's no clean way to make existing scripts work with a simple code tweak, this is a bug.

CasualPokePlayer commented 1 year ago

To be clear, existing scripts work fine themselves (assuming they are actually running). The actual complaint is that the lua surface isn't cleared when you pause or stop a script. A one-liner can be added to clear it out on stop event.onexit(function() gui.clearGraphics() end) (can be slapped on top of the script). There isn't any way to do it on pause (outside of manually doing so in the lua console, but that's not really a solution).

Of course, this issue has stated that a simple 1 liner would be too much as it's too much to ask for "every script to be changed" in that way, so a "simple code tweak" is not possible as any code tweak is too much according to this issue.

However, note Yoshi's reply in the linked issue in the OP (bold set by me)

Under the assumption that multiple scripts should be able to draw at once (#2600), I thought about when autoclearing would be necessary and when it would be annoying, and came to the conclusion that removing a script should trigger a clear, but not disabling/stopping a script.

vadosnaprimer commented 1 year ago

My Gargoyles (Genesis) script shipped with hawk is an example of a broken script. It's meant to only display level background during the level (because there's no useful data to display otherwise), but it now gets stuck during all the non-level time, together with hitboxes. Just run the script and let the game play the demos.

If there's a way to clear background and hitboxes while still having the rest of the HUD, it would at least be better than having a broken script. Adding code that restores usual behavior would be a workaround, and killing all drawing when some drawing is not needed would be a hack. There's definitely some ground for a compromise here, depending on what is feasible.

The actual complaint is that the lua surface isn't cleared when you pause or stop a script.

No? The complain is that the lua surface isn't updated unless you draw again, with a code that makes it easy to show the problem.

CasualPokePlayer commented 1 year ago

No? The complain is that the lua surface isn't updated unless you draw again, with a code that makes it easy to show the problem.

The given scripts later given as examples seem to operate correctly (they draw every frame, and doing a draw operation clears out the entire surface currently as trying to do so otherwise is buggy, again by #2501), with then the complaint being they aren't cleared when pausing/stopping the script (quoting ThunderAxe: If I put it after that, there will be no difference at all (drawings will remain on screen after stopping or pausing the script).). The example script given in the OP is a different case (which is probably where the confusion came from). That itself can be solved by simply putting gui.clearGraphics() after emu.frameadvance(), like I demostrated in my first post on this issue.

Of course, then the previous point applies of even adding 1 line of code is too much. So a "compromise" here it would seem will require that the lua script need not to add any code to restore the previous behavior.

vadosnaprimer commented 1 year ago

Oh I missed this bit! Putting it before stuff is drawn for the frame or after frameadvance restores old behavior, so the workaround does work already, even for my script. If it can't really happen automatically anymore, it's a shame, but I'm struggling to call it a feature. I'd rather it be a lua console option.

ThunderAxe31 commented 1 year ago

Grabbing some of those scripts at random, you didn't use DrawNew/DrawFinish. If you had, you would have gotten warnings when updating to 2.6, and eventually you would have figured out that you needed to replace gui.DrawNew("emu") with gui.clearGraphics("emu") (or gui.clearGraphics() as I believe there's an implicit gui.use_surface("emu")). As for clearing drawings when removing the script, see #3014 (comment).

I didn't know about the existence of DrawNew/DrawFinish until now, and now that I know I still don't feel the need to use them, even for more complex scripts. They look like functions that unnecessarily make things more complicated.

I can't even begin to imagine a hypothetical script that would need to display unchanging stuff

"Slow to change" stuff also applies.

Which would get even more hacky, because it would require to code some extra logic for detecting when the script is drawing something new or not.

EmuHawk shouldn't need to record draw calls and detect changes or anything like that, because a script that's not constantly clearing the canvas only needs to make draw calls when something changes, drawing over the top. Not that it matters because, again, that feature doesn't work.

I was referring to hacky code to add in a Lua script as a workaround to this unwanted behavior. I wasn't referring to the emulator's code.

The actual complaint is that the lua surface isn't cleared when you pause or stop a script.

My complaint is about both scenarios, because they're both consequences of this new behavior introduced in 2.6.

DerpPrincess commented 1 year ago

Hi,

Ignoring my ignorance concerning optimizing CPU/GPU for an emulator aside...

Assuming I understand the issue correctly, I believe it'd be great to have the ability to let users decide how they want to show the gui on screen. If possible, I suggest a layering approach so the canvas can have layers, one that is persistent and one that is to be redrawn every frame. If that's not possible, then implementing a lua function that can set their intended behavior, false by default, gui.setPersistentDrawing(boolean).

It largely makes not much sense that anything gui related stays on screen if the script closes or crashes or whatever... Bizhawk should clear it if not running a script. If they only want a one and done thing without a loop so that Bizhawk doesn't have to use the CPU / GPU to draw it again, then gui.setPersistentDrawing(true) on that script for that layer, or some functionality similar. Current if you draw outside of the screen like gui.pixelText(50, 50, "TEST"); like above then if you draw in the loop it'll clear it and only draw what is in the loop afterwards .

Concerning drawing outside of the loop and then when the loop happens... it should draw over the previous layer on the canvas if there is only one layer (as is now). It shouldn't just... stay there. Though the intended idea if it WOULD stay there (as opposed to now) where it could draw a logo (a one and done thing). ALTHOUGH this is only with the current functionality of one singular canvas. But if it was changed to support layering or if you had a way to set that that drawing is persistent, it'd make everyone happy. Let people choose how they want to draw stuff correct?

So we could do:

Drawing outside of the loop stays forever (good for logos or text we'll only use once like game name) and anything inside the loop is just redrawn each frame.
Give a layering approach so that someone stays persistent on one layer and constantly updates on another layer. So one canvas, two layers.
Regardless of the above two, when a script crashes or the user closes it... it shouldn't still be drawing on the screen. Bizhawk should check to see if that particular script is still enabled in the lua console (even if it is finished) and clear the screen.

Thanks

YoshiRulz commented 1 year ago

If possible, I suggest a layering approach so the canvas can have layers [...] 2. Give a layering approach so that someone stays persistent on one layer and constantly updates on another layer. So one canvas, two layers.

Duplicate of #1322.

If that's not possible, then implementing a lua function that can set their intended behavior, false by default, gui.setPersistentDrawing(boolean).

You're describing the bug #2501 we've been talking about.

It largely makes not much sense that anything gui related stays on screen if the script closes or crashes or whatever [...] 3. Regardless of the above two, when a script crashes or the user closes it... it shouldn't still be drawing on the screen. Bizhawk should check to see if that particular script is still enabled in the lua console (even if it is finished) and clear the screen.

This will be fixed in 2.9.

Current if you draw outside of the screen like gui.pixelText(50, 50, "TEST"); like above then if you draw in the loop it'll clear it and only draw what is in the loop afterwards

That's an interesting new angle, how should draw calls from the console be treated? I haven't thought about it.

Drawing outside of the loop stays forever (good for logos or text we'll only use once like game name) and anything inside the loop is just redrawn each frame.

There's no way to differentiate these calls.

I think I've addressed all of that, let me know if anything's still unclear. And do leave a :+1: on #1322 so we can gauge interest.

TASEmulators / BizHawk

Lua drawing surface isn't updated unless you draw again #3474

2501 prevents that from working.