widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

some minor nits in the code.

This is way better encapsulated than before, but it still does not make me all warm and fuzzy inside - it feels weird that the animation passes out its calculated source rect, just to get it back in the blitting call. I do not have any more immediate suggestions how to make this better though.

Diff comments:

> 
> === modified file 'src/graphic/animation.cc'
> --- src/graphic/animation.cc	2016-10-26 10:56:02 +0000
> +++ src/graphic/animation.cc	2016-11-18 17:52:33 +0000
> @@ -256,20 +257,35 @@
>  	}
>  }
>  
> +Rectf NonPackedAnimation::source_rectangle(int percent_from_bottom) const {

const int per...

> +	ensure_graphics_are_loaded();
> +	float height = percent_from_bottom * frames_[0]->height() / 100;
> +	return Rectf(0.f, frames_[0]->height() - height, frames_[0]->width(), height);
> +}
> +
> +Rectf NonPackedAnimation::destination_rectangle(const Vector2f& position,
> +                                                const Rectf& source_rect,
> +                                                float scale) const {

const float

> +	ensure_graphics_are_loaded();
> +	return Rectf(position.x - (hotspot_.x - source_rect.x / scale_) * scale,
> +	             position.y - (hotspot_.y - source_rect.y / scale_) * scale,
> +	             source_rect.w * scale / scale_, source_rect.h * scale / scale_);
> +}
> +
>  void NonPackedAnimation::blit(uint32_t time,
> -                              const Rectf& dstrc,
> -                              const Rectf& srcrc,
> +                              const Rectf& source_rect,
> +                              const Rectf& destination_rect,
>                                const RGBColor* clr,
>                                Surface* target) const {
> +	ensure_graphics_are_loaded();
>  	assert(target);
> -
>  	const uint32_t idx = current_frame(time);
>  	assert(idx < nr_frames());
> -
>  	if (!hasplrclrs_ || clr == nullptr) {
> -		target->blit(dstrc, *frames_.at(idx), srcrc, 1., BlendMode::UseAlpha);
> +		target->blit(destination_rect, *frames_.at(idx), source_rect, 1., BlendMode::UseAlpha);
>  	} else {
> -		target->blit_blended(dstrc, *frames_.at(idx), *pcmasks_.at(idx), srcrc, *clr);
> +		target->blit_blended(
> +		   destination_rect, *frames_.at(idx), *pcmasks_.at(idx), source_rect, *clr);
>  	}
>  }
>  
> 
> === modified file 'src/graphic/animation.h'
> --- src/graphic/animation.h	2016-10-25 08:14:28 +0000
> +++ src/graphic/animation.h	2016-11-18 17:52:33 +0000
> @@ -57,8 +58,17 @@
>  	}
>  
>  	/// The dimensions of this animation.

Fix comment.

> -	virtual uint16_t width() const = 0;
> -	virtual uint16_t height() const = 0;
> +	virtual float height() const = 0;

I am not entirely sure, but I *think* this can be in a protected section now.

> +
> +	/// The size of the animation source images. Use 'percent_from_bottom' to crop the animation.

Units?

> +	virtual Rectf source_rectangle(int percent_from_bottom) const = 0;
> +
> +	/// Calculates the destination rectangle for blitting the animation.
> +	/// 'position' is where the top left corner of the animation will end up,
> +	/// 'source_rect' is the rectangle calculated by source_rectangle,
> +	/// 'scale' is the zoom scale.
> +	virtual Rectf
> +	destination_rectangle(const Vector2f& position, const Rectf& source_rect, float scale) const = 0;
>  
>  	/// The number of animation frames of this animation.
>  	virtual uint16_t nr_frames() const = 0;


-- 
https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/animation_scaling.