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] Accelerated rendering #1753

Closed
wants to merge 119 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Vultraz
Member

Vultraz commented Jun 4, 2017

WIP work to get the game using textures and/or accelerated rendering. Opening a PR for feedback as I go along. GUI2 almost entirely works except for some alpha issues with the text in the story viewer and the minimap not showing up.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 4, 2017

Member

@aginor I know you've ceased development for the project, I wouldn't mind some general feedback from you as you're good with SDL (if you feel like it, that is).

Member

Vultraz commented Jun 4, 2017

@aginor I know you've ceased development for the project, I wouldn't mind some general feedback from you as you're good with SDL (if you feel like it, that is).

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Jun 4, 2017

Member

Just make sure you don't merge this.

Member

CelticMinstrel commented Jun 4, 2017

Just make sure you don't merge this.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 4, 2017

Member

I am attempting to get this to a mergable state. It is not there yet.

Member

Vultraz commented Jun 4, 2017

I am attempting to get this to a mergable state. It is not there yet.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Jun 4, 2017

Member

There is no mergeable state that will be acceptable for this PR in the near future.

Member

CelticMinstrel commented Jun 4, 2017

There is no mergeable state that will be acceptable for this PR in the near future.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 4, 2017

Member

Well, not exactly true, not if I can solve the black screen issue when gui2 dialogs appear in-game. Remember, I also added compatibility with the software renderer.

Member

Vultraz commented Jun 4, 2017

Well, not exactly true, not if I can solve the black screen issue when gui2 dialogs appear in-game. Remember, I also added compatibility with the software renderer.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Jun 4, 2017

Member

You are still completely missing the point. This is a big change and/or refactor. We are in the beginning of a release cycle. Do you intend to release 1.14 this year or not?

Member

CelticMinstrel commented Jun 4, 2017

You are still completely missing the point. This is a big change and/or refactor. We are in the beginning of a release cycle. Do you intend to release 1.14 this year or not?

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 4, 2017

Member

Of course.

Member

Vultraz commented Jun 4, 2017

Of course.

@aginor

The only bit that actually enables accelerated rendering here is the change to the renderer flags.
You need to test those carefully across multiple platforms, I've had a play with it in the past and it broke things depending on the hardware. There's also no real performance (might actually be the opposite!) as you now need to shuffle a lot more data between the video ram and main ram, which is slower.

Show outdated Hide outdated src/sdl/window.cpp
void draw(
const int canvas_w,
const int canvas_h,
SDL_Renderer* renderer,

This comment has been minimized.

@aginor

aginor Jun 5, 2017

Contributor

Don't pass raw pointers

@aginor

aginor Jun 5, 2017

Contributor

Don't pass raw pointers

void draw(
const int canvas_w,
const int canvas_h,
SDL_Renderer* renderer,

This comment has been minimized.

@aginor

aginor Jun 5, 2017

Contributor

Don't pass raw pointers

@aginor

aginor Jun 5, 2017

Contributor

Don't pass raw pointers

void draw(
const int canvas_w,
const int canvas_h,
SDL_Renderer* renderer,

This comment has been minimized.

@aginor

aginor Jun 5, 2017

Contributor

Don't pass raw pointers

@aginor

aginor Jun 5, 2017

Contributor

Don't pass raw pointers

Show outdated Hide outdated src/gui/core/canvas.hpp
Show outdated Hide outdated src/gui/core/canvas.cpp
@aginor

This comment has been minimized.

Show comment
Hide comment
@aginor

aginor Jun 5, 2017

Contributor

It might also be worth mentioning that I cannot test this branch as it segfaults immediately.

#0  0x0000000001180ea7 in screen_area() ()
#1  0x0000000000dda3cb in gui2::window::update_screen_size() [clone .part.38] ()
#2  0x0000000000d940de in gui2::load_settings() ()
#3  0x0000000000d4e799 in gui2::init() ()
#4  0x0000000000843f08 in main ()
Contributor

aginor commented Jun 5, 2017

It might also be worth mentioning that I cannot test this branch as it segfaults immediately.

#0  0x0000000001180ea7 in screen_area() ()
#1  0x0000000000dda3cb in gui2::window::update_screen_size() [clone .part.38] ()
#2  0x0000000000d940de in gui2::load_settings() ()
#3  0x0000000000d4e799 in gui2::init() ()
#4  0x0000000000843f08 in main ()
@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 5, 2017

Member

Oh dear. That's not good. O_O

Member

Vultraz commented Jun 5, 2017

Oh dear. That's not good. O_O

@aginor

This comment has been minimized.

Show comment
Hide comment
@aginor

aginor Jun 5, 2017

Contributor

I never enabled the framebuffer acceleration flags because I found in my testing that they caused a lot of problems across different platforms (nothing insurmountable), but fixing it all would take a lot of time and testing would be difficult. You will see different behaviours across different operating systems and between different hardware configurations. Most of the difficulty will be in setting it all up properly, once that's done SDL should go and abstract it all away for you.

Contributor

aginor commented Jun 5, 2017

I never enabled the framebuffer acceleration flags because I found in my testing that they caused a lot of problems across different platforms (nothing insurmountable), but fixing it all would take a lot of time and testing would be difficult. You will see different behaviours across different operating systems and between different hardware configurations. Most of the difficulty will be in setting it all up properly, once that's done SDL should go and abstract it all away for you.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 5, 2017

Member

Hm. I'll disable, it, then.

Member

Vultraz commented Jun 5, 2017

Hm. I'll disable, it, then.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 5, 2017

Member

@aginor I've added the wrapper class.

Member

Vultraz commented Jun 5, 2017

@aginor I've added the wrapper class.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Jun 5, 2017

Contributor

You can't just change the copyright from 2003-2017 to simply read 2017.

You're fine with just 2017 on a completely new file or a completely rewritten file. But if you're changing a file all you can do is extend the range to include 2017. The parts you didn't change remain copyright 2003 (or whatever).

Contributor

GregoryLundberg commented Jun 5, 2017

You can't just change the copyright from 2003-2017 to simply read 2017.

You're fine with just 2017 on a completely new file or a completely rewritten file. But if you're changing a file all you can do is extend the range to include 2017. The parts you didn't change remain copyright 2003 (or whatever).

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 5, 2017

Member

@GregoryLundberg it's a new file I wrote that I had just added eaelier where I forgot to remove the '2003' part from the copyright when I copied it (the copyright) from another file.

Member

Vultraz commented Jun 5, 2017

@GregoryLundberg it's a new file I wrote that I had just added eaelier where I forgot to remove the '2003' part from the copyright when I copied it (the copyright) from another file.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 6, 2017

Member

@aginor the startup crash should be fixed.

Member

Vultraz commented Jun 6, 2017

@aginor the startup crash should be fixed.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 8, 2017

Member

So, just want to leave a progress report. As you can see in the above commits, I've made per-frame drawing global and am now working to refactor the main display to properly work with this drawing method. This is what I have so far:
progress2

Member

Vultraz commented Jun 8, 2017

So, just want to leave a progress report. As you can see in the above commits, I've made per-frame drawing global and am now working to refactor the main display to properly work with this drawing method. This is what I have so far:
progress2

@Vultraz Vultraz self-assigned this Jun 8, 2017

@Vultraz Vultraz added this to the 1.15 milestone Jun 8, 2017

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 12, 2017

Member

@aginor especially could use feedback on b7000a5. Not sure if I'm supposed to be calling join() outside the ctor.

Member

Vultraz commented Jun 12, 2017

@aginor especially could use feedback on b7000a5. Not sure if I'm supposed to be calling join() outside the ctor.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Jun 12, 2017

Member

@aginor actually f50c077 now.

Member

Vultraz commented Jun 12, 2017

@aginor actually f50c077 now.

@sigurdfdragon

bf5133b should be on master

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Jan 13, 2018

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 3
- Added 2
           

See the complete overview on Codacy

codacy-bot commented Jan 13, 2018

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 3
- Added 2
           

See the complete overview on Codacy

@wesnoth wesnoth deleted a comment from codacy-bot Mar 8, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Mar 8, 2018

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Mar 11, 2018

Member

I think it would've actually been better if that Xcode project fix wasn't merged, since this branch now has a conflict in the Xcode project that will need to be resolved when it's rebased...

Member

CelticMinstrel commented Mar 11, 2018

I think it would've actually been better if that Xcode project fix wasn't merged, since this branch now has a conflict in the Xcode project that will need to be resolved when it's rebased...

@wesnoth wesnoth deleted a comment from codacy-bot Mar 11, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Mar 11, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Mar 11, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Mar 11, 2018

Vultraz and others added some commits Jul 22, 2017

Moved SDL_SetRenderDrawColor wrapper to utils file and added a second…
… overload

Also renamed the function to a more colloquial version.
GUI2/Canvas: optimized cached texture management
Instead of throwing out the cached texture every time the canvas was dirty, I've added a flag so that
only happens if its size has actually changed. There still seem to be some issues in GUI2 where canvas
objects keep getting reset, but I'm not sure why. Will have to look into that.

Anyway, the addition of the SDL_RenderClear call also seems to have fixed the weird graphics bleed-through
I was getting when running the game with the OGL driver.

I don't know if the canvas copy or move ctors are needed for anything. I added them for a test, but I'll
leave them for now in case I need them later.
Added new minimap drawing method that works wholly with textures
I've left the old getMinimap function alone and added a new function that will eventually replace it
that utilizes texture rendering to a given texture instead of relying on surface blits and scaling.
This has the benefit of, well, reducing surface ops, for one, as well as removing the need to create
a new texture from the minimap surface every time it's updated.
GUI2/Canvas: allow the default draw routine (shape drawing) to be ove…
…rridden

Previously widgets that wanted to implement custom drawing behavior overrode styled_widget::impl_draw_background
or impl_draw_foreground. Those functions in the base class simply called the canvas render methods. Some widgets
such as the minimap ignored that rendered the contents on their own.

This new method allows rendering directly to the canvas's texture, meaning all the caching and sizing is already
handled by the canvas and doesn't need to be done by the widget; everything's always the right size and redrawn
when necessary.
GUI2/Minimap: refactored drawing code
Instead of using the minimap surface method (which was getting converted from surface to texture every
draw cycle since the caching mechanism the canvas implemented wasn't available and the weird custom
cache the widget implemented itself didn't seem to do actually do anything) we use the new render-to-
canvas-texture method which is a lot cleaner and should be a lot faster. This also means we no longer
override styled_widget::impl_draw_background.

Do note I might use the design of the removed cache here to add some age functionality to the
font::pango_text cache.
Game Display: reimplemented more functionality in draw_movement_info
Not completely done yet, still need to get some stuff working, but this gets rid of the old commented-out
code and puts stuff mostly in the right place.
GUI2/Canvas: some refactoring to correctly reimplement the shape cach…
…ing mechanism

The purpose of the shape caching is essentially to allow multiple draws to consecutively add to
previously drawn content. It essentially adds another condition for clearing the texture prior
to rendering.

I also moved the shape list handling into the draw function itself so it's only executed once
if needed instead multiple times when calling functions like set_variable - not that it actually
would have done anything after the first call, but this allows me to keep the clearing-the-texture
code in one place.
Pango Text: removed extra font path stroke
Looks alright now without it for some reason...go figure.

@wesnoth wesnoth deleted a comment from codacy-bot Mar 13, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Mar 13, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Mar 13, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Mar 13, 2018

/** Helper struct which removes the floating label with the given id upon destruction. */
struct floating_label_scope_helper
{
floating_label_scope_helper(int handle = 0);
// Validate that the passed texture is the current render target.
// TODO: if it's not, maybe set it here?
assert(SDL_GetRenderTarget(video.get_renderer()) == tex);
#ifdef _OPENMP
#pragma omp critical(texture_cache)
#endif

This comment has been minimized.

*
* @param window The SDL window to attach a context to.
*/
context(sdl::window* window);

This comment has been minimized.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Mar 18, 2018

Member

Merged manually.

Member

Vultraz commented Mar 18, 2018

Merged manually.

@Vultraz Vultraz closed this Mar 18, 2018

@Vultraz Vultraz deleted the accelerated_rendering branch Mar 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment