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

Draft
wants to merge 29 commits into
base: master
from

Conversation

@techgeeknz
Copy link
Contributor

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.

@techgeeknz techgeeknz force-pushed the techgeeknz:jgr_master_gui branch from 8eefbf1 to 7058896 Jun 11, 2020
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

techgeeknz commented Jun 12, 2020

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

JGRennison commented Jun 12, 2020

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

JGRennison commented Jun 12, 2020

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

techgeeknz commented Jun 12, 2020

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

techgeeknz commented Jun 12, 2020

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

techgeeknz commented Jun 12, 2020

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 Jun 12, 2020
@techgeeknz techgeeknz force-pushed the techgeeknz:jgr_master_gui branch from e4c6061 to 4455011 Jun 12, 2020
@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 Jun 13, 2020
@techgeeknz
Copy link
Contributor Author

techgeeknz commented Jun 13, 2020

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 Jun 14, 2020
@techgeeknz techgeeknz force-pushed the techgeeknz:jgr_master_gui branch 2 times, most recently from 152de98 to 0fde2fa Jun 16, 2020
@techgeeknz techgeeknz marked this pull request as ready for review Jun 26, 2020
@techgeeknz techgeeknz force-pushed the techgeeknz:jgr_master_gui branch from b078c96 to 44020be Jun 26, 2020
@techgeeknz
Copy link
Contributor Author

techgeeknz commented Jun 26, 2020

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

Copy link
Contributor Author

techgeeknz left a comment

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.

This comment has been minimized.

Copy link
@LordAro

LordAro Jun 28, 2020

Member

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 ;)

This comment has been minimized.

Copy link
@techgeeknz

techgeeknz Jun 28, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@techgeeknz

techgeeknz Jun 29, 2020

Author Contributor

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 😉

This comment has been minimized.

Copy link
@techgeeknz

techgeeknz Jun 29, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@JGRennison

JGRennison Jun 30, 2020

Contributor

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.

This comment has been minimized.

Copy link
@techgeeknz

techgeeknz Jun 30, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@techgeeknz

techgeeknz Jul 1, 2020

Author Contributor

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

techgeeknz commented Jun 28, 2020

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

techgeeknz commented Jun 28, 2020

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 25 commits Sep 9, 2016
…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.
std::function<void(Window const * const, int, int, int, int)> draw_overlapped;
draw_overlapped = [&w, &gfx_dirty, &draw_overlapped](Window const * const w_from, int left, int top, int right, int bottom){
Comment on lines +881 to +882

This comment has been minimized.

Copy link
@JGRennison

JGRennison Jul 2, 2020

Contributor

In general I would be wary of this sort of structure.
std::function is really for dynamic dispatch, as it adds significant overhead in code size, run time and memory use.
The call targets are entirely static here, just calling the target function directly as before is fine.

This comment has been minimized.

Copy link
@techgeeknz

techgeeknz Jul 2, 2020

Author Contributor

How else would you do a nested recursive function in C++?
The entire point of the closure is to be able to eliminate unnecessary global variables (well, not so much in this case; but in general).

This comment has been minimized.

Copy link
@nielsmh

nielsmh Jul 2, 2020

Contributor

Wrap the relevant code in a class. The class can even be private to the compilation unit and have a simple function as call interface in the header.

This comment has been minimized.

Copy link
@LordAro

LordAro Jul 2, 2020

Member

I'd say that if this much code/overhead is necessary to eliminate a global variable, then the global variable is fine. Hell, I think a goto would be better than this.

This comment has been minimized.

Copy link
@techgeeknz

techgeeknz Jul 3, 2020

Author Contributor

I am with you on this one, @LordAro. Jumping through this many hoops for what should be a basic, straightforward language feature is downright ugly at best. The alternative of passing around a bazillion copies of the parent function's constant parameters on the stack is not much better; and the thought of wrapping the whole thing in a Java-esque class just to avoid duplicating the common values makes me sick.

Why, oh why, doesn't C just support nested functions like any other decent language? It's only a layer of indirection via (a copy of) the parent's base pointer, after all.

True, C++ has closures and lambdas but, as seen above, the current implementation (C++11) still misses the mark for this one incredibly useful feature included in nearly every other language 😞.

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.