Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
Fixed in trunk. -- 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
It seems like this broke Travis. Gun, are you looking into this? -- 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. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/animation_scaling into lp:widelands has been updated. Status: Needs review => Merged For more details, see: 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
@bunnybot merge -- 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
Review: Approve height(): ack. the other answered inline. Diff comments: > > === modified file 'src/graphic/animation.h' > --- src/graphic/animation.h 2016-10-25 08:14:28 + > +++ src/graphic/animation.h 2016-11-18 17:52:33 + > @@ -57,8 +58,17 @@ > } > > /// The dimensions of this animation. > - virtual uint16_t width() const = 0; > - virtual uint16_t height() const = 0; > + virtual float height() const = 0; > + > + /// The size of the animation source images. Use 'percent_from_bottom' > to crop the animation. Sorry, I meant the units from the returned Rectangle. 'scale' seems fine. > + 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
Yep, I'm not really happy with it either, but I don't have a better idea. I added 2 replies to your comment, please have a look. Diff comments: > > === modified file 'src/graphic/animation.h' > --- src/graphic/animation.h 2016-10-25 08:14:28 + > +++ src/graphic/animation.h 2016-11-18 17:52:33 + > @@ -57,8 +58,17 @@ > } > > /// The dimensions of this animation. > - virtual uint16_t width() const = 0; > - virtual uint16_t height() const = 0; > + virtual float height() const = 0; This is used by Soldiers to position their health bar etc. > + > + /// The size of the animation source images. Use 'percent_from_bottom' > to crop the animation. The unit is %, as the variable name says. I don't know what the unit for scale is, shall we rename it to zoom_scale? > + 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
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 + > +++ src/graphic/animation.cc 2016-11-18 17:52:33 + > @@ -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 + > +++ src/graphic/animation.h 2016-11-18 17:52:33 + > @@ -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. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
Continuous integration builds have changed state: Travis build 1623. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/176793249. Appveyor build 1461. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_animation_scaling-1461. -- 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
Review: Resubmit Refactoring done, please have another look. - Moved all the sizing and positioning calculations into the animation class. - get_width() and hotspot() are gone. - get_height() is still used by the soldiers to position their health bar etc. - We now have source_rectangle and destination_rectangle functions to get rid of the rendertarget reference in the animation class - Killed some programming by exception -- 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
> I'd like to finish the spritemaps first though. agreed. > I also think it will only be worth our effort once we have the bigger > textures exported from Blender. No, I think the benefits will be immediately visible once you use downscaled textures. It just looks better. > [representative image] okay, as long as you have it on the radar and we agree that it has to die eventually :) About this change: I am still unhappy about it... Sorry :/. Now we have a circular dependency between RenderTarget and Surface. I feel the semantics break down in animation::get_width() and height. We have two dimensions now: the "perceived" (logical) one and the images on disk ones (physical) - the drawing code only knows about the first, and Animation should be the only knowing about the other. Can we not simply scale the source rect whenever blit_data() is requested? Other solution would be to not expose Animation width/height and instead deal in all code with relative quantities - i.e. a source rect of {w:0.5, h:1, x:0.25, y:0.} would mean: use the full height of the texture, but only 50% of the width starting at 25%. Diff comments: > > === modified file 'src/graphic/animation.cc' > --- src/graphic/animation.cc 2016-10-26 10:56:02 + > +++ src/graphic/animation.cc 2016-11-16 08:32:51 + > @@ -144,9 +146,19 @@ > 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 %f needs to be > 0.0f. > First image of this animation is %s", scale_, s/First image of/This animation has/ > +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); > } catch (const LuaError& e) { > throw wexception("Error in animation table: %s", e.what()); > } > > === modified file 'src/graphic/rendertarget.h' > --- src/graphic/rendertarget.h2016-10-17 20:39:15 + > +++ src/graphic/rendertarget.h2016-11-16 08:32:51 + > @@ -130,9 +130,10 @@ > return offset_; > } > > + bool to_surface_geometry(Rectf* destination_rect, Rectf* source_rect) > const; Add a comment what this does? > + > protected: > bool clip(Rectf& r) const; > - bool to_surface_geometry(Rectf* destination_rect, Rectf* source_rect) > const; > > // Does the actual blitting. > void do_blit_animation(const Vector2f& dst, -- 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
Review: Resubmit Good idea about the mipmaps - I'd like to finish the spritemaps first though. I also think it will only be worth our effort once we have the bigger textures exported from Blender. The representative_image in Lua is indeed needed for the font renderers. I really don't want to implement a new tag parameter into 2 font renderers for this right now. I have wanted Animation::representative_image_filename() to be gone for a while now so that we can have player colors on it too, but it will take some time until we get there. Since this function is only ever called for the idle animation, I decided to have the extra representative_image in MapObjectDescr rather than in the animation class. -- 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. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
This feels a bit messy. The scale of an animation must now be publicly knowable on the outside for doing math on it, though it feels like it should be a purely internal information. Can this not be internalized somehow by changing the animations interface? Also, if you want bigger images to look good at smaller zoom, we probably should enable and create mipmaps while loading them. Diff comments: > === modified file > 'data/tribes/buildings/productionsites/barbarians/shipyard/init.lua' > --- data/tribes/buildings/productionsites/barbarians/shipyard/init.lua > 2016-09-03 14:59:10 + > +++ data/tribes/buildings/productionsites/barbarians/shipyard/init.lua > 2016-11-13 16:57:34 + > @@ -7,6 +7,7 @@ > descname = pgettext("barbarians_building", "Shipyard"), > helptext_script = dirname .. "helptexts.lua", > icon = dirname .. "menu.png", > + representative_image = dirname .. "representative_image.png", So we are back shipping an individual image for the representative image? I understand that is for 'pics' in rich text, i.e. in messages and so on? > size = "medium", > needs_seafaring = true, > > > === modified file 'src/graphic/animation.cc' > --- src/graphic/animation.cc 2016-10-26 10:56:02 + > +++ src/graphic/animation.cc 2016-11-13 16:57:34 + > @@ -194,12 +208,12 @@ > > uint16_t NonPackedAnimation::width() const { > ensure_graphics_are_loaded(); > - return frames_[0]->width(); > + return frames_[0]->width() / scale_; These should now return float? > } > > uint16_t NonPackedAnimation::height() const { > ensure_graphics_are_loaded(); > - return frames_[0]->height(); > + return frames_[0]->height() / scale_; > } > > uint16_t NonPackedAnimation::nr_frames() const { > @@ -215,20 +229,17 @@ > return hotspot_; > } > > -Image* NonPackedAnimation::representative_image(const RGBColor* clr) const { > +const Image* NonPackedAnimation::representative_image(const RGBColor* clr) > const { I am confused: why do you need the representative_image lua option? > assert(!image_files_.empty()); > const Image* image = g_gr->images().get(image_files_[0]); > - > - if (!hasplrclrs_ || clr == nullptr) { > - // No player color means we simply want an exact copy of the > original image. > - const int w = image->width(); > - const int h = image->height(); > - Texture* rv = new Texture(w, h); > - rv->blit(Rectf(0, 0, w, h), *image, Rectf(0, 0, w, h), 1., > BlendMode::Copy); > - return rv; > - } else { > - return playercolor_image(clr, image, > g_gr->images().get(pc_mask_image_files_[0])); > + if (hasplrclrs_ && clr) { > + image = playercolor_image(clr, image, > g_gr->images().get(pc_mask_image_files_[0])); > } > + const int w = image->width(); > + const int h = image->height(); > + Texture* rv = new Texture(w / scale_, h / scale_); > + rv->blit(Rectf(0, 0, w / scale_, h / scale_), *image, Rectf(0, 0, w, > h), 1., BlendMode::Copy); > + return rv; > } > > const std::string& NonPackedAnimation::representative_image_filename() const > { -- https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/animation_scaling into lp:widelands. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
Continuous integration builds have changed state: Travis build 1601. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/175510561. Appveyor build 1439. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_animation_scaling-1439. -- https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/animation_scaling into lp:widelands. ___ 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
This looks very promising! -- https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/animation_scaling into lp:widelands. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/animation_scaling into lp:widelands
GunChleoc has proposed merging lp:~widelands-dev/widelands/animation_scaling into lp:widelands. Commit message: Implemented scaling for animations. Bigger idle textures for Barbarian warehouse and shipyard. Requested reviews: Widelands Developers (widelands-dev) For more details, see: https://code.launchpad.net/~widelands-dev/widelands/animation_scaling/+merge/310718 This implements scaling for animations, so it will look nice when zoomed in. I am having problems with the roof textures in the Blender models, but the warehouse and shipyard look good enough for testing. We will need spritemaps before we convert the whole lot, otherwise our file size will explode - but that's for another branch. -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/animation_scaling into lp:widelands. === modified file 'data/tribes/buildings/productionsites/barbarians/shipyard/idle_00.png' Binary files data/tribes/buildings/productionsites/barbarians/shipyard/idle_00.png 2014-12-13 20:54:34 + and data/tribes/buildings/productionsites/barbarians/shipyard/idle_00.png 2016-11-13 16:57:34 + differ === modified file 'data/tribes/buildings/productionsites/barbarians/shipyard/idle_00_pc.png' Binary files data/tribes/buildings/productionsites/barbarians/shipyard/idle_00_pc.png 2014-12-13 20:54:34 + and data/tribes/buildings/productionsites/barbarians/shipyard/idle_00_pc.png 2016-11-13 16:57:34 + differ === modified file 'data/tribes/buildings/productionsites/barbarians/shipyard/init.lua' --- data/tribes/buildings/productionsites/barbarians/shipyard/init.lua 2016-09-03 14:59:10 + +++ data/tribes/buildings/productionsites/barbarians/shipyard/init.lua 2016-11-13 16:57:34 + @@ -7,6 +7,7 @@ descname = pgettext("barbarians_building", "Shipyard"), helptext_script = dirname .. "helptexts.lua", icon = dirname .. "menu.png", + representative_image = dirname .. "representative_image.png", size = "medium", needs_seafaring = true, @@ -26,6 +27,7 @@ idle = { pictures = path.list_files(dirname .. "idle_??.png"), hotspot = { 62, 48 }, + scale = 3.26 }, build = { pictures = path.list_files(dirname .. "build_??.png"), === added file 'data/tribes/buildings/productionsites/barbarians/shipyard/representative_image.png' Binary files data/tribes/buildings/productionsites/barbarians/shipyard/representative_image.png 1970-01-01 00:00:00 + and data/tribes/buildings/productionsites/barbarians/shipyard/representative_image.png 2016-11-13 16:57:34 + differ === modified file 'data/tribes/buildings/warehouses/barbarians/warehouse/idle_00.png' Binary files data/tribes/buildings/warehouses/barbarians/warehouse/idle_00.png 2015-02-05 17:05:45 + and data/tribes/buildings/warehouses/barbarians/warehouse/idle_00.png 2016-11-13 16:57:34 + differ === modified file 'data/tribes/buildings/warehouses/barbarians/warehouse/idle_00_pc.png' Binary files data/tribes/buildings/warehouses/barbarians/warehouse/idle_00_pc.png 2015-02-05 17:05:45 + and data/tribes/buildings/warehouses/barbarians/warehouse/idle_00_pc.png 2016-11-13 16:57:34 + differ === modified file 'data/tribes/buildings/warehouses/barbarians/warehouse/init.lua' --- data/tribes/buildings/warehouses/barbarians/warehouse/init.lua 2015-12-28 21:53:11 + +++ data/tribes/buildings/warehouses/barbarians/warehouse/init.lua 2016-11-13 16:57:34 + @@ -7,6 +7,7 @@ descname = pgettext("barbarians_building", "Warehouse"), helptext_script = dirname .. "helptexts.lua", icon = dirname .. "menu.png", + representative_image = dirname .. "representative_image.png", size = "medium", buildcost = { @@ -26,7 +27,8 @@ animations = { idle = { pictures = path.list_files(dirname .. "idle_??.png"), - hotspot = { 60, 78 } + hotspot = { 60, 78 }, + scale = 3.62 }, build = { pictures = path.list_files(dirname .. "build_??.png"), === added file 'data/tribes/buildings/warehouses/barbarians/warehouse/representative_image.png' Binary files data/tribes/buildings/warehouses/barbarians/warehouse/representative_image.png 1970-01-01 00:00:00 + and data/tribes/buildings/warehouses/barbarians/warehouse/representative_image.png 2016-11-13 16:57:34 + differ === modified file 'doc/sphinx/source/animations.rst' --- doc/sphinx/source/animations.rst 2016-10-14 07:02:10 + +++ doc/sphinx/source/animations.rst 2016-11-13 16:57:34 + @@ -7,6 +7,7 @@ idle = { pictures = path.list_files(path.dirname(__file__) .. "idle_??.png"), hotspot = { 5, 7 }, + scale = 2.5, fps = 4, sound_effect = { directory = "sound/foo",