widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

A few nits in the comments

Diff comments:

> 
> === added file 'src/graphic/gl/workarea_program.cc'
> --- src/graphic/gl/workarea_program.cc	1970-01-01 00:00:00 +0000
> +++ src/graphic/gl/workarea_program.cc	2019-03-11 16:44:08 +0000
> @@ -0,0 +1,139 @@
> +/*
> + * Copyright (C) 2006-2019 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.
> + *
> + */
> +
> +#include "graphic/gl/workarea_program.h"
> +
> +#include "graphic/gl/coordinate_conversion.h"
> +#include "graphic/gl/fields_to_draw.h"
> +#include "graphic/gl/utils.h"
> +#include "graphic/texture.h"
> +
> +// QuickRef:
> +// http://www.cs.unh.edu/~cs770/docs/glsl-1.20-quickref.pdf
> +// Full specification:
> +// http://www.opengl.org/registry/doc/GLSLangSpec.Full.1.20.8.pdf
> +// We target OpenGL 2.1 for the desktop here.
> +WorkareaProgram::WorkareaProgram() {
> +	gl_program_.build("workarea");
> +
> +	attr_position_ = glGetAttribLocation(gl_program_.object(), "attr_position");
> +	attr_overlay_ = glGetAttribLocation(gl_program_.object(), "attr_overlay");
> +
> +	u_z_value_ = glGetUniformLocation(gl_program_.object(), "u_z_value");
> +}
> +
> +void WorkareaProgram::gl_draw(int gl_texture, float z_value) {
> +	glUseProgram(gl_program_.object());
> +
> +	auto& gl_state = Gl::State::instance();
> +	gl_state.enable_vertex_attrib_array(
> +	   {attr_position_, attr_overlay_});
> +
> +	gl_array_buffer_.bind();
> +	gl_array_buffer_.update(vertices_);
> +
> +	Gl::vertex_attrib_pointer(
> +	   attr_position_, 2, sizeof(PerVertexData), offsetof(PerVertexData, gl_x));
> +	Gl::vertex_attrib_pointer(
> +	   attr_overlay_, 4, sizeof(PerVertexData), offsetof(PerVertexData, overlay_r));
> +
> +	gl_state.bind(GL_TEXTURE0, gl_texture);
> +
> +	glUniform1f(u_z_value_, z_value);
> +
> +	glDrawArrays(GL_TRIANGLES, 0, vertices_.size());
> +}
> +
> +#define WORKAREA_TRANSPARENCY 127

Please use a constexpr called kWorkareaTransparency

> +static RGBAColor workarea_colors[] {
> +	RGBAColor(63, 31, 127, WORKAREA_TRANSPARENCY), // All three circles
> +	RGBAColor(127, 63, 0, WORKAREA_TRANSPARENCY),  // Medium and outer circle
> +	RGBAColor(0, 127, 0, WORKAREA_TRANSPARENCY),   // Outer circle
> +	RGBAColor(63, 0, 127, WORKAREA_TRANSPARENCY),  // Inner and medium circle
> +	RGBAColor(127, 0, 0, WORKAREA_TRANSPARENCY),   // Medium circle
> +	RGBAColor(0, 0, 127, WORKAREA_TRANSPARENCY),   // Inner circle
> +};
> +static inline RGBAColor apply_color(RGBAColor c1, RGBAColor c2) {
> +	uint8_t r = (c1.r * c1.a + c2.r * c2.a) / (c1.a + c2.a);
> +	uint8_t g = (c1.g * c1.a + c2.g * c2.a) / (c1.a + c2.a);
> +	uint8_t b = (c1.b * c1.a + c2.b * c2.a) / (c1.a + c2.a);
> +	uint8_t a = (c1.a + c2.a) / 2;
> +	return RGBAColor(r, g, b, a);
> +}
> +
> +void WorkareaProgram::add_vertex(const FieldsToDraw::Field& field, RGBAColor overlay) {
> +	vertices_.emplace_back();
> +	PerVertexData& back = vertices_.back();
> +
> +	back.gl_x = field.gl_position.x;
> +	back.gl_y = field.gl_position.y;
> +	back.overlay_r = overlay.r / 255.f;
> +	back.overlay_g = overlay.g / 255.f;
> +	back.overlay_b = overlay.b / 255.f;
> +	back.overlay_a = overlay.a / 255.f;
> +}
> +
> +void WorkareaProgram::draw(uint32_t texture_id,
> +             	           Workareas workarea,
> +                           const FieldsToDraw& fields_to_draw,
> +                           float z_value) {
> +	vertices_.clear();
> +	vertices_.reserve(fields_to_draw.size() * 3);
> +
> +	for (size_t current_index = 0; current_index < fields_to_draw.size(); ++current_index) {
> +		const FieldsToDraw::Field& field = fields_to_draw.at(current_index);
> +
> +		// The bottom right neighbor fields_to_draw is needed for both triangles
> +		// associated with this field. If it is not in fields_to_draw, there is no need to
> +		// draw any triangles.
> +		if (field.brn_index == FieldsToDraw::kInvalidIndex) {
> +			continue;
> +		}
> +
> +		// Down triangle.
> +		if (field.bln_index != FieldsToDraw::kInvalidIndex) {
> +			RGBAColor color(0, 0, 0, 0);
> +			for (const std::map<Widelands::TCoords<>, uint8_t>& wa_map : workarea) {
> +				const auto it = wa_map.find(Widelands::TCoords<>(field.fcoords, Widelands::TriangleIndex::D));
> +				if (it != wa_map.end()) {
> +					color = apply_color(color, workarea_colors[it->second]);
> +				}
> +			}
> +			add_vertex(fields_to_draw.at(current_index), color);
> +			add_vertex(fields_to_draw.at(field.bln_index), color);
> +			add_vertex(fields_to_draw.at(field.brn_index), color);
> +		}
> +
> +		// Right triangle.
> +		if (field.rn_index != FieldsToDraw::kInvalidIndex) {
> +			RGBAColor color(0, 0, 0, 0);
> +			for (const std::map<Widelands::TCoords<>, uint8_t>& wa_map : workarea) {
> +				const auto it = wa_map.find(Widelands::TCoords<>(field.fcoords, Widelands::TriangleIndex::R));
> +				if (it != wa_map.end()) {
> +					color = apply_color(color, workarea_colors[it->second]);
> +				}
> +			}
> +			add_vertex(fields_to_draw.at(current_index), color);
> +			add_vertex(fields_to_draw.at(field.brn_index), color);
> +			add_vertex(fields_to_draw.at(field.rn_index), color);
> +		}
> +	}
> +
> +	gl_draw(texture_id, z_value);
> +}
> 
> === added file 'src/graphic/gl/workarea_program.h'
> --- src/graphic/gl/workarea_program.h	1970-01-01 00:00:00 +0000
> +++ src/graphic/gl/workarea_program.h	2019-03-11 16:44:08 +0000
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (C) 2006-2019 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_WORKAREA_PROGRAM_H
> +#define WL_GRAPHIC_GL_WORKAREA_PROGRAM_H
> +
> +#include <vector>
> +
> +#include "base/vector.h"
> +#include "graphic/gl/fields_to_draw.h"
> +#include "graphic/gl/utils.h"
> +#include "logic/description_maintainer.h"
> +#include "logic/map_objects/world/terrain_description.h"
> +
> +class WorkareaProgram {
> +public:
> +	// Compiles the program. Throws on errors.
> +	WorkareaProgram();
> +
> +	// Draws the workarea overlay.
> +	void draw(uint32_t texture_id,
> +	          Workareas workarea,
> +	          const FieldsToDraw& fields_to_draw,
> +	          float z_value);
> +
> +private:
> +	struct PerVertexData {
> +		float gl_x;
> +		float gl_y;
> +		float overlay_r;
> +		float overlay_g;
> +		float overlay_b;
> +		float overlay_a;
> +	};
> +	static_assert(sizeof(PerVertexData) == 24, "Wrong padding.");
> +
> +	void gl_draw(int gl_texture, float z_value);
> +
> +	// Adds a vertex to the end of vertices with data from 'field' and 'texture_coordinates'.

Update comment to fit the actual parameters

> +	void add_vertex(const FieldsToDraw::Field& field, RGBAColor overlay);
> +
> +	// The program used for drawing the workarea overlay.
> +	Gl::Program gl_program_;
> +
> +	// The buffer that will contain 'vertices_' for rendering.
> +	Gl::Buffer<PerVertexData> gl_array_buffer_;
> +
> +	// Attributes.
> +	GLint attr_position_;
> +	GLint attr_overlay_;
> +
> +	// Uniforms.
> +	GLint u_z_value_;
> +
> +	// Objects below are kept around to avoid memory allocations on each frame.
> +	// They could theoretically also be recreated.
> +	std::vector<PerVertexData> vertices_;
> +
> +	DISALLOW_COPY_AND_ASSIGN(WorkareaProgram);
> +};
> +
> +#endif  // end of include guard: WL_GRAPHIC_GL_WORKAREA_PROGRAM_H
> 
> === modified file 'src/wui/interactive_base.cc'
> --- src/wui/interactive_base.cc	2019-02-27 17:19:00 +0000
> +++ src/wui/interactive_base.cc	2019-03-11 16:44:08 +0000
> @@ -305,12 +310,39 @@
>  	workarea_previews_[coords] = &workarea_info;
>  }
>  
> -std::map<Coords, const Image*>
> -InteractiveBase::get_workarea_overlays(const Widelands::Map& map) const {
> -	std::map<Coords, const Image*> result;
> -	for (const auto& pair : workarea_previews_) {
> -		const Coords& coords = pair.first;
> -		const WorkareaInfo* workarea_info = pair.second;
> +// Helper function to get the correct index for graphic/gl/workarea_program.cc::workarea_colors
> +static uint8_t workarea_max(uint8_t a, uint8_t b, uint8_t c) {

What do a, b, c stand for? I find this bit of the code hard to understand.

> +	bool inner = (a == 0 || a == 3 || a == 5) && (b == 0 || b == 3 || b == 5) && (c == 0 || c == 3 || c == 5);
> +	bool medium = (a == 0 || a == 1 || a == 3 || a == 4) && (b == 0 || b == 1 || b == 3 || b == 4) &&
> +			(c == 0 || c == 1 || c == 3 || c == 4);
> +	bool outer = a <= 2 && b <= 2 && c <= 2;
> +	if (medium && outer && inner) {
> +		return 0;
> +	}
> +	if (medium && outer) {
> +		return 1;
> +	}
> +	if (medium && inner) {
> +		return 3;
> +	}
> +	if (medium) {
> +		return 4;
> +	}
> +	if (outer) {
> +		assert(!inner);
> +		return 2;
> +	}
> +	assert(inner);
> +	return 5;
> +}
> +
> +Workareas InteractiveBase::get_workarea_overlays(const Widelands::Map& map) const {
> +	Workareas result_set;
> +	for (const auto& wa_pair : workarea_previews_) {
> +		std::map<Coords, uint8_t> intermediate_result;
> +		const Coords& coords = wa_pair.first;
> +		const WorkareaInfo* workarea_info = wa_pair.second;
> +		intermediate_result[coords] = 0;
>  		WorkareaInfo::size_type wa_index;
>  		switch (workarea_info->size()) {
>  		case 0:


-- 
https://code.launchpad.net/~nordfriese/widelands/workareas/+merge/364266
Your team Widelands Developers is requested to review the proposed merge of lp:~nordfriese/widelands/workareas into lp:widelands.