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

WIP: Refactor gfx engine's dirty block system #8214

Closed

Conversation

@techgeeknz
Copy link
Contributor

@techgeeknz techgeeknz commented Jun 10, 2020

This is a work in progress aimed at increasing the efficiency of the gfx engine and preparing it for decoupling the GUI from the video driver. I am not seeking a merge with mainline at this point, but would welcome code reviews, suggestions, feedback, and testing in a wide range of scenarios.

This is not the main branch in which I am developing the functionality; so I will avoid force-pushing to this branch if at all possible; in case anyone wants to fork it and base further work upon it.

@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Jun 10, 2020

It may be worth doing some performance measurements with these changes.
I would expect that the sub-block mechanism that you've added could be potentially problematic, as it would largely inhibit coalescing of adjacent dirty areas.

Redrawing a block has significant overhead, in part because there is a large margin around the redraw area which also needs to be scanned/redrawn. Making blocks arbitrarily small, such that a greater number of block redraw operations need to occur is not necessarily good for performance.
It's also worth bearing in mind that because the map axes are not aligned to the screen axes, unpadded redraw areas do not tend to coalesce well.

For what it's worth I've got many changes in this area in my branch which might be of interest.

@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 10, 2020

Hello JGR. Thanks for the feedback.

I would expect that the sub-block mechanism that you've added could be potentially problematic, as it would largely inhibit coalescing of adjacent dirty areas.

The sub-blocks are coalesced during both the adding and drawing process, but it would be simple to change the latter to coalesce adjacent blocks that don't include neighbouring sub-blocks. I had intended to simply change the bitmap to 1-bit per block; but then I thought, "why not make better use of the bits that are already there?"

Redrawing a block has significant overhead, in part because there is a large margin around the redraw area which also needs to be scanned/redrawn. Making blocks arbitrarily small, such that a greater number of block redraw operations need to occur is not necessarily good for performance.
It's also worth bearing in mind that because the map axes are not aligned to the screen axes, unpadded redraw areas do not tend to coalesce well.

I have not looked too deeply into the viewport invalidating/drawing code yet; however, I would expect that the coalescing operation within AddDirtyBlock will coalesce these into a (nearly) fully dirty block, which should then further coalesce into a larger dirty rectangle in DrawDirtyBlocks.

It is worth noting that I am not married to the sub-block idea; I do think it is worth discussing, to see what merits it may have, though.

For what it's worth I've got many changes in this area in my branch which might be of interest.

Which branch should I look at?

@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 10, 2020

The direction I want to head into next is to make each window responsible for determining which areas of itself are invalidated by a resize operation, so that only those areas are actually redrawn. This should allow us to perform tricks such as resizing viewports (or even the entire screen) in a performant manner; without having to invalidated literally everything every single time.

@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Jun 10, 2020

The various changes in this area in my repo are not isolated on their branch unfortunately, maintaining a large number of active branches has become a bit cumbersome.

Key commits are:
Summary/docs: JGRennison/OpenTTD-patches@749b9da
Main commit: JGRennison/OpenTTD-patches@8f44250
Subsequent bugfixes: JGRennison/OpenTTD-patches@c227d30 JGRennison/OpenTTD-patches@0a6a388 JGRennison/OpenTTD-patches@5ac2401

I moved to 1 bit per block, but I changed the block grid to be associated with an individual viewport instead of the screen, and changed the block size to vary with the zoom level and where useful be horizontally aligned with the tile grid. I found that making the block size too small for the zoom level made the performance worse.
Non-viewport windows/widgets are tracked separately as they do not benefit from using a block grid.

This doesn't include much on resizing as it doesn't happen very often, and for most windows other than viewports resizing implies a complete re-draw anyway. Other than the above I moved more in the direction of optimising scrolling and some aspects of rendering/drawing.

@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 10, 2020

The various changes in this area in my repo are not isolated on their branch unfortunately, maintaining a large number of active branches has become a bit cumbersome.

I can see how that would be a (maintenance) problem. If you'd like to try splitting each patch in JGRPP into its own branch, I may be able to help with that. There's a lot of features in JGRPP that probably should be in mainline anyway.

I moved to 1 bit per block, but I changed the block grid to be associated with an individual viewport instead of the screen, and changed the block size to vary with the zoom level and where useful be horizontally aligned with the tile grid. I found that making the block size too small for the zoom level made the performance worse.

That is an interesting system; I'll look into it. It may very well be beneficial for viewport dirtiness to be tracked independently of other widgets; especially if each window/widget were to become responsible for invalidating itself when moving/scrolling/resizing.

While it is not an end goal in itself, I am more than willing to implement window compositing if it aids the process of restoring responsiveness to the GUI. If that happens, it would certainly make sense for each window's dirtiness to be tracked independently.

This doesn't include much on resizing as it doesn't happen very often, and for most windows other than viewports resizing implies a complete re-draw anyway. Other than the above I moved more in the direction of optimising scrolling and some aspects of rendering/drawing.

