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 #8217

Closed
wants to merge 30 commits into from

Conversation

techgeeknz
Copy link
Contributor

@techgeeknz techgeeknz commented Jun 11, 2020

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.

Although the code compiles and the game seems to work properly, please consider this a work in progess; as there were a lot of commits I had to find and cherry-pick, I cannot be sure that I got them all, and I had to revert my recent patch to master to get it to work. Extensive review, testing, and feedback is recommended.

Once we are sure this is an accurate representation of JGR's changes, I can use it as a base for further work in my quest of decoupling the user interface from the simulation engine.

src/stdafx.h Outdated Show resolved Hide resolved
src/stdafx.h Outdated Show resolved Hide resolved
src/stdafx.h Outdated Show resolved Hide resolved
@techgeeknz
Copy link
Contributor Author

techgeeknz commented Jun 12, 2020

(resolved by 4455011)

Ugh!

What is a munmap_chunk, and why is it getting an invalid pointer?!

@techgeeknz
Copy link
Contributor Author

techgeeknz commented Jun 12, 2020

(resolved by 4455011)

*** Error in `./openttd': double free or corruption (out): 0x00005560ee826ec0 ***

Clearly, there is an errant pointer in that patch that is busy stomping all over memory. Locating that ought to keep me busy for a while %-(

@techgeeknz
Copy link
Contributor Author

I do not understand why the automated checks are failing; as far as I'm aware, the parts of the code it is complaining about were not touched by the patch?

@JGRennison
Copy link
Contributor

You'll need this commit or something like it as well: JGRennison/OpenTTD-patches@4bad77c

You can't use bare malloc/free for structures with non-POD fields like std::vector

@JGRennison
Copy link
Contributor

You'll need this commit as well, as otherwise you'll run off the end of the viewport dirty block buffer, probably causing the issue you noticed, JGRennison/OpenTTD-patches@7f32bb3

@techgeeknz
Copy link
Contributor Author

Thanks. I knew there had to be something I missed. Frankly, I'm surprised the game managed to run as long as it did without crashing.

@techgeeknz
Copy link
Contributor Author

I am quite liking your approach to the problem, @JGRennison. You're quite right that the majority of the widget drawing cannot benefit from a complicated grid-based dirty tracking scheme; as most of them are simple, non-overlapping rectangles anyway. And tracking each viewport independently is also rather genius, not least of all because it gives each widget the ability to manage its own invalidation.

I can't wait to see how I can build on top of this solid foundation 😎

@michicc
Copy link
Member

michicc commented Jun 12, 2020

Short housekeeping note: Consider converting this PR to draft status as long as you wouldn't consider merging it.

@techgeeknz
Copy link
Contributor Author

Short housekeeping note: Consider converting this PR to draft status as long as you wouldn't consider merging it.

Good idea. How do I do that?

@techgeeknz techgeeknz marked this pull request as draft June 12, 2020 21:12
@techgeeknz techgeeknz changed the title WIP: Prepare: Port GUI rendering improvements from JGRPP Port GUI rendering improvements from JGRPP Jun 13, 2020
@techgeeknz techgeeknz marked this pull request as ready for review June 13, 2020 00:57
@techgeeknz
Copy link
Contributor Author

I consider that this is ready to be merged now. Granted, it still has the patch that renames SetDirtyBlocks to AddDirtyBlock reverted, as it conflicts with the similarly-named AddDirtyBlocks from JGRPP; however, I will likely be renaming all of those within the first patch I make on top of this; and I don't want to create unnecessary work for myself or others when the master with this patch is merged back into JGRPP.

src/viewport.cpp Outdated Show resolved Hide resolved
@techgeeknz techgeeknz marked this pull request as draft June 14, 2020 18:00
@techgeeknz techgeeknz force-pushed the jgr_master_gui branch 2 times, most recently from 152de98 to 0fde2fa Compare June 17, 2020 02:13
@techgeeknz techgeeknz marked this pull request as ready for review June 26, 2020 01:39
@techgeeknz
Copy link
Contributor Author

Rebased to resolve the merge-conflict with src/stdafx.h

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.

There are also a few strings that spell VIEWPORT as VIEW_PORT. I have not touched those, as I do not know the consequences of doing so.

@@ -361,6 +361,30 @@ static inline int DivAwayFromZero(int a, uint b)
}
}

/**
* Computes a / b rounded towards negative infinity for signed a and unsigned b.
Copy link
Member

Choose a reason for hiding this comment

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

Documentation says "signed a and unsigned b", but function just takes the same type. Could be really fancy and use https://en.cppreference.com/w/cpp/types/is_unsigned with an assert_compile if you like ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know, cppreference.com has become my new favorite website over the last few weeks? It's brilliant!

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 decided to go for the best of both worlds; an assert_compile to guarantee we're dealing with arithmetic types, and a runtime assert if unsigned b turns out to be signed after all. If I can make the compiler spit out a warning that b should be an unsigned type, then that's all the better 😉

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 also threw in a static_cast to the type of a, since that seems to be the in thing to do with these particular functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "signed a and unsigned b" it should probably say something like "positive values of b" or "b > 0".
T does not strictly have to be signed for the function to return correct results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the other related functions use an unsigned type for the divisor; and I have rigged the function to work either way; if it's fed s signed divisor, you'll get a run-time assertion, else a compile-time assertion.

For that matter, it will work with an unsigned dividend, too.

Copy link
Contributor Author

@techgeeknz techgeeknz Jul 1, 2020

Choose a reason for hiding this comment

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

Is there any reason why these functions actually need to be templated? I have been toying with templating all the other related functions; and there's a lot of work to get right. I actually got my abomination to compile; but there's no way to guarantee the resulting program is correct (i.e. choosing generic types of an appropriate width). For instance; if supplied with an rvalue of (byte_x * 100), will the templated type be 8 bits, 16 bits, signed, or unsigned?

@LordAro
Copy link
Member

LordAro commented Jun 28, 2020

Needs a rebase :)
Couple of commit message thoughts - Cleanup is overused. IMO:

Cleanup: Improve documentation of dirty block system. -> Doc
Cleanup: Rename AddDirtyBlocks reflective of internal use. -> Codechange
Cleanup: Further documentation improvements for dirty block system -> Doc
Cleanup: Spell 'Viewport' consistently -> Codechange

@techgeeknz
Copy link
Contributor Author

Needs a rebase :)
Couple of commit message thoughts - Cleanup is overused. IMO:

Thanks for the feedback. I'll try to add some spice to my commit message.

And here I was thinking that the mass renaming of ViewPort would be the most controversial change 😎

@techgeeknz
Copy link
Contributor Author

I have also become aware that some of my pull requests can have a tendency to veer off topic. I sometimes have a hard time recognising where a commit belongs; so if that happens, please let me know and I will happily split them off into a separate topic branch.

JGRennison and others added 26 commits July 19, 2020 19:41
…g window list.

This is to speed up marking all viewports dirty, which is done very
frequently.
…acked for later redrawing

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.
Partially re-applies 8652a4d, which
was reverted to allow commits to be cherry-picked from JGRPP.
AddDirtyBlocks is an internal implementation detail of SetDirtyBlocks,
and its name should reflect that.
The viewport cache is transparently managed by the ViewportData struct,
and memory associated with the viewport and cache is automatically
managed using smart pointers.
Some places in the codebase misspell 'Viewport' as 'ViewPort' or 'view_port'.
This patch makes everything consistent.
Since they are only ever called with an int and a (int)uint; the
templating seems completely unnecessary and redundant.
@TrueBrain
Copy link
Member

As I understood from #8279, this can be closed at #8279 supersedes this.

@TrueBrain TrueBrain closed this Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants