[BUG] Selected redrawn after backskip | Skill panel sound muted post-fix

[WillLem]

Copy-pasted from Discord

Noticed a bug in NeoLemmix ages ago but never mentioned it because it seemed so minor and likely only affected me because of the custom panel graphics I use
But now that I know a bit more about how NL works, I wonder if I might be able to help fix it
Basically, if you swap the current “selected button” icon graphic with a paint-bucked-filled semitransparent one, and then hit backwards frame step a bunch of times, the graphic will load again and again, making the pause button appear darker and darker
I’m guessing this might have something to do with Delphi’s need to “free” the graphic…
Because a backwards framestep, as I understand it, loads the replay and plays it up until 1 frame before the current one, then pauses the game
If this is done repeatedly, then the “select button” graphic is getting repeatedly loaded over the pause button
If I have that right, I’ll try to fix it myself and the code can be backported to NeoLemmix

It won't be to do with freeing. It'll be that the selected graphic is redrawn without redrawing the unselected button first (which doesn't cause any issues unless semitransparency is involved, hence why it would have gone unnoticed)
It very likely isn't loading it again either; just drawing it again

AFAIK the pause button is the only one for which this happens, and then only when backwards framestepping. So, it should be easy enough to isolate the code and find a fix.

It may be that the full "redraw original button" is only triggered when unpausing specifically
But for some reason, redrawing the frame is called after a backwards skip

@WillLem adding to what Namida said: in pretty much every 2D graphics libraries I've used, whenever you draw something, it just plops it right on top of what's already there. Thus, the "safe" thing to do is clear the screen and redraw it each frame. But: you can be a bit more efficient if you only update the parts you actually need to change.
Typical NeoLemmix graphics really only use 0 and 255 alpha: pixels are either transparent or they're not, no semitransparency involved. In that case, you plop it on top of whatever was there before, and it looks the same as it would have if you'd actually cleaned up and cleared the screen first (well, at least for everything in that area)
Under these conditions, if you draw the same thing on top of itself, nothing really happens so it goes unnoticed. A little wasteful, but whatever, and hey, it's better than clearing everything and redrawing unnecessarily.
But once you add more options for the alpha channel, now the end color for each pixel depends on its starting color, so it changes every time you do a redundant draw call.
Thus, when redrawing a semitransparent object (sometimes it's necessary, maybe something moved) you need to redraw whatever was behind it too (and even a solid background would count as something behind it)
In this case, though, it's probably as Namida said and it's just being drawn unnecessarily many times, in which case dealing with that's the best fix. If for some reason you can't fix that the hacky fix would be to redraw the button before drawing the semitransparent graphic on top of it.

This is probably something I should fix in NL too tbh. Semitransparency is meant to be supported in most places where it would be sane to use it.

[WillLem]

The bug only occurs when "Pause After Backwards Frameskip" is active; this may help to isolate the code which needs to be modified.

OK, GameWindow.pas has the only reference to PauseAfterBackwardsSkip that isn't to do with setting it up as a Config:

Code: [Select]

procedure TGameWindow.GotoSaveState(aTargetIteration: Integer; PauseAfterSkip: Integer = 0; aForceBeforeIteration: Integer = -1);
var
  UseSaveState: Integer;
begin
  if aForceBeforeIteration < 0 then
    aForceBeforeIteration := aTargetIteration;

  CanPlay := False;
  if PauseAfterSkip < 0 then
    GameSpeed := gspNormal
  else if ((aTargetIteration < Game.CurrentIteration) and GameParams.PauseAfterBackwardsSkip)
       or (PauseAfterSkip > 0) then
    GameSpeed := gspPause;
I've added the following lines:

Code: [Select]

procedure TGameWindow.GotoSaveState(aTargetIteration: Integer; PauseAfterSkip: Integer = 0; aForceBeforeIteration: Integer = -1);
var
  UseSaveState: Integer;
  TempBmp: TBitmap32;           <-------------------------------- first added line
begin
  if aForceBeforeIteration < 0 then
    aForceBeforeIteration := aTargetIteration;

  CanPlay := False;
  if PauseAfterSkip < 0 then
    GameSpeed := gspNormal
  else if ((aTargetIteration < Game.CurrentIteration) and GameParams.PauseAfterBackwardsSkip)
       or (PauseAfterSkip > 0) then
    GameSpeed := gspPause;
    TempBmp.DrawTo(fSkillIcons[spbPause]); <-------------------- second added line

I get an error on the second added line: "E1012 Constant expression violates subrange bounds".

Maybe I need to specify what TempBmp is supposed to be calling, i.e. the Pause Button graphic (and the panel button beneath it). Not exactly sure how to do this, I'll look at it again later.

[WillLem]

Special thanks to Dullstar for isolating the code and finding the eventual fix

For those interested, and for namida in order to port the fix to NL, here's what did it:

GameWindow.pas

Code: [Select]

procedure TGameWindow.GotoSaveState(aTargetIteration: Integer; PauseAfterSkip: Integer = 0; aForceBeforeIteration: Integer = -1);
var
  UseSaveState: Integer;
begin
  if aForceBeforeIteration < 0 then
    aForceBeforeIteration := aTargetIteration;

  CanPlay := False;
  if PauseAfterSkip < 0 then
    GameSpeed := gspNormal
  else if ((aTargetIteration < Game.CurrentIteration) and GameParams.PauseAfterBackwardsSkip)
       or (PauseAfterSkip > 0) then
    SkillPanel.DrawButtonSelector(spbPause, False);  <-----------------this line provided the fix
    GameSpeed := gspPause;
In Dullstar's words (from Discord), this is how the fix works:

This will force it draw the button without the highlight (which I believe would necessitate a redraw of the entire button because you can't just undraw the highlight), and then GameSpeed is a property that's set to actually call SetGameSpeed when it's written to, and so SetGameSpeed will draw the highlight on the freshly-redrawn pause button.

[WillLem]

OK... so, it turns out that this fix actually broke "Don't Pause After Backskip" behaviour somehow. So:

1) Go to the Config menu and uncheck "Pause After Backskip"
2) Play a level and perform a backwards framestep. It pauses, and we get the previous bug (i.e. the semitransparent graphic is overlayed again and again with each subsequent skip)

Placing the same line of code outside the block (so, on its own but still within the GoToSaveState procedure) achieves the following:

1) Pause/Don't Pause After Backskip checkbox works again
2) However, the pause button doesn't get highlighted at all with either option (it will still highlight when just pressing the pause button itself)

