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

Weird scrolling regression in the GDI renderer #13270

Closed
j4james opened this issue Jun 10, 2022 · 8 comments · Fixed by #13271
Closed

Weird scrolling regression in the GDI renderer #13270

j4james opened this issue Jun 10, 2022 · 8 comments · Fixed by #13271
Assignees
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-1 A description (P1) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Jun 10, 2022

Windows Terminal version

Commit 75e4624

Windows build number

10.0.19044.1706

Other Software

No response

Steps to reproduce

  1. Checkout commit ed27737 or later.
  2. Make sure you've got the GDI renderer enabled in the registry (i.e. UseDx is 0).
  3. Build and run OpenConsole.
  4. Perform a directory listing to make the viewport scroll.
  5. Press enter a few times to scroll some more.

Expected Behavior

Just normal scrolling.

Actual Behavior

Random bits of the screen scrolling independently of each other. Here's an example of the sort of thing I end up with.

image

I'm not sure how I hadn't noticed this before, but I think I've been using the DX renderer and only recently switched back to GDI (that's the only renderer that seems to be affected).

/cc @lhecker I think this might be PR #13025 again.

@j4james j4james added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Jun 10, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jun 10, 2022
@lhecker lhecker self-assigned this Jun 10, 2022
@lhecker
Copy link
Member

lhecker commented Jun 10, 2022

Thanks for finding all these! Also, I have to confess: I think I just realized that despite the large changes to the GDI engine, I'm not sure I ever seriously tested it with UseDx != 2. 😨 I'll try to fix this as soon as I can.

@lhecker
Copy link
Member

lhecker commented Jun 11, 2022

Hmm... weird. I'm unable to reproduce the issue so far. I've tried it at 100%, 150% and 200% zoom scale. I've deleted all keys/values under HKCU:\Console before testing and I'm using a debug build of OpenConsole.

@j4james
Copy link
Collaborator Author

j4james commented Jun 11, 2022

OK, let me try clear out my reg keys and see if I can narrow down a cause. But I can tell you my scale is 200% and I'm using a release build.

@j4james
Copy link
Collaborator Author

j4james commented Jun 11, 2022

I tried deleting everything under HKCU\Console and I'm still seeing the problem. It couldn't be a Windows version thing could it? I'm assuming you're on Windows 11 and I'm still on 10.

But if you can't reproduce it, I'll try and look at the code and see if I can figure out where it's going wrong.

@lhecker
Copy link
Member

lhecker commented Jun 11, 2022

This could also be a difference due to our hardware. I could imagine that my Nvidia GPU might redraw the entire window according to the GDI information it has on every frame, whereas yours might strictly use the given dirty areas instead.

I do have a Windows 10 Hyper-V VM, which seems to reproduce the issue.
I could've sworn that I saw "ghost characters" seldomly while scrolling slowly up and down, but they always disappeared within a split second. This makes it certainly very hard to debug. 😅
(This also explains why I didn't notice it during testing...)

@j4james
Copy link
Collaborator Author

j4james commented Jun 11, 2022

I think I've found the culprit.

RECT rcScrollLimit;
RETURN_IF_FAILED(LongSub(_szMemorySurface.cx, szGutter.cx, &rcScrollLimit.right));
RETURN_IF_FAILED(LongSub(_szMemorySurface.cy, szGutter.cy, &rcScrollLimit.bottom));

That rcScrollLimit used to be initialized as:

RECT rcScrollLimit = { 0 };

But now it's not, so the left and top coordinates can end up with garbage values. I suspect it doesn't affect you because the debug build initializes everything on the stack to zero anyway.

@lhecker
Copy link
Member

lhecker commented Jun 11, 2022

Oh nice find! I actually tried it with a Release build as well...

@lhecker
Copy link
Member

lhecker commented Jun 11, 2022

I'll try to see if I can automate searching the large diff for similar mistakes somehow.
And I'll submit a PR for it as soon as possible. (I'll add you as Co-Authored-By if you don't mind. 🙂)

@lhecker lhecker added Product-Conhost For issues in the Console codebase Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Priority-1 A description (P1) labels Jun 11, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 11, 2022
@ghost ghost added the In-PR This issue has a related PR label Jun 11, 2022
@ghost ghost closed this as completed in #13271 Jun 13, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jun 13, 2022
ghost pushed a commit that referenced this issue Jun 13, 2022
ed27737 contains a regression were a `RECT` in `GdiEngine` wasn't properly
initialized anymore. Due to this, rendering during scrolling behaved erratic.

To find other cases of this bug in ed27737 the following regex was used:
```
^-.* = \{\s*\d*\s*\};
```

It appears that only `GdiEngine` was affected by a bug of this kind,
but just to be sure, this PR reverts all other instances.
This bug was likely caused when I tried to undo some of the changes in
ed27737 to make the PR smaller, but failed to revert the code properly.

## PR Checklist
* [x] Closes #13270
* [x] I work here

## Validation Steps Performed
I'm unable to reproduce the issue on my hardware and am unable to test
this change, but the uninitialized struct is clearly a bug regardless.

Co-authored-by: James Holderness <j4_james@hotmail.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-1 A description (P1) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants