Port GUI rendering improvements from JGRPP #8279

techgeeknz · 2020-07-21T23:44:55Z

This branch aims to take the improved dirty block tracking that @JGRennison developed for JGRPP earlier this year and patch it into master so that everyone can benefit from it.

This is, essentially, a rehash of #8217 addressing many of the issues that were raised there; and is intended to follow up #8260.

The remaining issues raised in #8217 will be addressed in this PR, then the original will be closed.

glx22

Quick analysis of the 1476 Warnings and 587 Errors in the win64 build log.

src/window_gui.h

src/viewport_func.h

src/gfx.cpp

glx22 · 2020-07-22T00:47:36Z

src/core/math_func.hpp

 {
+	if (unlikely(b >= std::numeric_limits<int>::max())) return 0;


warning C4018: '>=': signed/unsigned mismatch (MSVC warning)

glx22 · 2020-07-22T00:47:52Z

src/core/math_func.hpp

 {
-	const int _b = static_cast<int>(b);
+	if (unlikely(b >= std::numeric_limits<int>::max())) return signum(a);


warning C4018: '>=': signed/unsigned mismatch (MSVC warning)

glx22 · 2020-07-22T00:48:15Z

src/core/math_func.hpp

+ */
+static inline int DivTowardsNegativeInf(const int a, const uint b)
+{
+	if (unlikely(b >= std::numeric_limits<int>::max())) return min(0, signum(a));


warning C4018: '>=': signed/unsigned mismatch (MSVC warning)

glx22 · 2020-07-22T00:48:26Z

src/core/math_func.hpp

+ */
+static inline int DivTowardsPositiveInf(const int a, const uint b)
+{
+	if (unlikely(b >= std::numeric_limits<int>::max())) return max(0, signum(a));


warning C4018: '>=': signed/unsigned mismatch (MSVC warning)

What is the compiler actually complaining about here? Should std::numeric_limits<int>::max() be typecast to uint?

techgeeknz · 2020-07-22T02:27:24Z

Quick analysis of the 1476 Warnings and 587 Errors in the win64 build log.

Thanks for the review.

techgeeknz · 2020-07-28T23:32:23Z

src/gfx_func.h

- * This whole mechanism is controlled by an rectangle defined in #_invalid_rect. This
+ * This whole mechanism was controlled by an rectangle defined in #_invalid_rect. This


Once I completely understand how the new system works, this documentation will be rewritten to reflect the current state of affairs.

techgeeknz · 2020-07-28T23:39:50Z

src/gfx.cpp

+		extern void ViewportDrawChk(const Viewport *vp, int left, int top, int right, int bottom);
+		ViewportDrawChk(_dirty_viewport, left, top, right, bottom);


I have a hunch that this may not be strictly necessary; or, at the very least, there may be a better way to refactor it. The public interface appears to be Window::DrawViewport.

src/smallmap_gui.cpp

techgeeknz · 2020-07-29T06:36:33Z

src/widget.cpp

-	int abs_top = w->top + this->pos_y;
-	SetDirtyBlocks(abs_left, abs_top, abs_left + this->current_x, abs_top + this->current_y);
+	this->is_dirty = true;
+	w->flags |= WF_DIRTY;


Do we need to be marking the entire window dirty here, or should it be WF_WIDGETS_DIRTY; or have I misunderstood the situation completely?

techgeeknz · 2020-07-29T06:45:19Z

src/window.cpp

 /**
 * Mark entire window as dirty (in need of re-paint)
 * @ingroup dirty
 */
-void Window::SetDirty() const
+void Window::SetDirtyAsBlocks()


It seems like lots of things around here could use better names; this is probably one of them. I intend to deal with them all in a later commit; once I figure out what those names should be.

techgeeknz

This commit (a66dbcc) was split out from 1646cf3 in order to make the relocation of MayBeShown more obvious.

Fix: Test division denominator against signed limits

This reverts commit 8652a4d. This is necessary to aid in the cherry-picking of commits from JGRPP.

…w list. This is to speed up marking all viewports dirty, which is done very frequently. Fix: Use new/delete for Viewport info struct Fix: Clip dirty region to be within viewport in MarkViewportDirty

Track dirty viewport areas seperately form general screen redraws. Maintain a dirty block grid per viewport, with a smaller block size. Use even smaller block size in viewport map mode. Use a rectangle array for general screen redraws instead of a block grid. Add a dirty bit to windows and widgets, to simplify the common case of repainting a whole window or widget, without catching neighbouring windows or viewports. Fix: Window re-rendering when ReInit() called within OnPaint/draw handler Fix: [Viewport] Updates being applied to shaded windows Fix: Wrong viewport virtual size in InitializeWindowViewport

AddDirtyBlocks is an internal implementation detail of SetDirtyBlocks, and its name should reflect that.

Doc: Improve documentation of dirty block system Partially re-applies 8652a4d, which was reverted to allow commits to be more easily cherry-picked from JGRPP. Doc: Further documentation improvements for dirty block system

DrawOverlappedWindow caches the value of _cur_dpi and then uses it to modify the structure. This commit removes that level of indirection in order to make operations on the global variable more obvious.

LordAro · 2021-05-13T13:10:58Z

This PR is nearly a year out of date at this point, and there have been lots of changes in the general area of rendering in that time. Is there anything in here that's still relevant?

JGRennison · 2021-05-13T16:54:56Z

All of the additional functionality in here is still in my branch, and I at least consider it beneficial.
However, I do not have any recent performance metrics for comparison purposes.

I was not planning on taking over this PR, so if the original author does not want to resurrect it then closing it seems sensible.

techgeeknz · 2021-05-14T03:06:38Z

I was not planning on taking over this PR, so if the original author does not want to resurrect it then closing it seems sensible.

Original author here. While I am still interested in seeing OpenTTD progress forward, there just hasn't been any time for me to work on it since gaining employment. If anybody else wants to take my work and run with it, they are more than welcome to do so. I agree that, at this point, it is likely not worth pursuing this PR (or any others that might be outstanding) in its present state.

techgeeknz force-pushed the jgr_master_gui2 branch from fee5ac9 to 1cde9d6 Compare July 22, 2020 00:13

glx22 reviewed Jul 22, 2020

View reviewed changes

techgeeknz force-pushed the jgr_master_gui2 branch 2 times, most recently from 4981e53 to f1fc54c Compare July 22, 2020 06:27

Add: Use likely/__builtin_expect for assertion macros.

0fcb4e4

techgeeknz force-pushed the jgr_master_gui2 branch from f1fc54c to ed34f55 Compare July 28, 2020 22:59

techgeeknz commented Jul 29, 2020

View reviewed changes

techgeeknz force-pushed the jgr_master_gui2 branch from ed34f55 to efef7ab Compare July 29, 2020 06:58

techgeeknz commented Jul 29, 2020

View reviewed changes

techgeeknz force-pushed the jgr_master_gui2 branch 4 times, most recently from 5ba08b2 to af2d5b9 Compare July 30, 2020 02:20

techgeeknz and others added 12 commits July 30, 2020 16:29

Codechange: Document and 'const'-ify functions in math_func.hpp

eae05e9

Fix: Test division denominator against signed limits

Add: Add utility functions for integer division towards +/- infinity

c080954

Revert: "Cleanup: Give SetDirtyBlocks a more descriptive name."

3822ec4

This reverts commit 8652a4d. This is necessary to aid in the cherry-picking of commits from JGRPP.

Add: Cache list of active viewports instead of always iterating windo…

7fd257d

…w list. This is to speed up marking all viewports dirty, which is done very frequently. Fix: Use new/delete for Viewport info struct Fix: Clip dirty region to be within viewport in MarkViewportDirty

Add: Clip widget draw calls which are outside _cur_dpi

e7ba56a

Prepare: Export MayBeShown to window_gui.h

d39c952

Codechange: Rename AddDirtyBlocks reflective of internal use.

f252c15

AddDirtyBlocks is an internal implementation detail of SetDirtyBlocks, and its name should reflect that.

Codechange: Refactor and document dirty blocks system

893c16f

Doc: Improve documentation of dirty block system Partially re-applies 8652a4d, which was reverted to allow commits to be more easily cherry-picked from JGRPP. Doc: Further documentation improvements for dirty block system

Codechange: Refactor and document DrawOverlappedWindow

7e2e5a1

Codechange: Make operations on _cur_dpi explicit

130041a

DrawOverlappedWindow caches the value of _cur_dpi and then uses it to modify the structure. This commit removes that level of indirection in order to make operations on the global variable more obvious.

Remove: Unused import in viewport.cpp

10f4fd1

techgeeknz force-pushed the jgr_master_gui2 branch from af2d5b9 to 10f4fd1 Compare July 30, 2020 05:40

TrueBrain added the candidate: needs considering label Dec 14, 2020

TrueBrain added the size: large label Dec 14, 2020

TrueBrain mentioned this pull request Dec 14, 2020

Port GUI rendering improvements from JGRPP #8217

Closed

techgeeknz closed this May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port GUI rendering improvements from JGRPP #8279

Port GUI rendering improvements from JGRPP #8279

techgeeknz commented Jul 21, 2020

glx22 left a comment

glx22 Jul 22, 2020

glx22 Jul 22, 2020

glx22 Jul 22, 2020

glx22 Jul 22, 2020

techgeeknz Jul 22, 2020

techgeeknz commented Jul 22, 2020

techgeeknz Jul 28, 2020

techgeeknz Jul 28, 2020

techgeeknz Jul 29, 2020

techgeeknz Jul 29, 2020

techgeeknz left a comment •

edited

Loading

LordAro commented May 13, 2021

JGRennison commented May 13, 2021

techgeeknz commented May 14, 2021

		{
		if (unlikely(b >= std::numeric_limits<int>::max())) return 0;

		* This whole mechanism is controlled by an rectangle defined in #_invalid_rect. This
		* This whole mechanism was controlled by an rectangle defined in #_invalid_rect. This

		extern void ViewportDrawChk(const Viewport *vp, int left, int top, int right, int bottom);
		ViewportDrawChk(_dirty_viewport, left, top, right, bottom);

Port GUI rendering improvements from JGRPP #8279

Port GUI rendering improvements from JGRPP #8279

Conversation

techgeeknz commented Jul 21, 2020

glx22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techgeeknz commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techgeeknz left a comment • edited Loading

Choose a reason for hiding this comment

LordAro commented May 13, 2021

JGRennison commented May 13, 2021

techgeeknz commented May 14, 2021

techgeeknz left a comment •

edited

Loading