[namida]

It broke the pause after backwards skip because of the lack of begin / end around the lines you wanted to group together.

Commit 9249ee0 fixes the bug in NeoLemmix. It doesn't touch GameWindow at all; rather, it fixes it in GameBaseSkillPanel via a slight change to how the selector is drawn (in particular, it erases the selector always before drawing it). This means other code can simply continue saying "I want this selector on" or "I want this selector off", without worrying about if it's already on or off.

[WillLem]

Re-fixed in 500e6efe8 using the same code from the NL fix.

Thanks for the explanation namida, helps to know exactly what was going wrong there.

In fairness to Dullstar, his fix has helped me to learn a lot, so it's all good

[WillLem]

The most recent fix causes the skill panel sound (changeop.wav / SFX_SKILLBUTTON) not to cue.

I've looked for a cause and tried a few fixes. The closest I got was that it worked again, but also cued at the beginning of the level:

Code: [Select]

{GameBaseSkillPanel.pas around line 1100}

  RemoveHighlight(aButton);                                   <---------------- this is the current fix for the re-draw bug
  if Highlight then
  DrawHighlight(aButton);
end;

procedure TBaseSkillPanel.DrawHighlight(aButton: TSkillPanelButton);
var
  BorderRect: TRect;
begin
  if aButton <= LAST_SKILL_BUTTON then // we don't want to memorize this for eg. fast forward
  begin
    BorderRect := fButtonRects[aButton];
    fHighlitSkill := aButton;
    //if (fLastHighlitSkill <> spbNone) and (fLastHighlitSkill <> fHighlitSkill) then   <------------ commenting this out brings the sound back, but also cues it at the start of the level
    SoundManager.PlaySound(SFX_SKILLBUTTON);
  end else
  BorderRect := fButtonRects[aButton];

One suggestion might be to cue it from somewhere other than the DrawHighlight procedure, but I haven't got as far as figuring out where yet.

[WillLem]

Fixed in Commit f3c21c703