← Back to team overview

widelands-dev team mailing list archive

Re: [Merge] lp:~widelands-dev/widelands/mipmaps into lp:widelands

 

Code looks good to me, some diff comments with smaller suggestions.

Will test this more thoroughly the next days…

Diff comments:

> === modified file 'data/scripting/mapobjects.lua'
> --- data/scripting/mapobjects.lua	2017-02-08 18:48:32 +0000
> +++ data/scripting/mapobjects.lua	2019-03-02 13:59:45 +0000
> @@ -20,32 +22,114 @@
>  end
>  
>  
> --- RST
> --- .. function:: add_walking_animations(table, animationname, dirname, basename, hotspot, fps)
> ---
> ---    Adds 6 walk or sail animations - one for each walking direction - to 'table'.
> ---
> ---    :arg table: A table that the animation data is added to. It may already contain some animations.
> +-- Helper function for finding animation files with varying numbering length
> +function get_animation_files(prefix)
> +   local animation_files = path.list_files(prefix .. "_??.png")
> +   if #animation_files < 1 then
> +      animation_files = path.list_files(prefix .. "_???.png")
> +   end
> +   if #animation_files < 1 then
> +      animation_files = path.list_files(prefix .. "_?.png")
> +   end
> +   return animation_files
> +end
> +
> +
> +-- The mipmap scales supported by the engine.
> +-- Ensure that this always matches kSupportedScales in animations.cc
> +local supported_scales = { 0.5, 1, 2, 4 }
> +
> +
> +-- RST
> +-- .. function:: add_animation(animationtable, animationname, dirname, basename, hotspot [, fps])
> +--
> +--    Convenience function for adding an animation to `animationtable`.
> +--    Supports both simple animations and mipmaps.
> +--    See :ref:`animations` for more documentation and code examples.
> +--
> +--    :arg animationtable: The table that the animation data will be added to.
> +--       It may already contain some animations.
> +--    :type animationtable: :class:`table`
> +--    :arg animationname: The name of the animation to be added, e.g. ``idle``.
> +--    :type animationname: :class:`string`
> +--    :arg dirname: The name of the directory where the animation image files are located.
> +--    :type dirname: :class:`string`
> +--    :arg basename: The basename of the animation files. The filenames of the
> +--       animation files need to have the format ``<basename>_\d{2,3}.png`` for simple
> +--       file animations, and  ``<basename>_<scale>_\d{2,3}.png`` for mipmaps.
> +--       Supported scales are ``0.5``, ``1``, ``2`` and ``4``.
> +--    :type basename: :class:`string`
> +--    :arg hotspot: The hotspot coordinates for blitting, e.g. ``{2, 20}``.
> +--    :type hotspot: :class:`table`

Minor suggestion: Specify hotspot type as array to make sure nobody tries to use a table like {x=2, y=20}

> +--    :arg fps: Frames per second. Only use this if the animation has more than
> +--       1 frame, and if you need to deviate from the default frame rate.
> +--    :type fps: :class:`integer`
> +function add_animation(animationtable, animationname, dirname, basename, hotspot, fps)
> +   mipmap = {}
> +   for scale_idx, current_scale in ipairs(supported_scales) do
> +      local listed_files = get_animation_files(dirname .. basename .. "_" .. current_scale)
> +      if #listed_files > 0 then
> +         table.insert(
> +            mipmap,
> +            {
> +               scale = current_scale,
> +               files = listed_files,
> +            }
> +         )
> +      end
> +   end
> +   if #mipmap < 1 then
> +      table.insert(
> +      mipmap,
> +         {
> +            scale = 1,
> +            files = get_animation_files(dirname .. basename),
> +         }
> +      )
> +   end
> +   animationtable[animationname] = {
> +      mipmap = mipmap,
> +      hotspot = hotspot,
> +   }
> +   if (fps ~= nil) then

Idea: Perhaps you could here check whether the animation has more than 1 frame to prevent the error that occurs in that case?

