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

Port GUI rendering improvements from JGRPP #8279

Closed
wants to merge 13 commits into from

Conversation

techgeeknz
Copy link
Contributor

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.

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/window_gui.h Show resolved Hide resolved
src/viewport_func.h Show resolved Hide resolved
src/gfx.cpp Show resolved Hide resolved
{
if (unlikely(b >= std::numeric_limits<int>::max())) return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@techgeeknz
Copy link
Contributor Author

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

Thanks for the review.

@techgeeknz techgeeknz force-pushed the jgr_master_gui2 branch 2 times, most recently from 4981e53 to f1fc54c Compare July 22, 2020 06:27
* 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +1484 to +1494
extern void ViewportDrawChk(const Viewport *vp, int left, int top, int right, int bottom);
ViewportDrawChk(_dirty_viewport, left, top, right, bottom);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@techgeeknz techgeeknz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@techgeeknz 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
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.
@TrueBrain TrueBrain added the candidate: needs considering This Pull Request needs more opinions label Dec 14, 2020
@TrueBrain TrueBrain added the size: large This Pull Request is large in size; review will take a while label Dec 14, 2020
@LordAro
Copy link
Member

LordAro commented May 13, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

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 techgeeknz closed this May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants