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 hit bonus from BS_WEAPONRESEARCH being applied twice in pre-renewal #3221

Closed
wants to merge 2 commits into from

Conversation

wiryawanadipa
Copy link

@wiryawanadipa wiryawanadipa commented Jul 21, 2023

Pull Request Prelude

This condition is for renewal

For the pre-renewal bonus hit, the rate bonus is already calculated in status.c

If both are active (battle.c and status.c), in pre-renewal it will give +20 HIT (this is BS_WEAPONRESEARCH pre-renewal behavior) first and then add +20% accuracy after the hit rate is determined (this is BS_WEAPONRESEARCH renewal behavior).

For the pre-renewal bonus hit, just use the bonus from status.c

Note: Sorry for the previous commit, I made a mess. That was my first commit to other people's project.

Changes Proposed

  • Add renewal if condition for BS_WEAPONRESEARCH HIT bonus in battle.c since it's for renewal

Based on this wiki
https://irowiki.org/classic/Weaponry_Research
https://irowiki.org/wiki/Weaponry_Research

The renewal uses a percentage (+20% accuracy after the hit rate determine) while the pre-renewal use fix HIT number (+20 HIT)

Issues addressed:

  • Fix hit bonus from BS_WEAPONRESEARCH being applied twice in pre-renewal

this particular comments
// is this correct in pre? there is already hitrate bonus in battle.c
@skyleo skyleo self-assigned this Jul 21, 2023
@skyleo
Copy link
Contributor

skyleo commented Jul 21, 2023

Note: Sorry for the previous commit, I made a mess. That was my first commit to other people's project.

Don't worry about it, you're working on your own fork so no mess can bother us. :)

Although you could change the removing comment commit to Remove comment to fit our commit naming style.
And the line you touched could have applied our coding style to it:

if((skill_lv=pc->checkskill(sd,BS_WEAPONRESEARCH))>0)

->

if ((skill_lv = pc->checkskill(sd, BS_WEAPONRESEARCH)) > 0)

But don't worry about that. You might want to read up on Git's rebase feature and I suggest you to use the CLI for Git.
(It can be used to reword commits and edit them)

@MishimaHaruna MishimaHaruna added this to the Release v2023.08.09 milestone Aug 10, 2023
@skyleo
Copy link
Contributor

skyleo commented Aug 10, 2023

After researching this further in Aegis, I've come to the conclusion that this is indeed official behavior.

It adds a flat + 2 * skill_lv HIT which is seen in client too. And a hidden + 2 * skill_lv % on top of the final hitrate (Which is used internally).

Thus this PR should not be merged.

I thank you for your effort nontheless and I apologize for not having done thorough research prior to approving your PR.

It's used at CPC::GetAttackFinalDamage(...) in Aegis for the percentual bonus and in CPC::UpdateAttSucPercentOnClient(CPC *this with the flat bonus for anyone that wants to verify this.

@skyleo skyleo closed this Aug 10, 2023
@skyleo
Copy link
Contributor

skyleo commented Aug 10, 2023

If you think this is a logical bug in the official server software itself.

Feel free to rewrite your fix in such a way that it can be toggled in the battle configurations, in case one wants the Hercules behavior instead of the Aegis one.

@MishimaHaruna MishimaHaruna removed this from the Release v2023.08.09 milestone Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants