widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

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.