> +      animationtable[animationname]["fps"] = fps
> +   end
> +end
> +
> +
> +
> +-- RST
> +-- .. function:: add_walking_animations(animationtable, animationname, dirname, basename, hotspot [, fps])
> +--
> +--    Adds 6 walk or sail animations - one for each walking direction - to `animationtable`.
> +--    Supports both simple animations and mipmaps.
> +--    See :ref:`animations` for more documentation and code examples.
> +--
> +--    :arg animationtable: The table that the animation data will be added to.
> +--       It may already contain some animations.
> +--    :type animationtable: :class:`table`
>  --    :arg animationname: The name of the animation to be added, e.g. ``walkload``.
> +--    :type animationname: :class:`string`
>  --    :arg dirname: The name of the directory where the animation image files are located.
> ---    :arg basename: The basename of the animation files. The filenames of the animation files need to have the format ``<basename>_(e|ne|se|sw|w|nw)_\d+.png``
> ---    :arg hotspot: The hotspot coordinates for blitting, e.g. ``{ 2, 20 }``.
> ---    :arg fps: Frames per second. Only use this if the animation has more than 1 frame, and if you need to deviate from the default frame rate.
> -function add_walking_animations(table, animationname, dirname, basename, hotspot, fps)
> -   if (fps ~= nil) then
> -      for idx, dir in ipairs{ "ne", "e", "se", "sw", "w", "nw" } do
> -         table[animationname .. "_" .. dir] = {
> -            pictures = path.list_files(dirname .. basename .. "_" .. dir ..  "_??.png"),
> -            hotspot = hotspot,
> -            fps = fps,
> -         }
> -      end
> -   else
> -      for idx, dir in ipairs{ "ne", "e", "se", "sw", "w", "nw" } do
> -         table[animationname .. "_" .. dir] = {
> -            pictures = path.list_files(dirname .. basename .. "_" .. dir ..  "_??.png"),
> -            hotspot = hotspot,
> -         }
> +--    :type dirname: :class:`table`
> +--    :arg basename: The basename of the animation files. The filenames of the
> +--       animation files need to have the format
> +--       ``<basename>_(e|ne|se|sw|w|nw)_\d{2,3}.png`` for simple animations, and
> +--       ``<basename>_(e|ne|se|sw|w|nw)_<scale>_\d{2,3}.png`` for mipmaps.
> +--       Supported scales are ``0.5``, ``1``, ``2`` and ``4``.
> +--    :type basename: :class:`string`
> +--    :arg hotspot: The hotspot coordinates for blitting, e.g. ``{2, 20}``.
> +--    :type hotspot: :class:`table`
> +--    :arg fps: Frames per second. Only use this if the animation has more than
> +--       1 frame, and if you need to deviate from the default frame rate.
> +--    :type fps: :class:`integer`
> +function add_walking_animations(animationtable, animationname, dirname, basename, hotspot, fps)
> +   for idx, dir in ipairs{ "ne", "e", "se", "sw", "w", "nw" } do
> +      if fps ~= nil then
> +         add_animation(animationtable, animationname .. "_" .. dir, dirname, basename .. "_" .. dir, hotspot, fps)
> +      else
> +         add_animation(animationtable, animationname .. "_" .. dir, dirname, basename .. "_" .. dir, hotspot)
>        end
>     end
>  end
> +
> 
> === modified file 'doc/sphinx/source/animations.rst'
> --- doc/sphinx/source/animations.rst	2018-04-09 08:01:28 +0000
> +++ doc/sphinx/source/animations.rst	2019-03-02 13:59:45 +0000
> @@ -24,8 +35,11 @@
>  **idle**
>     *Mandatory*. This is the name of the animation. The animation can then be referenced by this name e.g. if you want to trigger it in a production program.
>  
> +**files**
> +   *Mandatory*. A template for the image names. Our example will pick up any image from ``idle_00.png`` through ``idle_99.png`` in the specified directory path -- the current path in our example. These images can optionally have corresponding player color mask images called ``idle_00_pc.png`` through ``idle_99_pc.png``. Make sure to include leading 0's in the file names and to have consistent length -- we support either 2 digit or 3 digit numbers in an animation.

