-
-
Notifications
You must be signed in to change notification settings - Fork 418
Add army estimation mode option #9678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
oleg-derevenetz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @modo-lv I left a few comments regarding this PR, could you please to take a look when possible?
As for the idea of PR in general, we usually avoid introducing new settings if they cannot be done through the GUI, because on some platforms (that are considered first-class - for instance, on modern Android versions), manually editing the config is very difficult for an average user.
17def48 to
a57f858
Compare
a57f858 to
fb54d41
Compare
oleg-derevenetz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this PR looks OK, but I'd wait for @ihhub decision, because, as I already mentioned, there are certain requirements for new options.
|
I will review this PR later. Overall, I am not against this change as I've seen many players asking for this but I see that players won't like the idea of changing such a parameter inside a configuration file. It is not a user friendly approach. Therefore, this change must have a dedicated UI option. We don't need to implement it in this PR but at least have a brief idea how it should look like in the game and where to put it. I am tagging @Branikolog for this matter. |
|
@modo-lv , icons for the option must be suitable, not some random images. We need to create new ones. Having settled about the name of the option we should update the current source code as well. I think the option should be called something like "Army size view" and has 2 states: canonical and mumeric. What do you think @zenseii , @oleg-derevenetz and @Branikolog ? |
I agree. |
|
@ihhub. I agree. We shouldn't re-use these icons. Concepts for the new ones need to be come up with and then we can ask pixel artists for help. I think "army size view" would be interpreted as the exact size . How about "Army Size Range"? |
|
Hi!
Maybe we can stick to just two words: army size or army view. I think it would be enough. Also, if we are going to expand settings window to 3x2 we need to add one more option there. 😅 As for the icon itself, I personally think, that making a collage with several monsters sprites and toggling text (pack/"10-19") under them could work. |
|
Why not "army estimates"? Since it's not an exact size. |
|
I can see that we are stuck here. Let me make the review and propose the needed changes for the future UI. |
ihhub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @modo-lv , I added few comments here. Would you mind to take a look at them?
|
@ihhub and @Districh-ru, I'd appreciate your reviews on this. The placement of the option in the dialog is going to change because of #9971 so this is only temporary. |
zenseii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved now that the PR contains the changes I requested.
ihhub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zenseii , the changes look good to me. The only bit of doubt I have is the location of the option. I think it should belong to Adventure Map options.
|
@ihhub, I thought it might fit better there too. One problem is that we already have a lot of options there and can't add another line in the main page of the options: |
|
@zenseii , interface is a better place than the current one. @Districh-ru , what do you think? |
|
Hello everyone, Like @zenseii, I think "Army Range" would be more appropriate.
Personally, I would have chosen this brave Peasant. :D |
|
@LeHerosInconnu, while you're here, do you have any thoughts about the pixel-precision of the alignment of the creature and the army range? And yes I did prefer army range, but I'm willing to let it go to just get this PR wrapped up 😅 |
|
Hello @zenseii,
I would like to have a space of one pixel (or two) between the shadow of the chosen creature's sprite and the top of the numbers below, all centered horizontally and vertically within the frame area.
|
Districh-ru
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zenseii , interface is a better place than the current one.
@Districh-ru , what do you think?
I agree that the Interface settings is a better place for the setting of UI indication of creatures' count.
Hi @zenseii, I left one comment with suggestion and question about the need of making ICN::EMPTY_OPTION_ICON_BACKGROUND and moving the image processing to agg_image.cpp. Could you please take a look when you have time?
Would this still be the case if the new color palette that Districh made in #10309 is applied? Edit: maybe a Hydra is a better alternative? I chose a phoenix because it pops out against the purple background, but a yellow Hydra would do well too I think |
had to make it language dependent icn
I will move the option and icon to the interface settings. Unless there are disagreements this means it will not be in the system options.
I'm not familiar with the changes done, but my guess would be no.
I changed to the hydra. It looks good imo.
@LeHerosInconnu, I put 2 pixels distance between the creature sprite and the text to allow for accents in other languages. |
… "Few" translation
Districh-ru
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zenseii, all works fine, I just did some minor change:
- optimize the full image copy (to copy the whole image by one
memsetand set thesingleLayerflag from the input image); - update "few" -> "army|Few" to use the same text as in game with the already existent translations;
- remove the
_()from "1-4" because it don't need a translation :)
|
@modo-lv , @zenseii and @Districh-ru , thank you very much for making this feature happen! |







This PR adds a
numeric army size estimatessetting tofheroes2.cfgfile. When changed toon, army estimates in game will display number ranges ("10-19") instead of the original verbal descriptions ("lots").This has been requested in #1530, and I agree that it should be an option, however adding it to the UI turned out to be beyond my ability (at least for now). So this is not a prefect fix, useful for those who can manually edit their configuration files. But I believe this is better than nothing, and hopefully the UI parts can be added in the future.
Fix #1530