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

Refactor Auto Spell and move its config to libconfig #3237

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

guilherme-gm
Copy link
Member

Pull Request Prelude

Changes Proposed

This PR was mostly part of #3230 where I have refactored Auto Spell to make easier implement the renewal rebalance. Since these rebalance PRs are taking some time for me to finish and with Keikun's suggestion of moving the config to libconfig, I decided to make this as a separate PR so we can move this part earlier.

This PR moves Auto Spell skill list to LibConfig and reworks the listing/selection logic to remove the if hard to read if chains to something more generic. This way we can implement the renewal rebalance changes with a few lines instead of a huge ugly #if/#else blocks.

To avoid as much as possible from breaking something with this rewrite, I made some sort of "unit tests" for Auto Spell to try to cover as many cases as I could. You can check the testing code in this PR in my fork: https://github.com/guilherme-gm/Hercules/pull/9/files#diff-57e0249af7272bf23858146fc95e4674d48e2ffdf89e0824333cb54e232dd56d (Only PRE-RE version of it applies to this PR, and it should work for both pre-re and re)

It is kinda hackish, but this testing PR only extracts the functions to smaller ones with the same names as in this PR so I can test it using a plugin to run as many cases as possible. The same plugin can be used in this PR and it should pass for PRE-RE and RE (if you tweak to use pre-re version of the plugin).

About the decisions on the rewrite:

  • The decision of which skills to show was moved from clif to skill so skill logic stays on skill.c (as a bonus, this clif could now be used for other things, I guess)
  • Cast end logic was moved out of the huge skill castend function so we can have smaller and clearer functions (and also to make it testable and pluggable)
  • skill->autospell renamed to make it clear at which moment this function takes part
  • Everything now uses the same list of skills, so changing the list doesn't require changing 3 different places

If you are wondering how this will cover the rebalance PR, my idea is to add a constant that may be used in SkillLevel to indicate to use half of AutoSpell level. From my tests, it should be a simple change and quite clean.

Issues addressed:
None

src/map/skill.c Outdated Show resolved Hide resolved
for now, it is still not linked to the application
- split spell selection logic into smaller functions
- move spell list building to skill.c instead of clif
- use config file for autospell list
- replace if chain with array lookup/config file
- renamed skill_autospell to skill_autospell_spell_selected
@guilherme-gm guilherme-gm force-pushed the autospell-refactor branch 2 times, most recently from 8a3ca84 to 17d3acd Compare October 19, 2023 03:40
@MishimaHaruna MishimaHaruna merged commit 811d1fd into HerculesWS:master Oct 19, 2023
163 of 507 checks passed
@guilherme-gm guilherme-gm deleted the autospell-refactor branch October 22, 2023 03:58
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.

2 participants