Thanks for the review. I addressed all comments. I might have made a mistake
though, it seems that the buildhelp is now broken. Can somebody repo that?
Bug 1519361 does not happen for me with this branch.
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
done.
> +// 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:
done.
> + 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 {
Yes. You mentioned that before and I did not act on it, sorry. Done 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)
Done.
> + 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) {
I prefer the current solution. It is also not prone to integer overflows
(though that should never happen here anyways).
> + 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,
done.
> + 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
