Review: Approve I agree that it is an improvement.
Since I'm still on he dead slow virtual machine, I will give my OK if somebody who can reproduce the fuzzy roads does some testing to confirm that the problem is fixed on their machine - I expect that the problem will have been the same one. Note to the tester: please check as well if https://bugs.launchpad.net/widelands/+bug/1519361 is fixed. We also shouldn't forget to double-check the minimap, including colors and visibility for both players and observers. Code LGTM, just smoe minor code style nits and ideas. Diff comments: > > === added file 'src/graphic/gl/coordinate_conversion.h' > --- src/graphic/gl/coordinate_conversion.h 1970-01-01 00:00:00 +0000 > +++ src/graphic/gl/coordinate_conversion.h 2016-01-04 20:54:29 +0000 > @@ -0,0 +1,73 @@ > +/* > + * Copyright (C) 2006-2015 by the Widelands Development Team > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > + * > + */ > + > +#ifndef WL_GRAPHIC_GL_COORDINATE_CONVERSION_H > +#define WL_GRAPHIC_GL_COORDINATE_CONVERSION_H > + > +#include "base/rect.h" > +#include "graphic/gl/blit_data.h" > + > +// Converts the pixel (x, y) in a texture to a gl coordinate in [0, 1]. > +inline void pixel_to_gl_texture(const int width, const int height, float* x, > float* y) { > + *x = (*x / width); > + *y = 1. - (*y / height); > +} > + > +// Converts the given pixel into an OpenGl point in the renderbuffer. > +inline void pixel_to_gl_renderbuffer(const int width, const int height, > float* x, float* y) { > + *x = (*x / width) * 2. - 1.; > + *y = 1. - (*y / height) * 2.; > +} > + > +// Converts 'rect' given on a screen of 'width' x 'height' pixels into a rect > +// in opengl coordinates in a renderbuffer, i.e. in [-1, 1]. The edges The > returned "The edges The returned" is not part of a sentence. > +// rectangle has positive width and height. > +inline FloatRect > +rect_to_gl_renderbuffer(const int width, const int height, const Rect& rect) > { > + float left = rect.x; > + float top = rect.y; > + float right = rect.x + rect.w; > + float bottom = rect.y + rect.h; > + pixel_to_gl_renderbuffer(width, height, &left, &top); > + pixel_to_gl_renderbuffer(width, height, &right, &bottom); > + return FloatRect(left, bottom, right - left, top - bottom); > +} > + > +// Converts 'rect' given on a texture of 'width' x 'height' pixels into a > rect > +// in opengl coordinates in a texture, i.e. in [0, 1]. Texture pixels are > sampled in their center. > +// The returned rectangle has positive width and height. > +inline FloatRect > +rect_to_gl_texture(const int width, const int height, const FloatRect& rect) > { > + float left = rect.x; > + float top = rect.y; > + float right = rect.x + rect.w; > + float bottom = rect.y + rect.h; > + pixel_to_gl_texture(width, height, &left, &top); > + pixel_to_gl_texture(width, height, &right, &bottom); > + return FloatRect(left, bottom, right - left, top - bottom); > +} > + > +// Convert 'blit_data' from pixel space into opengl space. > +inline FloatRect to_gl_texture(const BlitData& blit_data) { > + return rect_to_gl_texture( > + blit_data.parent_width, blit_data.parent_height, > + FloatRect(blit_data.rect.x, blit_data.rect.y, blit_data.rect.w, > blit_data.rect.h)); > +} > + > +#endif // end of include guard: WL_GRAPHIC_GL_COORDINATE_CONVERSION_H > > === modified file 'src/graphic/gl/dither_program.cc' > --- src/graphic/gl/dither_program.cc 2015-06-07 14:52:11 +0000 > +++ src/graphic/gl/dither_program.cc 2016-01-04 20:54:29 +0000 > @@ -116,16 +126,16 @@ > > switch (order_index) { > case 0: Can we use an enum class to name these, so they have some semantics? > + back.dither_texture_x = 1.; > + back.dither_texture_y = 1.; > + break; > + case 1: > back.dither_texture_x = 0.; > - back.dither_texture_y = 0.; > - break; > - case 1: > - back.dither_texture_x = 1.; > - back.dither_texture_y = 0.; > + back.dither_texture_y = 1.; > break; > case 2: > back.dither_texture_x = 0.5; > - back.dither_texture_y = 1.; > + back.dither_texture_y = 0.; > break; > default: > throw wexception("Never here."); > > === modified file 'src/graphic/gl/utils.h' > --- src/graphic/gl/utils.h 2014-11-08 13:59:33 +0000 > +++ src/graphic/gl/utils.h 2016-01-04 20:54:29 +0000 > @@ -59,22 +59,54 @@ > }; > > // Thin wrapper around a OpenGL buffer object to ensure proper cleanup. > Throws > -// on all errors. > -class Buffer { > +// on all errors. Also grows the server memory only when needed. > +template<typename T> > +class NewBuffer { Can this be renamed back to "Buffer", since I expect that you tracked down all occurrences now? > public: > - Buffer(); > - ~Buffer(); > - > - GLuint object() const { > - return buffer_object_; > + NewBuffer() : buffer_size_(0) { > + glGenBuffers(1, &object_); > + if (!object_) { > + throw wexception("Could not create GL program."); > + } > + } > + > + ~NewBuffer() { > + if (object_) { > + glDeleteBuffers(1, &object_); > + } > + } > + > + // Calls glBindBuffer on the underlying buffer data. > + void bind() const { > + glBindBuffer(GL_ARRAY_BUFFER, object_); > + } > + > + > + // Copies 'elements' into the buffer. If the buffer is too small to > hold the > + // data, it is reallocated. Does not check if the buffer is already > bound. > + void update(const std::vector<T>& items) { > + if (buffer_size_ < items.size()) { > + glBufferData(GL_ARRAY_BUFFER, items.size() * sizeof(T), > items.data(), GL_DYNAMIC_DRAW); > + buffer_size_ = items.size(); > + } else { > + glBufferSubData(GL_ARRAY_BUFFER, 0, items.size() * > sizeof(T), items.data()); > + } > } > > private: > - const GLuint buffer_object_; > + GLuint object_; > + size_t buffer_size_; // In number of elements. > > - DISALLOW_COPY_AND_ASSIGN(Buffer); > + DISALLOW_COPY_AND_ASSIGN(NewBuffer); > }; > > +// Calls glVertexAttribPointer. > +void vertex_attrib_pointer(int vertex_index, int num_items, int stride, int > offset); > + > +// Swap order of rows in m_pixels, to compensate for the upside-down nature > of the > +// OpenGL coordinate system. > +void swap_rows(int width, int height, int pitch, int bpp, uint8_t* pixels); > + > } // namespace Gl > > /** > > === modified file 'src/graphic/minimap_renderer.cc' > --- src/graphic/minimap_renderer.cc 2014-12-07 21:34:11 +0000 > +++ src/graphic/minimap_renderer.cc 2016-01-04 20:54:29 +0000 > @@ -168,90 +147,64 @@ > const int32_t mapwidth = egbase.get_map().get_width(); > const int32_t mapheight = map.get_height(); > > - Point ptopleft; // top left point of the current display frame > + Point ptopleft; // top left point of the current display frame > ptopleft.x = viewpoint.x + mapwidth / 2 - xsize; > - if (ptopleft.x < 0) ptopleft.x += mapwidth; > + if (ptopleft.x < 0) How about: if (ptopleft.x < 0) { ptopleft.x += mapwidth; } etc. > + ptopleft.x += mapwidth; > ptopleft.y = viewpoint.y + mapheight / 2 - ysize; > - if (ptopleft.y < 0) ptopleft.y += mapheight; > + if (ptopleft.y < 0) > + ptopleft.y += mapheight; > > - Point pbottomright; // bottom right point of the current display frame > + Point pbottomright; // bottom right point of the current display frame > pbottomright.x = viewpoint.x + mapwidth / 2 + xsize; > - if (pbottomright.x >= mapwidth) pbottomright.x -= mapwidth; > + if (pbottomright.x >= mapwidth) > + pbottomright.x -= mapwidth; > pbottomright.y = viewpoint.y + mapheight / 2 + ysize; > - if (pbottomright.y >= mapheight) pbottomright.y -= mapheight; > + if (pbottomright.y >= mapheight) > + pbottomright.y -= mapheight; > > uint32_t modx = pbottomright.x % 2; > uint32_t mody = pbottomright.y % 2; > > - if (!player || player->see_all()) { > - for (uint32_t y = 0; y < surface_h; ++y) { > - uint8_t * pix = pixels + y * pitch; > - Widelands::FCoords f > - (Widelands::Coords > - (viewpoint.x, viewpoint.y + (layers & > MiniMapLayer::Zoom2 ? y / 2 : y))); > - map.normalize_coords(f); > - f.field = &map[f]; > - Widelands::MapIndex i = Widelands::Map::get_index(f, > mapwidth); > - for (uint32_t x = 0; x < surface_w; ++x, pix += > sizeof(uint32_t)) { > - if (x % 2 || !(layers & MiniMapLayer::Zoom2)) > - move_r(mapwidth, f, i); > - > - if ((layers & MiniMapLayer::ViewWindow) && > - is_minimap_frameborder( > - f, ptopleft, pbottomright, mapwidth, > mapheight, modx, mody)) { > - *reinterpret_cast<uint32_t *>(pix) = > static_cast<uint32_t> > - > (SDL_MapRGB(&const_cast<SDL_PixelFormat &>(format), 255, 0, 0)); > - } else { > - *reinterpret_cast<uint32_t *>(pix) = > static_cast<uint32_t> > - (calc_minimap_color > - (format, egbase, f, > layers, f.field->get_owned_by(), true)); > + for (uint32_t y = 0; y < surface_h; ++y) { > + Widelands::FCoords f( > + Widelands::Coords(viewpoint.x, viewpoint.y + (layers & > MiniMapLayer::Zoom2 ? y / 2 : y))); > + map.normalize_coords(f); > + f.field = &map[f]; > + Widelands::MapIndex i = Widelands::Map::get_index(f, mapwidth); > + for (uint32_t x = 0; x < surface_w; ++x) { > + if (x % 2 || !(layers & MiniMapLayer::Zoom2)) > + move_r(mapwidth, f, i); > + > + RGBColor pixel_color; > + if ((layers & MiniMapLayer::ViewWindow) && > + is_minimap_frameborder(f, ptopleft, pbottomright, > mapwidth, mapheight, modx, mody)) { > + pixel_color = RGBColor(255, 0, 0); > + } else { > + uint16_t vision = > + 0; // See Player::Field::Vision: 1 if seen > once, > 1 if seen right now. > + Widelands::PlayerNumber owner = 0; > + if (player == nullptr || player->see_all()) { > + vision = 2; // Seen right now. > + owner = f.field->get_owned_by(); > + } else if (player != nullptr) { > + const auto& field = player->fields()[i]; > + vision = field.vision; > + owner = field.owner; > + } > + > + if (vision > 0) { > + pixel_color = > calc_minimap_color(egbase, f, layers, owner, vision > 1); > } > } > - } > - } else { > - Widelands::Player::Field const * const player_fields = > player->fields(); > - for (uint32_t y = 0; y < surface_h; ++y) { > - uint8_t * pix = pixels + y * pitch; > - Widelands::FCoords f > - (Widelands::Coords > - (viewpoint.x, viewpoint.y + > - (layers & MiniMapLayer::Zoom2 ? y / 2 > : y))); > - map.normalize_coords(f); > - f.field = &map[f]; > - Widelands::MapIndex i = Widelands::Map::get_index(f, > mapwidth); > - for (uint32_t x = 0; x < surface_w; ++x, pix += > sizeof(uint32_t)) { > - if (x % 2 || !(layers & MiniMapLayer::Zoom2)) > - move_r(mapwidth, f, i); > - > - if ((layers & MiniMapLayer::ViewWindow) && > - is_minimap_frameborder( > - f, ptopleft, pbottomright, mapwidth, > mapheight, modx, mody)) { > - *reinterpret_cast<uint32_t *>(pix) = > static_cast<uint32_t> > - (SDL_MapRGB > - > (&const_cast<SDL_PixelFormat &>(format), 255, 0, 0)); > - } else { > - const Widelands::Player::Field & > player_field = player_fields[i]; > - Widelands::Vision const vision = > player_field.vision; > - > - *reinterpret_cast<uint32_t *>(pix) = > - static_cast<uint32_t> > - (vision ? > - calc_minimap_color > - (format, > - egbase, > - f, > - layers, > - player_field.owner, > - 1 < vision) > - : > - > SDL_MapRGB(&const_cast<SDL_PixelFormat &>(format), 0, 0, 0)); > - } > + > + if (pixel_color.r != 0 || pixel_color.g != 0 || > pixel_color.b != 0) { How about: if ((pixel_color.r + pixel_color.g + pixel_color.b) > 0) { Would this be more readable? > + texture->set_pixel(x, y, pixel_color); > } > } > } > } > > - > } // namespace > > std::unique_ptr<Texture> draw_minimap(const EditorGameBase& egbase, > > === added file 'src/graphic/render_queue.h' > --- src/graphic/render_queue.h 1970-01-01 00:00:00 +0000 > +++ src/graphic/render_queue.h 2016-01-04 20:54:29 +0000 > @@ -0,0 +1,197 @@ > +/* > + * Copyright (C) 2006-2014 by the Widelands Development Team > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > + * > + */ > + > +#ifndef WL_GRAPHIC_RENDER_QUEUE_H > +#define WL_GRAPHIC_RENDER_QUEUE_H > + > +#include <memory> > +#include <vector> > + > +#include <stdint.h> > + > +#include "base/macros.h" > +#include "base/rect.h" > +#include "graphic/blend_mode.h" > +#include "graphic/color.h" > +#include "graphic/gl/fields_to_draw.h" > +#include "logic/description_maintainer.h" > +#include "logic/world/terrain_description.h" > + > +class DitherProgram; > +class RoadProgram; > +class TerrainProgram; > + > +// The RenderQueue is a singleton implementing the concept of deferred > +// rendering: Every rendering call that pretends to draw onto the screen will > +// instead enqueue an item into the RenderQueue. The Graphic::refresh() will > +// then setup OpenGL to render onto the screen and then call > +// RenderQueue::draw() which will execute all the draw calls. > +// > +// The advantage of this design is that render calls can be reordered and > +// batched up to avoid OpenGL state changes as much as possible. This can > +// reduce the amount of OpenGL calls done in the system per frame by an order > +// of magnitude if assets are properly batched up into texture atlases. > +// > +// Rendering is simple: first everything fully opaque is rendered front to > back > +// (so that no pixel is drawn twice). This allows for maximum program > batching, > +// as for example all opaque rectangles can be rendered in one draw call, > +// ignoring z-value. > +// > +// In the second step, all drawing calls with (partially) transparent pixels > +// are done. This has to be done strictly in z ordering (back to front), so > +// that transparency works correctly. But common operations can still be > +// batched - for example the blitting of houses could all be done with the > same > +// z value and using a common texture atlas. Then they could be drawn in one > +// woosh. > +// > +// Non overlapping rectangles can be drawn in parallel, ignoring z-order. I > +// experimented with a linear algorithm to find all overlapping rectangle > +// pairs (see bzr history), but it did not buy the performance I was hoping > it > +// would. So I abandoned this idea again. > +// > +// Note: all draw calls that target a Texture are not going to the > RenderQueue, > +// but are still immediately executed. The RenderQueue is only used for > +// rendering onto the screen. > +// > +// TODO(sirver): we could (even) better performance by being z-layer aware > +// while drawing. For example the UI could draw non-overlapping windows and > +// sibling children with the same z-value for better batching. Also for > example > +// build-help symbols, buildings, and flags could all be drawn with the same > +// z-layer for better batching up. This would also get rid of the z-layer > +// issues we are having. > +class RenderQueue { > +public: > + enum Program { > + TERRAIN_BASE, Code style: kTerrainBase etc. > + TERRAIN_DITHER, > + TERRAIN_ROAD, > + BLIT, > + BLIT_MONOCHROME, > + BLIT_BLENDED, > + RECT, > + LINE, > + HIGHEST_PROGRAM_ID, > + }; > + > + struct VanillaBlitArguments { > + BlitData texture; > + float opacity; > + }; > + > + struct MonochromeBlitArguments { > + BlitData texture; > + RGBAColor blend; > + }; > + > + struct BlendedBlitArguments { > + BlitData texture; > + BlitData mask; > + RGBAColor blend; > + }; > + > + struct RectArguments { > + RGBAColor color; > + }; > + > + struct LineArguments { > + RGBColor color; > + uint8_t line_width; > + }; > + > + struct TerrainArguments { > + TerrainArguments() {} > + > + int gametime; > + int renderbuffer_width; > + int renderbuffer_height; > + const DescriptionMaintainer<Widelands::TerrainDescription>* > terrains; > + FieldsToDraw* fields_to_draw; > + }; > + > + // The union of all possible program arguments represents an Item that > is > + // enqueued in the Queue. This is on purpose not done with OOP so that > the > + // queue is more cache friendly. > + struct Item { > + Item() {} > + > + inline bool operator<(const Item& other) const { > + return key < other.key; > + } > + > + // The program that will be used to draw this item. Also > defines which > + // union type is filled in. > + int program_id; > + > + // The z-value in GL space that will be used for drawing. > + float z_value; > + > + // The bounding box in the renderbuffer where this draw will > change pixels. > + FloatRect destination_rect; > + > + // The key for sorting this item in the queue. It depends on > the type of > + // item how this is calculated, but it will contain at least > the program, > + // the z-layer, if it is opaque or transparent and program > specific > + // options. After ordering the queue by this, it defines the > batching. > + uint64_t key; > + > + // If this is opaque or, if not, which blend_mode to use. > + BlendMode blend_mode; > + > + union { > + VanillaBlitArguments vanilla_blit_arguments; > + MonochromeBlitArguments monochrome_blit_arguments; > + BlendedBlitArguments blended_blit_arguments; > + TerrainArguments terrain_arguments; > + RectArguments rect_arguments; > + LineArguments line_arguments; > + }; > + }; > + > + static RenderQueue& instance(); > + > + // Enqueues 'item' in the queue with a higher 'z' value than the last > enqueued item. > + void enqueue(const Item& item); > + > + // Draws all items in the queue in an optimal ordering and as much > batching > + // as possible. This will draw one complete frame onto the screen and > this > + // function is the only one that actually triggers draws to the screen > + // directly. > + void draw(int screen_width, int screen_height); > + > +private: > + RenderQueue(); > + > + void draw_items(const std::vector<Item>& items); > + > + // The z value that should be used for the next draw, so that it is on > top > + // of everything before. > + int next_z_; > + > + std::unique_ptr<TerrainProgram> terrain_program_; > + std::unique_ptr<DitherProgram> dither_program_; > + std::unique_ptr<RoadProgram> road_program_; > + > + std::vector<Item> blended_items_; > + std::vector<Item> opaque_items_; > + > + DISALLOW_COPY_AND_ASSIGN(RenderQueue); > +}; > + > + > +#endif // end of include guard: WL_GRAPHIC_RENDER_QUEUE_H -- https://code.launchpad.net/~widelands-dev/widelands/render_queue/+merge/250524 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/render_queue. _______________________________________________ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
