Skip to content
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

Fix the mechanics of Genie special ability #8411

Merged
merged 8 commits into from
Feb 20, 2024

Conversation

oleg-derevenetz
Copy link
Collaborator

Fix part of #8409 regarding a bug in the game logic.

@oleg-derevenetz oleg-derevenetz marked this pull request as draft February 15, 2024 20:38
@oleg-derevenetz oleg-derevenetz added bug Something doesn't work logic Things related to game logic labels Feb 15, 2024
@oleg-derevenetz oleg-derevenetz added this to the 1.1.0 milestone Feb 15, 2024
@Alucard648
Copy link

The string that logs Genies ability to insta-kill half of enemy stack should be all caps to empasize extreme nature of creature`s absolute destruction.

@oleg-derevenetz
Copy link
Collaborator Author

The string that logs Genies ability to insta-kill half of enemy stack should be all caps to empasize extreme nature of creature`s absolute destruction.

It contains an exclamation mark at the end :) In any case, this PR is intended to fix the logic of the Genie ability. I don't know for sure why is this message displayed in its current form, but I suppose that this can be corrected/improved later separately if there is such a need.

@ihhub
Copy link
Owner

ihhub commented Feb 16, 2024

I am sure @Branikolog is going to fight against changing Genie's special ability chances :)

@oleg-derevenetz
Copy link
Collaborator Author

Hi @ihhub

I am sure @Branikolog is going to fight against changing Genie's special ability chances :)

But they were not changed. _randomGenerator.Get( 1, 10 ) == 2 in the master branch is exactly 10% (one chance in 10). If only there was a <= comparison instead of ==, then it would be a different matter.

@oleg-derevenetz oleg-derevenetz marked this pull request as ready for review February 16, 2024 11:39
Copy link
Collaborator

@Branikolog Branikolog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything works nice!
I hope chances for applying halving were left the same as before.

@oleg-derevenetz
Copy link
Collaborator Author

Hi @Branikolog

I hope chances for applying halving were left the same as before.

By the way, this is an ambiguous question :) According to the game manual and in fact it was 10%, but it was shown in unit's properties as 20%. Now it is in fact 10% and is shown as 10%.

@Branikolog
Copy link
Collaborator

Hello, @oleg-derevenetz

Hi @Branikolog

I hope chances for applying halving were left the same as before.

By the way, this is an ambiguous question :) According to the game manual and in fact it was 10%, but it was shown in unit's properties as 20%. Now it is in fact 10% and is shown as 10%.

There was a discussion 1+year ago regarding genies ability. Some players complained, that in fheroes2 "halving" occurred too rare, comparing to the original game. After making 150+ fights I came to conclusion, that in the original game the ability triggered in more than 15% attacks. So that's why it was set to 20% in fheroes2.
I'm not sure we should roll this change back, since players got used to more frequent triggering and the overall weakness of this troop without this special ability.

@oleg-derevenetz
Copy link
Collaborator Author

oleg-derevenetz commented Feb 20, 2024

@Branikolog the funny thing is that change to 20% was obviously made in wrong way. Currently in the master branch the logic looks like this: if ( _randomGenerator.Get( 1, 10 ) == 2) {... perform the halving...}, i.e. some random number in the range of 1...10 is taken and if this number is 2 (not less than or equal to 2!) then the halving is performed, so currently in the master branch the chance is still 10%. So in fact this PR changes nothing in regard to actual chance to trigger this ability, it just fixes the wrong chance displayed in the unit's properties. I can make the actual chance (as well as displayed chance) 20% if you wish.

@ihhub ihhub merged commit 7c5efe9 into ihhub:master Feb 20, 2024
20 checks passed
@ihhub
Copy link
Owner

ihhub commented Feb 20, 2024

@oleg-derevenetz , thank you so much for this update!

@oleg-derevenetz oleg-derevenetz deleted the genie-fix branch February 20, 2024 13:01
@Branikolog
Copy link
Collaborator

@oleg-derevenetz

@Branikolog the funny thing is that change to 20% was obviously made in wrong way. Currently in the master branch the logic looks like this: if ( _randomGenerator.Get( 1, 10 ) == 2) {... perform the halving...}, i.e. some random number in the range of 1...10 is taken and if this number is 2 (not less than or equal to 2!) then the halving is performed, so currently in the master branch the chance is still 10%. So in fact this PR changes nothing in regard to actual chance to trigger this ability, it just fixes the wrong chance displayed in the unit's properties. I can make the actual chance (as well as displayed chance) 20% if you wish.

To be honest, I don't remember, whether I've tested triggering chances in fheroes2. I was mainly focused on testing the original game leaving the implementation in fheroes2 to the development/contribution team.

I'm a bit busy this week, so I cannot perform deep tests. But I can try to make another set of Genies tests in the OG later to be assured, that 20% is an actual chance for triggering.

@oleg-derevenetz
Copy link
Collaborator Author

Hi @Branikolog

I'm a bit busy this week, so I cannot perform deep tests. But I can try to make another set of Genies tests in the OG later to be assured, that 20% is an actual chance for triggering.

Currently with this PR the chance of triggering is 10%, and it was 10% before merging of this PR too due to an implementation error. Judging by the fact that no one actually noticed this error, it seems that 10% is fine. I can change it to 20% if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something doesn't work logic Things related to game logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants