From 1a1262ffb5195e0ced2c84b3b0f6232b22bbef47 Mon Sep 17 00:00:00 2001 From: "Guilherme G. Menaldo" Date: Sat, 27 Apr 2024 16:00:56 -0300 Subject: [PATCH 1/7] Fix skill index check for Auto Shadow Spell the code was not taking into account cases where the player simply didn't had a skill in reproduce or clone slot. --- src/map/skill.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/map/skill.c b/src/map/skill.c index 4346a6c96c3..1ad686a359b 100644 --- a/src/map/skill.c +++ b/src/map/skill.c @@ -10445,15 +10445,19 @@ static int skill_castend_nodamage_id(struct block_list *src, struct block_list * } break; case SC_AUTOSHADOWSPELL: - if( sd ) { - int idx1 = skill->get_index(sd->reproduceskill_id), idx2 = skill->get_index(sd->cloneskill_id); - if( sd->status.skill[idx1].id || sd->status.skill[idx2].id ) { + if (sd != NULL) { + int reproduceIdx = sd->reproduceskill_id > 0 ? skill->get_index(sd->reproduceskill_id) : -1; + int cloneIdx = sd->cloneskill_id > 0 ? skill->get_index(sd->cloneskill_id) : -1; + + bool hasReproduceSkill = reproduceIdx >= 0 && sd->status.skill[reproduceIdx].id != 0; + bool hasCloneSkill = cloneIdx >= 0 && sd->status.skill[cloneIdx].id != 0; + if (hasReproduceSkill || hasCloneSkill) { sc_start(src, src, SC_STOP, 100, skill_lv, INFINITE_DURATION, skill_id); // The skill_lv is stored in val1 used in skill_select_menu to determine the used skill lvl [Xazax] clif->autoshadowspell_list(sd); - clif->skill_nodamage(src,bl,skill_id,1,1); - } - else + clif->skill_nodamage(src, bl, skill_id, 1, 1); + } else { clif->skill_fail(sd, skill_id, USESKILL_FAIL_IMITATION_SKILL_NONE, 0, 0); + } } break; From e78bfbc4fe98dca34b9e5219476a72bed4225fe1 Mon Sep 17 00:00:00 2001 From: "Guilherme G. Menaldo" Date: Sat, 27 Apr 2024 16:02:22 -0300 Subject: [PATCH 2/7] Fix Auto Shadow Spell not allowing to switch to skills of lower skill_id Due to the skill id being stored in val1, the default logic for SCs is that lower val1 values doesn't replace higher ones, so recasting Auto Shadow Spell to use another skill which has a smaller skill_id was failing, while it should be allowed. --- src/map/status.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/map/status.c b/src/map/status.c index 476032fb30f..04a8baa131a 100644 --- a/src/map/status.c +++ b/src/map/status.c @@ -7418,6 +7418,7 @@ static int status_change_start_sub(struct block_list *src, struct block_list *bl case SC_RESIST_PROPERTY_WIND: case SC_FLASHKICK: case SC_SOULUNITY: + case SC__AUTOSHADOWSPELL: // otherwise you can't change your shadow spell to a lower skill_id break; case SC_GOSPEL: //Must not override a casting gospel char. From 6d0b14d4a8ef2e0d1fa8ef23232cc6f18820af22 Mon Sep 17 00:00:00 2001 From: "Guilherme G. Menaldo" Date: Sun, 28 Apr 2024 14:59:12 -0300 Subject: [PATCH 3/7] Convert PACKET_ZC_SKILL_SELECT_REQUEST to packet struct --- src/map/clif.c | 42 ++++++++++++++++++++++++++++------------ src/map/packets_struct.h | 10 +++++++++- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/map/clif.c b/src/map/clif.c index 1432abd717a..da06447a026 100644 --- a/src/map/clif.c +++ b/src/map/clif.c @@ -20828,32 +20828,49 @@ static int clif_poison_list(struct map_session_data *sd, uint16 skill_lv) return 1; } + static int clif_autoshadowspell_list(struct map_session_data *sd) { - int fd, i, c; nullpo_ret(sd); - fd = sd->fd; - if( !fd ) return 0; - if( sd->menuskill_id == SC_AUTOSHADOWSPELL ) + int fd = sd->fd; + if (fd == 0) + return 0; + + if (sd->menuskill_id == SC_AUTOSHADOWSPELL) return 0; - WFIFOHEAD(fd, 2 * 6 + 4); - WFIFOW(fd,0) = 0x442; - for (i = 0, c = 0; i < MAX_SKILL_DB; i++) + // Max number of skills shown. This number should never go above 2, but let's leave some space for customization. + const int max_count = 10; + + struct PACKET_ZC_SKILL_SELECT_REQUEST *p; + int len = max_count * sizeof(*p->skillIds); + WFIFOHEAD(fd, len); + p = WFIFOP(fd, 0); + p->packetType = HEADER_ZC_SKILL_SELECT_REQUEST; + + int c = 0; + for (int i = 0; i < MAX_SKILL_DB && c < max_count; i++) { if (sd->status.skill[i].flag == SKILL_FLAG_PLAGIARIZED && sd->status.skill[i].id > 0 && sd->status.skill[i].id < GS_GLITTERING && skill->get_type(sd->status.skill[i].id, sd->status.skill[i].lv) == BF_MAGIC) { // Can't auto cast both Extended class and 3rd class skills. - WFIFOW(fd,8+c*2) = sd->status.skill[i].id; + p->skillIds[c] = sd->status.skill[i].id; c++; } + } - if( c > 0 ) { - WFIFOW(fd,2) = 8 + c * 2; - WFIFOL(fd,4) = c; - WFIFOSET(fd,WFIFOW(fd,2)); + if (c == max_count) + ShowError("%s: max_count shadow spells was reached, some skills may not be shown.\n", __func__); + + if (c > 0) { sd->menuskill_id = SC_AUTOSHADOWSPELL; sd->menuskill_val = c; + + len = c * sizeof(*p->skillIds) + sizeof(*p); + p->packetLength = len; + p->flag = c; + + WFIFOSET(fd, len); } else { status_change_end(&sd->bl,SC_STOP,INVALID_TIMER); clif->skill_fail(sd, SC_AUTOSHADOWSPELL, USESKILL_FAIL_IMITATION_SKILL_NONE, 0, 0); @@ -20861,6 +20878,7 @@ static int clif_autoshadowspell_list(struct map_session_data *sd) return 1; } + /*=========================================== * Skill list for Four Elemental Analysis * and Change Material skills. diff --git a/src/map/packets_struct.h b/src/map/packets_struct.h index 7b43e821e1e..dca0fa2c7f6 100644 --- a/src/map/packets_struct.h +++ b/src/map/packets_struct.h @@ -3319,6 +3319,14 @@ struct PACKET_ZC_MAKINGARROW_LIST { } __attribute__((packed)); DEFINE_PACKET_HEADER(ZC_MAKINGARROW_LIST, 0x01ad); +struct PACKET_ZC_SKILL_SELECT_REQUEST { + int16 packetType; + int16 packetLength; + int32 flag; //< 0 = old code compatibility; 1 = Auto Shadow Spell; same value is received in CZ_SKILL_SELECT_RESPONSE + int16 skillIds[]; +} __attribute__((packed)); +DEFINE_PACKET_HEADER(ZC_SKILL_SELECT_REQUEST, 0x0442); + #if PACKETVER_MAIN_NUM >= 20200916 || PACKETVER_RE_NUM >= 20200723 || PACKETVER_ZERO_NUM >= 20221024 #define REPAIRITEM_INFO REPAIRITEM_INFO2 struct PACKET_ZC_REPAIRITEMLIST { @@ -4768,7 +4776,7 @@ struct PACKET_ZC_NOTIFY_SKILL { int8 action; } __attribute__((packed)); DEFINE_PACKET_HEADER(ZC_NOTIFY_SKILL, 0x01de); -#endif +#endif #if PACKETVER_MAIN_NUM >= 20130731 || PACKETVER_RE_NUM >= 20130724 || defined(PACKETVER_ZERO) struct PACKET_ZC_USE_SKILL { From cbfd006917e7b9bd81ffbbd4ff0d8443b65eb61d Mon Sep 17 00:00:00 2001 From: "Guilherme G. Menaldo" Date: Sun, 28 Apr 2024 15:00:24 -0300 Subject: [PATCH 4/7] Convert PACKET_CZ_SKILL_SELECT_RESPONSE to struct --- src/map/clif.c | 3 ++- src/map/packets_struct.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/map/clif.c b/src/map/clif.c index da06447a026..1d7f94abdd6 100644 --- a/src/map/clif.c +++ b/src/map/clif.c @@ -20928,7 +20928,8 @@ static void clif_parse_SkillSelectMenu(int fd, struct map_session_data *sd) return; } - skill->select_menu(sd,RFIFOW(fd,6)); + const struct PACKET_CZ_SKILL_SELECT_RESPONSE *p = RP2PTR(fd); + skill->select_menu(sd, p->selectedSkillId); clif_menuskill_clear(sd); } diff --git a/src/map/packets_struct.h b/src/map/packets_struct.h index dca0fa2c7f6..288739c8342 100644 --- a/src/map/packets_struct.h +++ b/src/map/packets_struct.h @@ -3327,6 +3327,12 @@ struct PACKET_ZC_SKILL_SELECT_REQUEST { } __attribute__((packed)); DEFINE_PACKET_HEADER(ZC_SKILL_SELECT_REQUEST, 0x0442); +struct PACKET_CZ_SKILL_SELECT_RESPONSE { + int16 packetType; + int32 flag; //< currently unused, matches ZC_SKILL_SELECT_REQUEST.flag + int16 selectedSkillId; +} __attribute__((packed)); + #if PACKETVER_MAIN_NUM >= 20200916 || PACKETVER_RE_NUM >= 20200723 || PACKETVER_ZERO_NUM >= 20221024 #define REPAIRITEM_INFO REPAIRITEM_INFO2 struct PACKET_ZC_REPAIRITEMLIST { From 23252987c9fa87753c19056fab5bd0fe1e6cc824 Mon Sep 17 00:00:00 2001 From: "Guilherme G. Menaldo" Date: Sun, 28 Apr 2024 15:01:44 -0300 Subject: [PATCH 5/7] Create alternative version of skill->get_index that allows controlling error reporting Sometimes you want to check whether the skill exists, but its non-existance is not an error for Herc --- src/map/skill.c | 42 +++++++++++++++++++++++++++++++++--------- src/map/skill.h | 3 ++- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/map/skill.c b/src/map/skill.c index 1ad686a359b..62cd169ea2c 100644 --- a/src/map/skill.c +++ b/src/map/skill.c @@ -124,16 +124,23 @@ static int skill_name2id(const char *name) return strdb_iget(skill->name2id_db, name); } -/// Maps skill ids to skill db offsets. -/// Returns the skill's array index, or 0 (Unknown Skill). -static int skill_get_index(int skill_id) +/** + * Maps skill ids to skill db offsets. + * + * @param skill_id skill to search + * @param report_errors if the skill is not found, report an error to help solving it? + * @return Returns the skill's array index, or 0 (Unknown Skill). + */ +static int skill_get_index_sub(int skill_id, bool report_errors) { int length = ARRAYLENGTH(skill_idx_ranges); if (skill_id < skill_idx_ranges[0].start || skill_id > skill_idx_ranges[length - 1].end) { - ShowWarning("skill_get_index: skill id '%d' is not being handled!\n", skill_id); - Assert_report(0); + if (report_errors) { + ShowWarning("skill_get_index: skill id '%d' is not being handled!\n", skill_id); + Assert_report(0); + } return 0; } @@ -152,19 +159,35 @@ static int skill_get_index(int skill_id) } if (!found) { - ShowWarning("skill_get_index: skill id '%d' (idx: %d) is not handled as it lies outside the defined ranges!\n", skill_id, skill_idx); - Assert_report(0); + if (report_errors) { + ShowWarning("skill_get_index: skill id '%d' (idx: %d) is not handled as it lies outside the defined ranges!\n", skill_id, skill_idx); + Assert_report(0); + } return 0; } if (skill_idx >= MAX_SKILL_DB) { - ShowWarning("skill_get_index: skill id '%d'(idx: %d) is not being handled as it exceeds MAX_SKILL_DB!\n", skill_id, skill_idx); - Assert_report(0); + if (report_errors) { + ShowWarning("skill_get_index: skill id '%d'(idx: %d) is not being handled as it exceeds MAX_SKILL_DB!\n", skill_id, skill_idx); + Assert_report(0); + } return 0; } return skill_idx; } +/** + * Maps skill ids to skill db offsets. + * If something goes wrong, errors will be reported to console. + * + * @param skill_id skill to search not found, report an error to help solving it? + * @return Returns the skill's array index, or 0 (Unknown Skill). + */ +static int skill_get_index(int skill_id) +{ + return skill->get_index_sub(skill_id, true); +} + static const char *skill_get_name(int skill_id) { return skill->dbs->db[skill->get_index(skill_id)].name; @@ -25199,6 +25222,7 @@ void skill_defaults(void) skill->unit_group_newid = 0; /* accessors */ skill->get_index = skill_get_index; + skill->get_index_sub = skill_get_index_sub; skill->get_type = skill_get_type; skill->get_hit = skill_get_hit; skill->get_inf = skill_get_inf; diff --git a/src/map/skill.h b/src/map/skill.h index e7df5b8bfe9..bbf8bc69d10 100644 --- a/src/map/skill.h +++ b/src/map/skill.h @@ -143,7 +143,7 @@ enum e_skill_inf2 { INF2_HIDDEN_TRAP = 0x00080000, ///< Traps that are hidden (based on trap_visiblity battle conf) INF2_IS_COMBO_SKILL = 0x00100000, ///< Sets whether a skill can be used in combos or not INF2_NO_STASIS = 0x00200000, - INF2_NO_KAGEHUMI = 0x00400000, + INF2_NO_KAGEHUMI = 0x00400000, INF2_RANGE_VULTURE = 0x00800000, ///< Range is modified by AC_VULTURE INF2_RANGE_SNAKEEYE = 0x01000000, ///< Range is modified by GS_SNAKEEYE INF2_RANGE_SHADOWJUMP = 0x02000000, ///< Range is modified by NJ_SHADOWJUMP @@ -2067,6 +2067,7 @@ struct skill_interface { int unit_group_newid; /* accesssors */ int (*get_index) (int skill_id); + int (*get_index_sub) (int skill_id, bool report_errors); int (*get_type) (int skill_id, int skill_lv); int (*get_hit) (int skill_id, int skill_lv); int (*get_inf) (int skill_id); From 1eef975375ebf2ca05211ddc9f8811d8c867b5fa Mon Sep 17 00:00:00 2001 From: "Guilherme G. Menaldo" Date: Sun, 28 Apr 2024 15:03:08 -0300 Subject: [PATCH 6/7] Fix assertion selecting "no skills" in Shadow Spell or clicking cancel older clients allows you to click "ok" even when you have not selected a skill for Auto Shadow Spell. In those cases, the client sends a random SkillID which Hercules reports as an internal error (unexpected skill). This is outside our control, but they generate noise and confusion. Checking if the received skill_id is valid before going forward allows us to only let valid skills get into the actuall skill code. --- src/map/clif.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/map/clif.c b/src/map/clif.c index 1d7f94abdd6..aaad3fe9615 100644 --- a/src/map/clif.c +++ b/src/map/clif.c @@ -20929,6 +20929,19 @@ static void clif_parse_SkillSelectMenu(int fd, struct map_session_data *sd) } const struct PACKET_CZ_SKILL_SELECT_RESPONSE *p = RP2PTR(fd); + + /* selectedSkillId is 0 when cancel is clicked. + * + * Some clients (observed in PACKETVER < 2020) sends random skill ids if you click "ok" + * without selecting a skill. This check prevents the skill logic from running and generating bad reports. + */ + if (p->selectedSkillId == 0 || skill->get_index_sub(p->selectedSkillId, false) == 0) { + status_change_end(&sd->bl, SC_STOP, INVALID_TIMER); + clif->skill_fail(sd, sd->ud.skill_id, 0, 0, 0); + clif_menuskill_clear(sd); + return; + } + skill->select_menu(sd, p->selectedSkillId); clif_menuskill_clear(sd); From 15710bc3e2ff176a628d1c2e63a333effe6bcdc0 Mon Sep 17 00:00:00 2001 From: "Guilherme G. Menaldo" Date: Sun, 28 Apr 2024 21:13:43 -0300 Subject: [PATCH 7/7] Fix value of "flag" in ZC_SKILL_SELECT_REQUEST it should always be 1 (meaning auto shadow spell) and not the number of skills in the list --- src/map/clif.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/map/clif.c b/src/map/clif.c index aaad3fe9615..3254a3cbdf3 100644 --- a/src/map/clif.c +++ b/src/map/clif.c @@ -20868,7 +20868,7 @@ static int clif_autoshadowspell_list(struct map_session_data *sd) len = c * sizeof(*p->skillIds) + sizeof(*p); p->packetLength = len; - p->flag = c; + p->flag = 1; // 1 = auto shadow spell WFIFOSET(fd, len); } else {