Quick monster upgrade in towns by a1exsh · Pull Request #4063 · ihhub/fheroes2

a1exsh · 2021-08-22T13:53:40Z

If an army troop is located in a town that provides a possibility to upgrade
them, display a quick upgrade button on the troop sprite. Clicking the button
invokes the upgrade troop dialog directly.

If an army troop is located in a town that provides a possibility to upgrade them, display a quick upgrade button on the troop sprite. Clicking the button invokes the upgrade troop dialog directly.

a1exsh · 2021-08-22T13:54:24Z

To my taste, this change touches a bit too many places, but I haven't found a more direct way to achieve the same effect so far.

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/3)

src/fheroes2/gui/interface_itemsbar.h

src/fheroes2/castle/buildinginfo.cpp

src/fheroes2/castle/buildinginfo.h

src/fheroes2/gui/skill_bar.cpp

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (2/3)

src/fheroes2/gui/skill_bar.h

src/fheroes2/castle/buildinginfo.h

src/fheroes2/army/army_bar.h

src/fheroes2/army/army_bar.cpp

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (3/3)

src/fheroes2/resource/artifact.cpp

src/fheroes2/resource/artifact.h

a1exsh · 2021-08-22T13:57:40Z

Screenshot:

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/fheroes2/castle/buildinginfo.h

ihhub · 2021-08-22T14:09:40Z

Hi @a1exsh , I like your great enthusiasm about new ideas but we usually make a discussion at least between members of our team before implementing new things. Please next time propose your idea first in discussion topic :)

@Branikolog , @oleg-derevenetz and @LeHerosInconnu, could you please check this change and express your opinion about it?

Update parameter name

a1exsh · 2021-08-22T14:11:08Z

@ihhub I'm just using it as an opportunity to study the code ;) I'm going to play a locally modified version for my convenience anyway

oleg-derevenetz · 2021-08-22T14:26:16Z

@ihhub @a1exsh Having a working prototype is much better than having just an idea. However, it may happen that the prototype or even the idea itself will not be accepted by the upstream. The author should be ready for this :)

LeHerosInconnu · 2021-08-22T14:40:34Z

Hello everyone,

Hi @a1exsh , I like your great enthusiasm about new ideas but we usually make a discussion at least between members of our team before implementing new things. Please next time propose your idea first in discussion topic :)

@Branikolog , @oleg-derevenetz and @LeHerosInconnu, could you please check this change and express your opinion about it?

It is not yet possible to try the change.
But I completely agree with this improvement as it was already on my list of ideas not yet proposed. :D

Branikolog · 2021-08-22T15:00:30Z

Hi all.
The proposal to fast upgrade all units is not bad at all, but we firstly should focus on fixing existing bugs, than adding numerous new features.
Also, this feature should be relevant to castle GUI, which is not a trivial without artists.
We can leave this feature for some hot key in a castle view window, if it doesn't break anything.

LeHerosInconnu · 2021-08-22T15:17:05Z

Hello @Branikolog,

Hi all.
The proposal to fast upgrade all units is not bad at all, but we firstly should focus on fixing existing bugs, than adding numerous new features.
Also, this feature should be relevant to castle GUI, which is not a trivial without artists.
We can leave this feature for some hot key in a castle view window, if it doesn't break anything.

This is not to upgrade all creatures at once, but to visually indicate which troops can be upgraded.
The player can then choose whether or not to upgrade them one by one.

Branikolog · 2021-08-22T15:32:57Z

This is not to upgrade all creatures at once, but to visually indicate which troops can be upgraded.
The player can then choose whether or not to upgrade them one by one.

Oh, sorry for my slight misunderstanding...
Anyway, we can add thousands of such nice features, but without any professional artist, who would add such elements to GUI without ruining game style, the majority of them could be left unimplemented.

a1exsh · 2021-08-22T18:46:08Z

Yes, what is implemented here is a small "up" arrow in the top-bottom corner of an upgradable troop. Clicking on that arrow opens the upgrade dialog directly without going through army dialog first where one needs to click Upgrade button (it saves 2 clicks).

src/fheroes2/dialog/dialog_armyinfo.cpp

LeHerosInconnu · 2021-08-24T16:47:50Z

Hello everyone,

So, I did some tests.
Everything seems to work fine.
Good job on the singular plural for text for the information about the function of the button in the bottom bar depending on the number of creatures and also on the message when the player doesn't have enough resources to upgrade a troop. :)

Some suggestions and possible improvements (can be done in another PR for some of them).

display the graphic in the upper left corner to avoid the arrow to cover the sprite of the creature, in the examples with Dragons, Cavalry and Paladin.
in the TOWNS/CASTLES screen, also display the graphic in the upper left corner, it would be also better to use a smaller graphic in this screen.
for the Green Dragon, it should be possible either to add a second graphic below the first one to allow the Green Dragon to be upgraded directly to Black Dragon or to add at least one button that allows this in the validation window.
a color code for the graphic could also be used to visually indicate to the player if he can upgrade the troop directly (yellow arrow), if he can upgrade the troop provided he exchanges resources (orange arrow), if he cannot upgrade the troop even by exchanging resources (red arrow).

a1exsh · 2021-08-24T18:43:13Z

@LeHerosInconnu thanks for having a look: you raise a number of good points!

It occurs to me now that all monsters in Heroes II are looking to the right, so probably it makes sense to move the quick upgrade button to the top left corner.
Now I see where the mini army bar is used: in kingdom overview. Haven't found this in the code. 😅

Will check the green -> black dragon interaction. I also like the idea to give a (color) hint when upgrade is not possible due to lack of resources.

LeHerosInconnu · 2021-08-24T20:20:53Z

Hello @a1exsh,

Now I see where the mini army bar is used: in kingdom overview. Haven't found this in the code.

You made me think that a mini creature army bar is also displayed in the hero meeting screen.

In the following example, Rebecca is located in a castle where the Irons Golems can be upgraded.

But during the meeting with another hero (Rebecca stays in the castle), the graphic for upgrading the Iron Golems is not displayed, while it is possible to upgrade the Iron Golems with the default method.
The upgrade graphics should also be displayed in the hero meeting screen.

Edit:

The Iron Golem can still be upgraded if the player clicks where the upgrade graphic should be displayed.

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

github-actions · 2021-08-25T07:05:20Z

src/fheroes2/army/army_bar.cpp


+const fheroes2::Sprite ArmyBar::GetUpgradeButton()
+{
+    static const fheroes2::Sprite upButton = fheroes2::AGG::GetICN( ICN::RECRUIT, 0 );


⚠️ readability-const-return-type ⚠️
return type const fheroes2::Sprite is const-qualified at the top level, which may reduce code readability without improving const correctness

Suggested change

static const fheroes2::Sprite upButton = fheroes2::AGG::GetICN( ICN::RECRUIT, 0 );

static const fheroes2::Sprite upButton oes2::AGG::GetICN( ICN::RECRUIT, 0 );

Oh, these "suggested changes"... Sometimes they are completely broken. I'll have to take a look sometime.

Hi @ihhub I (hopefully) fixed such issues with suggested changes, corresponding PR is merged to the upstream, you may want to sync fheroes2/clang-tidy-pr-comments with the upstream.

src/fheroes2/army/army_bar.cpp

a1exsh · 2021-08-25T16:58:28Z

Thanks for the review, @ihhub and @LeHerosInconnu !

I've addressed the code comments and also made the clickable area of the button a bit bigger, as suggested. I will also try to see if we can cast a shadow under the button to make it stand out a little more, like on the actual "recruit troop" dialog.

ihhub · 2021-08-26T04:26:18Z

Thanks for the review, @ihhub and @LeHerosInconnu !

I've addressed the code comments and also made the clickable area of the button a bit bigger, as suggested. I will also try to see if we can cast a shadow under the button to make it stand out a little more, like on the actual "recruit troop" dialog.

Making a shadow is very simple. We already have a function to do this. Please check agg_image.cpp file for examples.

a1exsh · 2021-08-26T06:28:41Z

The shadow produced by addShadow() is really hard to see, or maybe I'm doing something wrong. Let's skip this for now.

src/fheroes2/army/army_bar.cpp

oleg-derevenetz · 2021-08-28T17:48:31Z

@a1exsh if you sync this PR with master branch, quick upgrades should work for guardian heroes armies too.

ihhub

Hi @a1exsh , I left just 2 more comments here. Would you mind to take a look?

I think we still need to make it as a button. I'll check in a while how to render such properly.

ihhub · 2021-09-02T10:23:03Z

src/fheroes2/army/army_bar.cpp

    return _army != nullptr;
 }