I agree that, on the whole, resizing doesn't happen often. But, when it does, the user experience can be quite painful.

@techgeeknz techgeeknz force-pushed the master_responsive_gui_wip branch 2 times, most recently from 45d2347 to afc1255 Jun 10, 2020
techgeeknz added 7 commits Jun 10, 2020
This is to increase code readability and improve consistency.
`ScreenSizeChanged` will return with an invalid state if it is not
followed by a call to `MarkWholeScreenDirty`. This patch fixes that
issue and (eventually) opens the possibility for video drivers to
dynamically resize the screen without forcing a complete redraw.
This is the first step towards making these functions readable and
refactoring them to use a 1-bit-per-block dirty block bitmap instead
of the current 8-bit-per-block bitmap.
The purpose of this refactoring is to increase the granularity with
which dirty blocks are tracked, while simultaneously improving the
documentation of the algorithm that is used.

This should decrease the size of the areas that are unneccessarily
redrawn, while still allowing small locallised changes to be
coalesced for efficient redrawing.
@techgeeknz techgeeknz force-pushed the master_responsive_gui_wip branch from afc1255 to 7cf893a Jun 10, 2020
@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 10, 2020

Unfortunately, I had to do a force push because, apparently, the commit checker here doesn't seem to like "&" in a commit title?

@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 10, 2020

I've added a patch to DrawDirtyBlocks to make it frame each area that is redrawn. This should alleviate any concerns about sub-blocks and blocks not being coalesced efficiently.

@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 11, 2020

Key commits are:
Summary/docs: JGRennison/OpenTTD-patches@749b9da
Main commit: JGRennison/OpenTTD-patches@8f44250
Subsequent bugfixes: JGRennison/OpenTTD-patches@c227d30 JGRennison/OpenTTD-patches@0a6a388 JGRennison/OpenTTD-patches@5ac2401

@JGRennison, you're amazing. Those patches look like they do a lot of the things I wish to have done; and should make an ideal base for my own work.

Visualising the repainted screen areas highlighted another inefficiency in the current system that yours might not have (if I correctly understand the purpose of UnsetDirtyBlocks) -- objects moving in a viewport that is hidden by another window will cause unnecessary (partial) repaints of that window. There are at least three ways I can think to solve this:

  • Change MarkViewportDirty to filter out blocks that are hidden by windows higher in the z-index. This would be an invasive change, and might not even work if the viewport is not currently associated with a window.
  • Track the viewport dirty state separately and somehow make the window owning the viewport invalidate the correct parts. This is possibly already implemented by your patch.
  • Bite the bullet and just implement a compositing window manager.

@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Jun 11, 2020

Visualising the repainted screen areas highlighted another inefficiency in the current system that yours might not have (if I correctly understand the purpose of UnsetDirtyBlocks) -- objects moving in a viewport that is hidden by another window will cause unnecessary (partial) repaints of that window.

  • Track the viewport dirty state separately and somehow make the window owning the viewport invalidate the correct parts. This is possibly already implemented by your patch.

That's basically what I've done.
MarkViewportDirty is changed to set the viewport dirty state directly and doesn't go through SetDirtyBlocks.

Broadly the procedure I've used in DrawDirtyBlocks is:

  1. If the whole screen is dirty flag is set, just draw the whole screen
  2. For each window:
    2.1. If the whole window is marked dirty draw it using DrawOverlappedWindow, and subtract the drawn area using UnsetDirtyBlocks
    2.2. Otherwise, if any window widgets are marked dirty, draw the widgets using DrawOverlappedWindow, and subtract the drawn area using UnsetDirtyBlocks
    2.3. If the window has a viewport and it wasn't drawn in 2.1 or 2.2, draw any per-viewport dirty blocks which don't overlap occluding windows or screen dirty blocks, using DrawDirtyViewport
  3. Draw any screen dirty blocks using RedrawScreenRect
  4. Draw any pending screen dirty blocks using RedrawScreenRect (e.g. where the window re-inited from within OnPaint)

SetDirtyBlocks is only needed for the cases where the window layout may change to reveal a new area that needs to re redrawn in other windows, e.g. window resize, move, close, shade.

Bite the bullet and just implement a compositing window manager.

For the most part drawing of separate windows and viewports still needs to be done by the same thread which limits the potential benefit. Fully redrawing windows when they are partially occluded may not be ideal either.

@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 11, 2020

I am in the middle of rebasing your patches onto master. I've had to locate and merge in several other dependency commits, but I think I've just about got it to compile. When it's done, I'll post a link to the branch so it can be reviewed for anything else that might be missing.

Edit: The rebase is complete. Please review it here: #8217

@techgeeknz techgeeknz marked this pull request as draft Jun 12, 2020
@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 28, 2020

I am closing this pull request now, as I intend to base future work upon #8217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants