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

Clean up auto-cast related code #2657

Merged
merged 21 commits into from
Apr 5, 2020

Conversation

Kenpachi2k13
Copy link
Member

@Kenpachi2k13 Kenpachi2k13 commented Mar 13, 2020

Pull Request Prelude

Changes Proposed

Currently the auto-cast related code is a mess.

  • We have map_session_data->state.autocast which is used to temporarily flag a skill as auto-cast for skills cast by bAutoSpell* bonuses or by Auto Shadow Spell. It's also (mis)used to prevent damage reflection from infinite looping.
  • We have map_session_data->state.abra_flag which isn't actually used. It's just set when casting Abracadabra or Improvised Song, but then never used for validation.
  • We have map_session_data->skillitem and map_session_data->skillitemlv which hold the auto-cast skill's ID and level. map_session_data->skillitem is misused to validate auto-casting all over the code.
  • As of late we have the itemskill() script command related states and helper variables.
  • Unsetting/validating the auto-cast data in the right place currently is more likely gambling.

------------------------------------------------------------------------------------------------------------------

I spent some days in getting into the subject and cleaned it up as far as possible.

  • I added an enumeration for auto-cast types to be able to handle each one separately and use those types for validation instead of the auto-cast skill's ID.
  • I added a structure to unify all auto-cast related data in one place.
  • I added a function which validates the plausibility of auto-cast related data.
  • I revised where and when pc_autocast_clear() (former pc_itemskill_clear()) gets called.
  • I removed obsolete code parts.
  • I made skills cast by Improvised Song ignore all requirements.

Closes #1211

@Kenpachi2k13 Kenpachi2k13 added type:enhancement Issue describes an enhancement or feature that should be implemented status:code-review Awaiting code review component:mechanics:skills Affecting the skills' game mechanics labels Mar 13, 2020
@Kenpachi2k13 Kenpachi2k13 added this to the Release v2020.04.05 milestone Mar 13, 2020
@Kenpachi2k13 Kenpachi2k13 self-assigned this Mar 13, 2020
@Kenpachi2k13 Kenpachi2k13 linked an issue Mar 13, 2020 that may be closed by this pull request
src/map/pc.h Show resolved Hide resolved
src/map/skill.c Outdated Show resolved Hide resolved
First of all: In official servers, skill casting item are consumed immediately and thus IT_DELAYCONSUME should not be used for those items
And additionally these code blocks are obsolete, because of the way how skill casting items work.
 * If the item won't check the skill's requirements, the code block to delete the item of type IT_DELAYCONSUME isn't even executed.
 * If the item does check the skill's requirements, the check is done prior to the skill casting which would be the same as using IT_USABLE.
@Kenpachi2k13 Kenpachi2k13 requested a review from 4144 March 14, 2020 21:35
@MishimaHaruna MishimaHaruna merged commit 024c9c4 into HerculesWS:master Apr 5, 2020
@Kenpachi2k13 Kenpachi2k13 deleted the autocast_clean_up branch April 5, 2020 19:10
MishimaHaruna added a commit to MishimaHaruna/Hercules that referenced this pull request Apr 6, 2021
This commit introduces various fixes to the Abracadabra / Improvise
skill validation:

- autocast code is moved to after homun/merc skill ranges so that when
  any of those are triggered, autocast data is not purged
- abracadabra/improvise is no longer purged immediately after cast, by
  calling autocast_clear_current in the proper manner
- the abracadabra/improvise skill requirement bypass that was removed in
  HerculesWS#2657 and related pull requests is re-implemented
- the condition that would get the character stuck and unable to cast
  other skills until teleporting or relogging when rolling AL_WARP from
  abracadabra (i.e. because AL_WARP lands in clif_parse_UseSkillMap in 2
  steps) is fixed, by not immediately clearing the data after the first
  one

All credits for the fix go to Heka of Origins

Related to HerculesWS#2859
Fixes HerculesWS#2823
Fixes HerculesWS#2824

Signed-off-by: Haru <[email protected]>
MishimaHaruna added a commit to MishimaHaruna/Hercules that referenced this pull request Apr 6, 2021
This commit introduces various fixes to the Abracadabra / Improvise
skill validation:

- abracadabra/improvise is no longer purged immediately after cast, by
  calling autocast_clear_current in the proper manner
- the abracadabra/improvise skill requirement bypass that was removed in
  HerculesWS#2657 and related pull requests is re-implemented
- the condition that would get the character stuck and unable to cast
  other skills until teleporting or relogging when rolling AL_WARP from
  abracadabra (i.e. because AL_WARP lands in clif_parse_UseSkillMap in 2
  steps) is fixed, by not immediately clearing the data after the first
  one

All credits for the fix go to Heka of Origins

Related to HerculesWS#2859
Fixes HerculesWS#2823
Fixes HerculesWS#2824

Signed-off-by: Haru <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:mechanics:skills Affecting the skills' game mechanics status:code-review Awaiting code review type:enhancement Issue describes an enhancement or feature that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvised Song not working
3 participants