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

clear window line by line, skipping untouched lines #72035

Merged

Conversation

db48x
Copy link
Contributor

@db48x db48x commented Feb 28, 2024

Summary

Bugfixes "fix windows with missing header text"

Purpose of change

Describe the solution

Many of the windows in the game, especially the ones with scrollbars, create multiple overlapping windows and then draw them in an odd order. This results in overdraw, but that was partly compensated for by the cache that we removed. Now that we clear the whole window in one go, this causes artifacts as we erase text that was already drawn in a different window that overlaps the current one. By erasing only the lines that are touched, we avoid the problem.

Fixes #71994.

Describe alternatives you've considered

We could just change every window to avoid the overdraw. It is almost always because it goes back and calls draw_scrollbar to add a scrollbar to the window after it has already drawn the window border and header text. By moving the call to draw_scrollbar up so that it happens before printing the header text, the problem would also be avoided.

Testing

I just manually tried out every window I could find that has a header and a scrollbar, including those mentioned in the bug report.

Many of the windows in the game, especially the ones with scrollbars,
create multiple overlapping windows and then draw them in an odd
order. This results in overdraw, but that was partly compensated for
by the cache that we removed. Now that we clear the whole window in
one go, this causes artifacts as we erase text that was already drawn
in a different window that overlaps the current one. By erasing only
the lines that are touched, we avoid the problem.

Fixes CleverRaven#71994.
@Qrox
Copy link
Contributor

Qrox commented Feb 28, 2024

Many of the windows in the game, especially the ones with scrollbars, create multiple overlapping windows and then draw them in an odd order. This results in overdraw, but that was partly compensated for by the cache that we removed.

Our code has always worked in curses so I think the SDL framebuffer code was simply mimicking curses behavior.

We could just change every window to avoid the overdraw

I think this PR is the correct solution as it avoids inconsistencies between the curses and SDL builds which might make testing harder.

erasing only the lines that are touched

Was this (instead of erasing the cells that are touched) the behavior before the removal? I'm worried that undrawn parts of a line might still be overwritten.

@db48x
Copy link
Contributor Author

db48x commented Feb 28, 2024

Many of the windows in the game, especially the ones with scrollbars, create multiple overlapping windows and then draw them in an odd order. This results in overdraw, but that was partly compensated for by the cache that we removed.

Our code has always worked in curses so I think the SDL framebuffer code was simply mimicking curses behavior.

Very likely.

erasing only the lines that are touched

Was this (instead of erasing the cells that are touched) the behavior before the removal? I'm worried that undrawn parts of a line might still be overwritten.

Yes. Either the line was touched or it was not, and the lines are all filled with spaces when the window is created. Thus the whole line is drawn no matter what.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) labels Feb 28, 2024
src/sdltiles.cpp Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Feb 28, 2024
@katemonster33
Copy link
Contributor

I compiled this locally and it seemed to fix the issue

@Maleclypse Maleclypse merged commit 60757df into CleverRaven:master Feb 28, 2024
26 checks passed
@IdleSol
Copy link
Contributor

IdleSol commented Feb 29, 2024

Good news, everyone!

Game version: cdda-windows-tiles-x64-2024-02-28-2115

The first problem.

1

  1. Default settings
  2. Enable imGui (settings / options / interface / Use imGui UI: true)
  3. Open the keybindings menu
  4. Close the keybindings menu
  5. Look at the bottom of the screen

A similar line appears on any invocation of the keybindings menu. For example:

  1. Start creating a new world
  2. In the difficulty selection menu, press ?
    or
  3. setting /safemode
  4. press a (start creating a new rule )
  5. press enter (set rule name)
  6. press F1 (calling the keybindings menu)

If Use imGui UI: false, there's no problem.

I'm assuming the problem is the size of the keybindings menu. It's longer than it should be.

The second problem.
The keybindings window again (settings / keybindungs). The search string does not become active.
2

To be more precise, it sometimes becomes inactive (does not allow you to enter text). The behavior is as follows. When opening a window, the cursor blinks once and disappears. When pressing keys nothing is typed.

  1. Default settings
  2. Enable imGui (settings / options / interface / Use imGui UI: true)
  3. Open the keybindings menu
  4. Try typing. If you succeed, go to the next step.
  5. Close the game and open it again.
  6. Go to step 3

Alternatively 1.

  1. Open the keybindings menu
  2. Close the keybindings menu
  3. Open another menu (safemode, options or create a world)
  4. Close this menu
  5. Go to step 1

Alternatively 2.
After opening the keybindings menu, press enter. Try typing in the text

If you disable imGui, everything works fine.

P.S. Or was it necessary to create a new topic?

@IdleSol
Copy link
Contributor

IdleSol commented Feb 29, 2024

Hotkey menu for the hotkey menu?

  1. Open the keybindings menu
  2. Press ? - another keybindings menu opens

@db48x
Copy link
Contributor Author

db48x commented Feb 29, 2024

Separate bug reports for separate bugs please!

@IdleSol
Copy link
Contributor

IdleSol commented Feb 29, 2024

@Qrox
Copy link
Contributor

Qrox commented Feb 29, 2024

Hotkey menu for the hotkey menu

You're supposed to be able to change the keybindings of the keybindings menu itself, so it is expected, unless something else is wrong.

@IdleSol
Copy link
Contributor

IdleSol commented Feb 29, 2024

SurFlurer added a commit to SurFlurer/Cataclysm-DDA that referenced this pull request Mar 19, 2024
SurFlurer added a commit to SurFlurer/Cataclysm-DDA that referenced this pull request Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some menus are blank
5 participants