Skip to content

Commit

Permalink
Display: one (hopefully) significant optimization and one minor one
Browse files Browse the repository at this point in the history
Copying the blit_helper objects when adding them to the drawing buffer was expensive since
blit_helper contains a vector of surfaces. We definitely want to copy that as little as
possible.

Also used std::move when fetching fog/shroud images. This function apparently has little
overhead, but still nice to not make copies here.

Also made get_terrain_images return void (this is just minor cleanup, not an optimization).
No need for it to return a reference to a class member.
  • Loading branch information
Vultraz committed Nov 27, 2017
1 parent 9b988f2 commit e8655af
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 15 deletions.
24 changes: 11 additions & 13 deletions src/display.cpp
Expand Up @@ -1067,15 +1067,15 @@ std::vector<surface> display::get_fog_shroud_images(const map_location& loc, ima
std::vector<surface> res;

for (std::string& name : names) {
const surface surf(image::get_image(name, image_type));
surface surf(image::get_image(name, image_type));
if (surf)
res.push_back(surf);
res.push_back(std::move(surf));
}

return res;
}

const std::vector<surface>& display::get_terrain_images(const map_location &loc,
void display::get_terrain_images(const map_location &loc,
const std::string& timeid,
TERRAIN_TYPE terrain_type)
{
Expand Down Expand Up @@ -1222,23 +1222,21 @@ const std::vector<surface>& display::get_terrain_images(const map_location &loc,
}
}
}

return terrain_image_vector_;
}

void display::drawing_buffer_add(const drawing_layer layer,
const map_location& loc, int x, int y, const surface& surf,
const SDL_Rect &clip)
{
drawing_buffer_.push_back(blit_helper(layer, loc, x, y, surf, clip));
drawing_buffer_.emplace_back(layer, loc, x, y, surf, clip);
}

void display::drawing_buffer_add(const drawing_layer layer,
const map_location& loc, int x, int y,
const std::vector<surface> &surf,
const SDL_Rect &clip)
{
drawing_buffer_.push_back(blit_helper(layer, loc, x, y, surf, clip));
drawing_buffer_.emplace_back(layer, loc, x, y, surf, clip);
}

// FIXME: temporary method. Group splitting should be made
Expand Down Expand Up @@ -2566,13 +2564,13 @@ void display::draw_hex(const map_location& loc) {

if(!shrouded(loc)) {
// unshrouded terrain (the normal case)
const std::vector<surface>& images_bg = get_terrain_images(loc, tod.id, BACKGROUND);
drawing_buffer_add(LAYER_TERRAIN_BG, loc, xpos, ypos, images_bg);
num_images_bg = images_bg.size();
get_terrain_images(loc, tod.id, BACKGROUND); // updates terrain_image_vector_

This comment has been minimized.

Copy link
@jyrkive

jyrkive Nov 27, 2017

Member

IMO, the code is less intuitive this way...

This comment has been minimized.

Copy link
@AI0867

AI0867 Nov 27, 2017

Member

I agree. This indeed needs the comment to explain it, which is bad.
As the method already updated the member, I don't really see the point in returning anything, so it should be renamed to better indicate what it does.

drawing_buffer_add(LAYER_TERRAIN_BG, loc, xpos, ypos, terrain_image_vector_);
num_images_bg = terrain_image_vector_.size();

const std::vector<surface>& images_fg = get_terrain_images(loc, tod.id, FOREGROUND);
drawing_buffer_add(LAYER_TERRAIN_FG, loc, xpos, ypos, images_fg);
num_images_fg = images_fg.size();
get_terrain_images(loc, tod.id, FOREGROUND); // updates terrain_image_vector_
drawing_buffer_add(LAYER_TERRAIN_FG, loc, xpos, ypos, terrain_image_vector_);
num_images_fg = terrain_image_vector_.size();

// Draw the grid, if that's been enabled
if(grid_) {
Expand Down
7 changes: 5 additions & 2 deletions src/display.hpp
Expand Up @@ -716,8 +716,7 @@ class display : public filter_context, public video2::draw_layering

enum TERRAIN_TYPE { BACKGROUND, FOREGROUND};

// Warning: the returned vector will be invalidated on the next call!
const std::vector<surface>& get_terrain_images(const map_location &loc,
void get_terrain_images(const map_location &loc,
const std::string& timeid,
TERRAIN_TYPE terrain_type);

Expand Down Expand Up @@ -930,6 +929,10 @@ class display : public filter_context, public video2::draw_layering
class blit_helper
{
public:
// We don't want to copy this.
// It's expensive when done frequently due to the surface vector.
blit_helper(const blit_helper&) = delete;

blit_helper(const drawing_layer layer, const map_location& loc,
const int x, const int y, const surface& surf,
const SDL_Rect& clip)
Expand Down

0 comments on commit e8655af

Please sign in to comment.