+const fheroes2::Sprite & ArmyBar::GetUpgradeButton() const


I think this method should be static instead of const as we can have multiple ArmyBar instances but all of them use the same resource. The static method could have a single boolean argument useMiniIcon as a replacement of use_mini_sprite member usage. In this case we can avoid memory duplication.

Also this method should be private as it's not used anywhere outside the class.

I guess there was no memory duplication either way, because of static storage used inside the function. Moved it to private namespace, I hope that also works with you? :)

ihhub · 2021-09-02T10:24:57Z

src/fheroes2/army/army_bar.h


+    const fheroes2::Sprite & GetUpgradeButton() const;
+    fheroes2::Rect GetUpgradeButtonPos( const fheroes2::Rect & itemPos ) const;
+    void DrawUpgadeButton( const fheroes2::Rect & pos, fheroes2::Image & dstsf ) const;


Could we please rename (copy-pasted) dstsf variable into something like outputImage to be more obvious? dstsf was standing for destination surface but here we don't have surfaces anymore :)

a1exsh · 2021-09-06T07:08:55Z

I think we still need to make it as a button. I'll check in a while how to render such properly.

@ihhub in the OG editor there is a clickable button (it has a pressed state image as well) for drop down menus:

I guess it should be part of the game resources, so we could use it here if we mirrored it vertically.

Do you by chance know the icon name for this one?

ihhub · 2021-09-06T10:30:32Z

I think we still need to make it as a button. I'll check in a while how to render such properly.

@ihhub in the OG editor there is a clickable button (it has a pressed state image as well) for drop down menus:

I guess it should be part of the game resources, so we could use it here if we mirrored it vertically.

Do you by chance know the icon name for this one?

ICN::LISTBOX is what you're looking for.

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

github-actions · 2021-09-08T07:28:26Z

src/fheroes2/gui/interface_itemsbar.h

        }

-        virtual bool ActionBarLeftMouseHold( Item &, Item & )
+        virtual bool ActionBarLeftMouseHold( const fheroes2::Point & /*unused*/, Item & /*unused*/, const fheroes2::Rect & /*unused*/ )


⚠️ clang-diagnostic-inconsistent-missing-override ⚠️
ActionBarLeftMouseHold overrides a member function but is not marked override

github-actions · 2021-09-08T07:28:26Z

src/fheroes2/gui/interface_itemsbar.h

        }

-        virtual bool ActionBarLeftMouseHold( Item & )
+        virtual bool ActionBarLeftMouseHold( Item &, Item & )


⚠️ readability-named-parameter ⚠️
all parameters should be named in a function

Suggested change

virtual bool ActionBarLeftMouseHold( Item &, Item & )

virtual bool ActionBarLeftMouseHold( Item & /*unused*/, Item & )

github-actions · 2021-09-08T07:28:26Z

src/fheroes2/gui/interface_itemsbar.h

        }

-        virtual bool ActionBarLeftMouseHold( Item & )
+        virtual bool ActionBarLeftMouseHold( Item &, Item & )


⚠️ readability-named-parameter ⚠️
all parameters should be named in a function

Suggested change

virtual bool ActionBarLeftMouseHold( Item &, Item & )

virtual bool ActionBarLeftMouseHold( Item &, Item & /*unused*/)

a1exsh · 2021-09-08T07:32:14Z

Updated the button icon, thanks for the hint, @ihhub ! :-)

In order to render pressed and released state I had to further extend ActionBarLeftMouseHold with cursor and item position parameters, though this approach doesn't seem to scale really well.

I see that we have a special class for the buttons here: https://github.com/ihhub/fheroes2/blob/master/src/fheroes2/gui/ui_button.h#L99
Using it, however, requires fixed position on the screen. For us to use it in the ArmyBar, we would need to keep a mapping between the troops and the buttons. That would also remove the need to pass item position in the Click/Hold event handlers.

I will have a look later.

…-monster-upgrade

The proper way is to use `makeButtonWithShadow()`, but for that we will need real buttons, not just stray sprites.

GeorgeK1ng · 2023-03-27T12:36:48Z

Wow! I really like these improvements!

Quick monster upgrade in towns

c1fd654