I added support for 1 digit as well because images added by my Graphics utils use the shortest file names possible

> +
>  **pictures**
> -   *Mandatory*. A template for the image names. Our example will pick up any image from ``idle_00.png`` through ``idle_99.png`` in the specified directory path – the current path in our example. These images can optionally have corresponding player color mask images called ``idle_00_pc.png`` through ``idle_99_pc.png``. Make sure to include leading 0's in the file names.
> +   *DEPRECATED*. The same as ``files``.
>  
>  **hotspot**
>     *Mandatory*. Hotspots define a place on a graphic image through its *x* and *y* coordinates that can be used as a handle or reference point to provide control over positioning the image on the map. For example, hotspots help carriers stay on the road, and control the positioning of the wares they carry. Increase ``x`` to shift the animation to the left and ``y`` to shift it upwards.
> @@ -33,22 +47,141 @@
>  **fps**
>     *Optional*. The frames per second for this animation if you want to deviate from the default fps. This will control the playback speed of the animation. Do not specify this value if you have only 1 animation frame.
>  
> -**scale**
> -   **DEPRECATED**. If the animation should be blitted at any other scale than 1:1,
> -   specify the float value here. For example, if the animation images are 2.5 times the size of what should be blitted at default zoom, use ``scale = 2.5``.
> -
>  **sound_effect**
>     *Optional*. Our example will look for the sound files ``bar_00.ogg`` through ``bar_99.ogg`` in the directory ``data/sound/foo`` and play them in sequence.
>  
>  
> +Mipmaps
> +-------
> +
> +We support mipmaps for animations. They allow us to provide the same image in different
> +resolutions for optimum rendering quality. Let's look at an example with a mipmap ``idle`` animation and a normal ``build`` animation:
> +
> +.. code-block:: lua
> +
> +   animations = {
> +      idle = {
> +         mipmap = {
> +            {
> +               scale = 0.5,
> +               files = path.list_files(dirname .. "idle_0.5_??.png"),
> +            },
> +            {
> +               scale = 1,
> +               files = path.list_files(dirname .. "idle_1_??.png"),
> +            },
> +            {
> +               scale = 2,
> +               files = path.list_files(dirname .. "idle_2_??.png"),
> +            },
> +            {
> +               scale = 4,
> +               files = path.list_files(dirname .. "idle_4_??.png"),
> +            }
> +         },
> +         hotspot = { 5, 7 },
> +         fps = 4,
> +         sound_effect = {
> +            directory = "sound/foo",
> +            name = "bar",
> +         },
> +      },
> +      build = {
> +         files = path.list_files(dirname .. "build_??.png"),
> +         hotspot = { 5, 7 },
> +      }
> +   },
> +
> +The scale of ``1`` is mandatory, and other supported scales are ``0.5``, ``2``
> +and ``4``.
> +The base table should no longer contain the ``files`` entry
> +when you're using a mipmap.
> +Each mimap entry must define the ``files`` and the ``scale``.
> +See also :ref:`animations_converting_formats`.
> +
> +
>  Directional Animations
>  ----------------------
>  
>  For objects that move around the map, like carriers, ships or animals, there need to be 6 animations for the walking directions northeast ``"ne"``, east ``"e"``, southeast ``"se"``, southwest ``"sw"``, west ``"w"``, and northwest ``"nw"``. So, a "walk" animation would consist of 6 animations called ``"walk_ne"``, ``"walk_e"``, ``"walk_se"``, ``"walk_sw"``, ``"walk_w"``, and ``"walk_nw"``.
>  
> -Each of these 6 animations will then be defined like the animation above, so we would end up with pictures called ``walk_ne_00.png``, ``walk_ne_01.png`` ... ``walk_nw_00.png``,  ``walk_nw_01.png`` ..., and for player color: ``walk_ne_00_pc.png``, ``walk_ne_01_pc.png`` ... ``walk_nw_00_pc.png``,  ``walk_nw_01_pc.png``, ...
> -
> -In order to cut down on the manual coding needed, we have a convenience function :any:`add_walking_animations` to define the animation tables for walking animations. The corresponding ``.lua`` script file is included centrally when the tribe or world loading is started, so you won't need to include it again.
> +Each of these 6 animations will then be defined like the animation above, so we would end up with files called ``walk_ne_00.png``, ``walk_ne_01.png`` ... ``walk_nw_00.png``,  ``walk_nw_01.png`` ..., and for player color: ``walk_ne_00_pc.png``, ``walk_ne_01_pc.png`` ... ``walk_nw_00_pc.png``,  ``walk_nw_01_pc.png``, ...
> +
> +We also support mipmaps here -- name the files ``walk_ne_0.5_00.png``,
> +``walk_ne_0.5_01.png`` etc. for scale `0.5`, ``walk_ne_1_00.png``,
> +``walk_ne_1_01.png`` etc. for scale `1` and so on.
> +
> +
> +
> +.. _animations_convenience_functions:
> +
> +Convenience Functions
> +---------------------
> +
> +In order to cut down on the manual coding needed, we provide the convenience functions
> +:any:`add_animation` for static animations and :any:`add_walking_animations` for walking
> +animations, both of which will also detect mipmaps automatically.
> +The corresponding ``.lua`` script file is included centrally when the tribe or world
> +loading is started, so you won't need to include it again. Example:
> +
> +.. code-block:: lua
> +
> +   dirname = path.dirname(__file__)
> +
> +   -- This table will contain the animations
> +   animations = {}
> +
> +   -- Add an idle animation with hotspot = {16, 30} and fps = 5
> +   add_animation(animations, "idle", dirname, "idle", {16, 30}, 5)
> +
> +   -- Add animations for the 6 directions with hotspot = {16, 30} and fps = 10
> +   add_walking_animations(animations, "walk", dirname, "walk", {16, 30}, 10)
> +
> +   -- Add a "walkload" animation. The animation hasn't been created yet in this example, so we reuse the files for the "walk" animation.
> +   add_walking_animations(animations, "walkload", dirname, "walk", {16, 30}, 10)
> +
> +
> +   tribes:new_worker_type {
> +      msgctxt = "fancytribe_worker",
> +      name = "fancytribe_diligentworker",
> +      ...
> +
> +      animations = animations, -- Include the animations table in your map object
> +      ...
> +   }
> +
> +The convenience functions don't support sound effects directly, so you'll have to
> +add them manually, like this:
> +
> +.. code-block:: lua
> +
> +   animations = {}
> +   add_animation(animations, "work", dirname, "work", {11, 26}, 10)
> +   animations["work"]["sound_effect"] = {
> +      name = "bar",
> +      directory = "sound/foo"
> +   }
> +
> +
> +.. _animations_converting_formats:
> +
> +Converting Animation Formats
> +----------------------------
> +
> +When converting a simple file animation to a mipmap animation, follow these steps:
> +
> +* Use `utils/rename_animation.py` to rename the previous animation, to make sure
> +  that our version control system will not lose its history, e.g.::
> +
> +   utils/rename_animation.py data/tribes/workers/fancytribe/diligentworker/walk_ne data/tribes/workers/fancytribe/diligentworker/walk_ne_1
> +   utils/rename_animation.py data/tribes/workers/fancytribe/diligentworker/walk_nw data/tribes/workers/fancytribe/diligentworker/walk_nw_1
> +   ...
> +
> +* Export the new animations from Blender, preferably at all supported scales.
> +  Only export the higher resolution scales if the textures have sufficient resolution.

I added a java tool named MipmapMaker in the media repo. It will quickly create mipmaps for all supported scales at once from a high-resolution set of images. That´s rather faster and more convenient than the method you describe here

> +
> +
> +.. _animations_map_object_types:
>  
>  Map Object Types
>  ----------------
> 
> === modified file 'src/graphic/animation.cc'
> --- src/graphic/animation.cc	2019-02-23 11:00:49 +0000
> +++ src/graphic/animation.cc	2019-03-02 13:59:45 +0000
> @@ -42,19 +44,34 @@
>  #include "sound/sound_handler.h"
>  
>  namespace {
> +// The mipmap scales supported by the engine.
> +// Ensure that this always matches supported_scales in data/scription/mapobjects.lua.

scripting, not scription

> +const std::set<float> kSupportedScales { 0.5, 1, 2, 4};
> +
>  /**
>   * Implements the Animation interface for an animation that is unpacked on disk, that
>   * is every frame and every pc color frame is an singular file on disk.
>   */
>  class NonPackedAnimation : public Animation {
>  public:
> +	struct MipMapEntry {
> +		explicit MipMapEntry(float scale, const LuaTable& table);
> +
> +		bool hasplrclrs;

Please name it has_pc or has_player_colors or at least add some underscores, but this is kind of hard to read ;)

> +		std::vector<std::string> image_files;
> +		std::vector<std::string> pc_mask_image_files;
> +
> +		std::vector<const Image*> frames;
> +		std::vector<const Image*> pcmasks;

here_as_well_please

> +	};
> +
>  	~NonPackedAnimation() override {
>  	}
>  	explicit NonPackedAnimation(const LuaTable& table);
>  
>  	// Implements Animation.
>  	float height() const override;
> -	Rectf source_rectangle(int percent_from_bottom) const override;
> +	Rectf source_rectangle(int percent_from_bottom, float scale) const override;
>  	Rectf destination_rectangle(const Vector2f& position,
>  	                            const Rectf& source_rect,
>  	                            float scale) const override;
> @@ -111,98 +156,128 @@
>  			g_sound_handler.load_fx_if_needed(directory, name, sound_effect_);
>  		}
>  
> +		// Repetition
>  		if (table.has_key("play_once")) {
>  			play_once_ = table.get_bool("play_once");
>  		}
>  
> -		image_files_ = table.get_table("pictures")->array_entries<std::string>();
> +		if (table.has_key("mipmap")) {
> +			std::unique_ptr<LuaTable> mipmaps_table = table.get_table("mipmap");
> +			for (const int key : mipmaps_table->keys<int>()) {
> +				std::unique_ptr<LuaTable> current_scale_table = mipmaps_table->get_table(key);
> +				const float current_scale = current_scale_table->get_double("scale");
> +				if (kSupportedScales.count(current_scale) != 1) {
> +					std::string supported_scales = "";
> +					for (const float supported_scale : kSupportedScales) {
> +						supported_scales = (boost::format("%s %.1f") % supported_scales % supported_scale).str();
> +					}
> +					throw wexception(
> +						"Animation has unsupported scale '%.1f' in mipmap - supported scales are:%s", current_scale, supported_scales.c_str());
> +				}
> +				mipmaps_.insert(std::make_pair(current_scale, std::unique_ptr<MipMapEntry>(new MipMapEntry(current_scale, *current_scale_table))));
> +			}
> +		} else {
> +			mipmaps_.insert(std::make_pair(1.0f, std::unique_ptr<MipMapEntry>(new MipMapEntry(1.0f, table))));
> +		}
>  
> -		if (image_files_.empty()) {
> -			throw wexception("Animation without pictures. The template should look similar to this:"
> -			                 " 'directory/idle_??.png' for 'directory/idle_00.png' etc.");
> -		} else if (table.has_key("fps")) {
> -			if (image_files_.size() == 1) {
> +		// Frames
> +		if (table.has_key("fps")) {
> +			if (nr_frames_ == 1) {
>  				throw wexception(
> -				   "Animation with one picture %s must not have 'fps'", image_files_[0].c_str());
> +					"Animation with one picture %s must not have 'fps'", mipmaps_.begin()->second->image_files[0].c_str());
>  			}
>  			frametime_ = 1000 / get_positive_int(table, "fps");
>  		}
> -
> -		for (std::string image_file : image_files_) {
> -			boost::replace_last(image_file, ".png", "_pc.png");
> -			if (g_fs->file_exists(image_file)) {
> -				hasplrclrs_ = true;
> -				pc_mask_image_files_.push_back(image_file);
> -			} else if (hasplrclrs_) {
> -				throw wexception("Animation is missing player color file: %s", image_file.c_str());
> -			}
> -		}
> -
> -		if (table.has_key("scale")) {
> -			scale_ = table.get_double("scale");
> -			if (scale_ <= 0.0f) {
> -				throw wexception("Animation scale needs to be > 0.0f, but it is %f. The first image of "
> -				                 "this animation is %s",
> -				                 static_cast<double>(scale_), image_files_[0].c_str());
> -			}
> -		}
> -
> -		assert(!image_files_.empty());
> -		assert(pc_mask_image_files_.size() == image_files_.size() || pc_mask_image_files_.empty());
> -		assert(scale_ > 0);
> +		nr_frames_ = mipmaps_.begin()->second->image_files.size();
> +
> +		// Perform some checks to make sure that the data is complete and consistent
> +		const bool should_have_playercolor = mipmaps_.begin()->second->hasplrclrs;
> +		for (const auto& mipmap : mipmaps_) {
> +			if (mipmap.second->image_files.size() != nr_frames_) {
> +				throw wexception("Mismatched number of images for different scales in animation table: %" PRIuS " vs. %u at scale %.2f",
> +									  mipmap.second->image_files.size(),
> +									  nr_frames_,
> +									  mipmap.first);
> +			}
> +			if (mipmap.second->hasplrclrs != should_have_playercolor) {
> +				throw wexception("Mismatched existence of player colors in animation table for scales %.2f and %.2f",
> +									  mipmaps_.begin()->first,
> +									  mipmap.first);
> +			}
> +		}
> +		if (mipmaps_.count(1.0f) != 1) {
> +			throw wexception("All animations must provide images for the neutral scale (1.0)");

As far as I can see, 1.0 is required only for representative_images and calculating the image height.
Wouldn´t it be also possible use another mipmap entry for the repr_image if 1.0 isn´t provided (see also my next diff comment), 
and calculate the height from the next-highest provided scale (or the highest provided if they´re all <1) and remove this restriction?

> +		}
>  	} catch (const LuaError& e) {
>  		throw wexception("Error in animation table: %s", e.what());
>  	}
>  }
>  
> +float NonPackedAnimation::find_best_scale(float scale) const {
> +	assert(!mipmaps_.empty());
> +	float result = mipmaps_.begin()->first;
> +	for (const auto& mipmap : mipmaps_) {
> +		// The map is reverse sorted, so we can break as soon as we are lower than the wanted scale
> +		if (mipmap.first < scale) {
> +			break;
> +		}
> +		result = mipmap.first;
> +	}
> +	return result;
> +}
> +
>  void NonPackedAnimation::ensure_graphics_are_loaded() const {
> -	if (frames_.empty()) {
> +	if (mipmaps_.begin()->second->frames.empty()) {
>  		const_cast<NonPackedAnimation*>(this)->load_graphics();
>  	}
>  }
>  
>  void NonPackedAnimation::load_graphics() {
> -	if (image_files_.empty())
> -		throw wexception("animation without pictures.");
> -
> -	if (pc_mask_image_files_.size() && pc_mask_image_files_.size() != image_files_.size())
> -		throw wexception("animation has %" PRIuS " frames but playercolor mask has %" PRIuS " frames",
> -		                 image_files_.size(), pc_mask_image_files_.size());
> -
> -	for (const std::string& filename : image_files_) {
> -		const Image* image = g_gr->images().get(filename);
> -		if (frames_.size() &&
> -		    (frames_[0]->width() != image->width() || frames_[0]->height() != image->height())) {
> -			throw wexception("wrong size: (%u, %u), should be (%u, %u) like the first frame",
> -			                 image->width(), image->height(), frames_[0]->width(),
> -			                 frames_[0]->height());
> -		}
> -		frames_.push_back(image);
> -	}
> -
> -	for (const std::string& filename : pc_mask_image_files_) {
> -		// TODO(unknown): Do not load playercolor mask as opengl texture or use it as
> -		//     opengl texture.
> -		const Image* pc_image = g_gr->images().get(filename);
> -		if (frames_[0]->width() != pc_image->width() || frames_[0]->height() != pc_image->height()) {
> -			// TODO(unknown): see bug #1324642
> -			throw wexception("playercolor mask has wrong size: (%u, %u), should "
> -			                 "be (%u, %u) like the animation frame",
> -			                 pc_image->width(), pc_image->height(), frames_[0]->width(),
> -			                 frames_[0]->height());
> -		}
> -		pcmasks_.push_back(pc_image);
> +	for (const auto& entry : mipmaps_) {
> +		MipMapEntry* mipmap = entry.second.get();
> +
> +		if (mipmap->image_files.empty()) {
> +			throw wexception("animation without image files at promised scale %.2f.", entry.first);
> +		}
> +		if (mipmap->pc_mask_image_files.size() && mipmap->pc_mask_image_files.size() != mipmap->image_files.size()) {
> +			throw wexception("animation has %" PRIuS " frames but playercolor mask has %" PRIuS " frames for scale %.2f",
> +								  mipmap->image_files.size(), mipmap->pc_mask_image_files.size(), entry.first);
> +		}
> +
> +		for (const std::string& filename : mipmap->image_files) {
> +			const Image* image = g_gr->images().get(filename);
> +			if (mipmap->frames.size() &&
> +				 (mipmap->frames[0]->width() != image->width() || mipmap->frames[0]->height() != image->height())) {
> +				throw wexception("wrong size: (%u, %u), should be (%u, %u) like the first frame",
> +									  image->width(), image->height(), mipmap->frames[0]->width(),
> +									  mipmap->frames[0]->height());
> +			}
> +			mipmap->frames.push_back(image);
> +		}
> +
> +		for (const std::string& filename : mipmap->pc_mask_image_files) {
> +			// TODO(unknown): Do not load playercolor mask as opengl texture or use it as
> +			//     opengl texture.
> +			const Image* pc_image = g_gr->images().get(filename);
> +			if (mipmap->frames[0]->width() != pc_image->width() || mipmap->frames[0]->height() != pc_image->height()) {
> +				// TODO(unknown): see bug #1324642
> +				throw wexception("playercolor mask has wrong size: (%u, %u), should "
> +									  "be (%u, %u) like the animation frame",
> +									  pc_image->width(), pc_image->height(), mipmap->frames[0]->width(),
> +									  mipmap->frames[0]->height());
> +			}
> +			mipmap->pcmasks.push_back(pc_image);
> +		}
>  	}
>  }
>  
>  float NonPackedAnimation::height() const {
>  	ensure_graphics_are_loaded();
> -	return frames_[0]->height() / scale_;
> +	return mipmaps_.at(1.0f)->frames.at(0)->height();
>  }
>  
>  uint16_t NonPackedAnimation::nr_frames() const {
> -	ensure_graphics_are_loaded();
> -	return frames_.size();
> +	return nr_frames_;
>  }
>  
>  uint32_t NonPackedAnimation::frametime() const {
> 
> === modified file 'src/logic/map_objects/map_object.cc'
> --- src/logic/map_objects/map_object.cc	2019-02-27 17:19:00 +0000
> +++ src/logic/map_objects/map_object.cc	2019-03-02 13:59:45 +0000
> @@ -255,11 +255,6 @@
>  			throw GameDataError("Map object %s has a menu icon, but it is empty", init_name.c_str());
>  		}
>  	}
> -	// TODO(GunChleoc): We can't scale down images in the font renderer yet, so we need an extra
> -	// representative image if the animation has high resolution.
> -	if (table.has_key("representative_image")) {
> -		representative_image_filename_ = table.get_string("representative_image");

Being able to use a different frame than the first one as representative image is very useful e.g. for ships under construction (compare in-game help for frisians_shipconstruction here and in trunk). I´d prefer to keep this feature around for such cases.

> -	}
>  	check_representative_image();
>  }
>  MapObjectDescr::~MapObjectDescr() {


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


References