If an army troop is located in a town that provides a possibility to upgrade them, display a quick upgrade button on the troop sprite. Clicking the button invokes the upgrade troop dialog directly.

github-actions bot assigned a1exsh Aug 22, 2021

github-actions bot reviewed Aug 22, 2021

View reviewed changes

Apply suggestions from code review

b0cf669

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot reviewed Aug 22, 2021

View reviewed changes

src/fheroes2/castle/buildinginfo.h Outdated Show resolved Hide resolved

Update src/fheroes2/castle/buildinginfo.h

fc115cf

Update parameter name

fix formatting after suggestions from clang-tidy

d7eca3c

one more

bcb27a8

oleg-derevenetz added improvement New feature, request or improvement ui UI/GUI related stuff labels Aug 22, 2021

ihhub reviewed Aug 23, 2021

View reviewed changes

src/fheroes2/dialog/dialog_armyinfo.cpp Outdated Show resolved Hide resolved

fix implicit bool cast

ee1c4b7

a1exsh added 2 commits August 25, 2021 08:38

resize upgrade button for mini sprite display

d892003

draw upgrade button when heroes are meeting each other

986791f

github-actions bot reviewed Aug 25, 2021

View reviewed changes

a1exsh requested a review from ihhub August 26, 2021 07:28

ihhub reviewed Aug 26, 2021

View reviewed changes

src/fheroes2/army/army_bar.cpp Outdated Show resolved Hide resolved

a1exsh added 2 commits August 26, 2021 21:17

rename constant in camelCase

afc6506

Merge branch 'master' into quick-monster-upgrade

5fbe9a0

Merge branch 'master' into quick-monster-upgrade

be65b5c

a1exsh requested a review from ihhub September 1, 2021 06:58

Add missing const which is present in source file

59858d2

ihhub requested changes Sep 2, 2021

View reviewed changes

Address comments from review

a081d34

ihhub added this to the 1.0 milestone Sep 5, 2021

a1exsh added 2 commits September 7, 2021 08:40

use a proper button icon

d5aebc7

draw pressed and released states for the upgrade button

47673b6

github-actions bot reviewed Sep 8, 2021

View reviewed changes

a1exsh added 6 commits September 8, 2021 09:36

fix missing override

18ae48c

s/normal/pressed/

b4b3fc5

fix parameter names

cb413f1

Merge branch 'master' of https://github.com/ihhub/fheroes2 into quick…

97683ef

…-monster-upgrade

fix compilation after merge

5c8725b

remove shadow: it should be done differently

1576369

The proper way is to use `makeButtonWithShadow()`, but for that we will need real buttons, not just stray sprites.

ihhub modified the milestones: 1.0, Beyond 1.0 Dec 19, 2022

ihhub marked this pull request as draft July 22, 2023 03:25

	static const fheroes2::Sprite upButton = fheroes2::AGG::GetICN( ICN::RECRUIT, 0 );
	static const fheroes2::Sprite upButton oes2::AGG::GetICN( ICN::RECRUIT, 0 );

	virtual bool ActionBarLeftMouseHold( Item &, Item & )
	virtual bool ActionBarLeftMouseHold( Item & /unused/, Item & )

Uh oh!

Conversation

a1exsh commented Aug 22, 2021

Uh oh!

a1exsh commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

a1exsh commented Aug 22, 2021

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ihhub commented Aug 22, 2021

Uh oh!

a1exsh commented Aug 22, 2021

Uh oh!

oleg-derevenetz commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeHerosInconnu commented Aug 22, 2021

Uh oh!

Branikolog commented Aug 22, 2021

Uh oh!

LeHerosInconnu commented Aug 22, 2021

Uh oh!

Branikolog commented Aug 22, 2021

Uh oh!

a1exsh commented Aug 22, 2021

Uh oh!

Uh oh!

LeHerosInconnu commented Aug 24, 2021

Uh oh!

a1exsh commented Aug 24, 2021

Uh oh!

LeHerosInconnu commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

oleg-derevenetz Aug 25, 2021

a1exsh commented Aug 22, 2021 •

edited

Loading

oleg-derevenetz commented Aug 22, 2021 •

edited

Loading

LeHerosInconnu commented Aug 24, 2021 •

edited

Loading

ihhub commented Aug 26, 2021 •

edited

Loading

oleg-derevenetz commented Aug 28, 2021 •

edited

Loading