Attack Delays on Cast Begin (#9160)

- Players can no longer immediately do a normal attack after using a skill
  * Same goes for mercenaries after using a ground skill
- Homunculus and mercenaries now have a fixed server-sided delay of 200ms between skill casts
  * Client-sided it's still slower as it uses attack motion
  * Added a config to go back to the previous, more-secure behavior that uses attack motion server-sided as well
- Code optimization
- Fixes #9155

conf/battle/skill.conf

@@ -24,6 +24,16 @@ delay_dependon_agi: no
 // Note: Setting this to anything above 0 can stop speedhacks.
 min_skill_delay_limit: 100
 
+// Should attack motion be applied as minimum skill delay at castbegin? (Note 1)
+// The client usually doesn't send skill commands faster than attack motion.
+// However, there are a few tricks to make the client send commands faster.
+// For some unit types like mercenaries and homunculus, there is no adequate
+// server-sided delay in some situations, so it's possible to use skills faster
+// than attack motion using these tricks.
+// Set this to "yes" if you want to prevent these tricks and ensure that the
+// delay between using skills at castbegin is at least attack motion.
+amotion_min_skill_delay: no
+
 // This delay is the min 'can't walk delay' of all skills.
 // NOTE: Do not set this too low, if a character starts moving too soon after 
 // doing a skill, the client will not update this, and the player will appear

src/map/battle.cpp

@@ -11783,6 +11783,7 @@ static const struct _battle_data {
 	{ "exp_bonus_attacker",                 &battle_config.exp_bonus_attacker,              25,     0,      INT_MAX,        },
 	{ "exp_bonus_max_attacker",             &battle_config.exp_bonus_max_attacker,          12,     2,      INT_MAX,        },
 	{ "min_skill_delay_limit",              &battle_config.min_skill_delay_limit,           100,    10,     INT_MAX,        },
+	{ "amotion_min_skill_delay",            &battle_config.amotion_min_skill_delay,         0,      0,      1,              },
 	{ "default_walk_delay",                 &battle_config.default_walk_delay,              300,    0,      INT_MAX,        },
 	{ "no_skill_delay",                     &battle_config.no_skill_delay,                  BL_MOB, BL_NUL, BL_ALL,         },
 	{ "attack_walk_delay",                  &battle_config.attack_walk_delay,               BL_ALL, BL_NUL, BL_ALL,         },

src/map/battle.hpp

@@ -433,6 +433,7 @@ struct Battle_Config
 	int32 exp_bonus_attacker;
 	int32 exp_bonus_max_attacker;
 	int32 min_skill_delay_limit;
+	int32 amotion_min_skill_delay;
 	int32 default_walk_delay;
 	int32 no_skill_delay;
 	int32 attack_walk_delay;

src/map/unit.cpp

@@ -50,6 +50,12 @@ using namespace rathena;
 	#define MIN_POS_INTERVAL 20
 #endif
 
+// Minimum delay after which new client-sided commands for slaves are accepted
+// Applies to mercenaries and homunculus
+#ifndef MIN_DELAY_SLAVE
+	#define MIN_DELAY_SLAVE MAX_ASPD_NOPC * 2
+#endif
+
 // Directions values
 // 1 0 7
 // 2 . 6
@@ -1828,7 +1834,8 @@ TIMER_FUNC(unit_resume_running){
 
 /**
  * Sets the delays that prevent attacks and skill usage considering the bl type
- * TODO: Currently this function is only called for normal attacks and parry events and is a work-in-progress
+ * Makes sure that delays are not decreased in case they are already higher
+ * Currently this function only handles normal attacks, cast begin and parry events
  * @param bl Object to apply attack delay to
  * @param tick Current tick
  * @param event The event that resulted in calling this function
@@ -1840,30 +1847,116 @@ void unit_set_attackdelay(block_list& bl, t_tick tick, e_delay_event event)
 	if (ud == nullptr)
 		return;
 
+	t_tick attack_delay = 0;
+	t_tick act_delay = 0;
+
 	switch (bl.type) {
 		case BL_PC:
 			switch (event) {
+				case DELAY_EVENT_CASTBEGIN_ID:
+				case DELAY_EVENT_CASTBEGIN_POS:
+					if (reinterpret_cast<map_session_data*>(&bl)->skillitem == ud->skill_id) {
+						// Skills used from items don't seem to give any attack or act delay
+						return;
+					}
+					[[fallthrough]];
 				case DELAY_EVENT_ATTACK:
 				case DELAY_EVENT_PARRY:
-					// TODO: This should also happen on cast begin
 					// Officially for players it just remembers the last attack time here and applies the delays during the comparison
 					// But we pre-calculate the delays instead and store them in attackabletime and canact_tick
-					ud->attackabletime = tick + status_get_adelay(&bl);
+					attack_delay = status_get_adelay(&bl);
 					// A fixed delay is added here which is equal to the minimum attack motion you can get
 					// This ensures that at max ASPD attackabletime and canact_tick are equal
-					ud->canact_tick = tick + status_get_amotion(&bl) + (pc_maxaspd(reinterpret_cast<map_session_data*>(&bl)) / AMOTION_DIVIDER_PC);
+					act_delay = status_get_amotion(&bl) + (pc_maxaspd(reinterpret_cast<map_session_data*>(&bl)) / AMOTION_DIVIDER_PC);
 					break;
 			}
 			break;
-		default:
+		case BL_MOB:
+			switch (event) {
+				case DELAY_EVENT_ATTACK:
+					// This represents setting of attack delay (recharge time) that happens for non-PCs
+					attack_delay = status_get_adelay(&bl);
+					break;
+				case DELAY_EVENT_CASTBEGIN_ID:
+				case DELAY_EVENT_CASTBEGIN_POS:
+					// When monsters use skills, they only get delays on cast end and cast cancel
+					break;
+			}
+			break;
+		case BL_HOM:
+			switch (event) {
+				case DELAY_EVENT_ATTACK:
+					// This represents setting of attack delay (recharge time) that happens for non-PCs
+					attack_delay = status_get_adelay(&bl);
+					break;
+				case DELAY_EVENT_CASTBEGIN_ID:
+				case DELAY_EVENT_CASTBEGIN_POS:
+					// For non-PCs that can be controlled from the client, there is a security delay of 200ms
+					// However to prevent tricks to use skills faster, we have a config to use amotion instead
+					if (battle_config.amotion_min_skill_delay == 1)
+						act_delay = status_get_amotion(&bl) + MAX_ASPD_NOPC;
+					else
+						act_delay = MIN_DELAY_SLAVE;
+					break;
+			}
+			break;
+		case BL_MER:
 			switch (event) {
 				case DELAY_EVENT_ATTACK:
 					// This represents setting of attack delay (recharge time) that happens for non-PCs
-					ud->attackabletime = tick + status_get_adelay(&bl);
+					attack_delay = status_get_adelay(&bl);
+					break;
+				case DELAY_EVENT_CASTBEGIN_ID:
+					// For non-PCs that can be controlled from the client, there is a security delay of 200ms
+					// However to prevent tricks to use skills faster, we have a config to use amotion instead
+					if (battle_config.amotion_min_skill_delay == 1)
+						act_delay = status_get_amotion(&bl) + MAX_ASPD_NOPC;
+					else
+						act_delay = MIN_DELAY_SLAVE;
+					break;
+				case DELAY_EVENT_CASTBEGIN_POS:
+					// For ground skills, mercenaries work similar to players
+					attack_delay = status_get_adelay(&bl);
+					act_delay = status_get_amotion(&bl) + MAX_ASPD_NOPC;
+					break;
+			}
+			break;
+		default:
+			// Fallback to original behavior as unit type is not fully integrated yet
+			switch (event) {
+				case DELAY_EVENT_ATTACK:
+					attack_delay = status_get_adelay(&bl);
+					break;
+				case DELAY_EVENT_CASTBEGIN_ID:
+				case DELAY_EVENT_CASTBEGIN_POS:
+					act_delay = status_get_amotion(&bl);
 					break;
 			}
 			break;
 	}
+
+	// When setting delays, we need to make sure not to decrease them in case they've been set by another source already
+	if (attack_delay > 0)
+		ud->attackabletime = i64max(tick + attack_delay, ud->attackabletime);
+	if (act_delay > 0)
+		ud->canact_tick = i64max(tick + act_delay, ud->canact_tick);
+}
+
+/**
+ * Updates skill delays according to cast time and minimum delay, and applies security casttime
+ * @param bl Object to apply update delay for
+ * @param tick Current tick
+ * @param event The event that resulted in calling this function
+ */
+void unit_set_castdelay(unit_data& ud, t_tick tick, int32 casttime) {
+	// Use casttime or minimum delay, whatever is longer
+	t_tick cast_delay = i64max(casttime, battle_config.min_skill_delay_limit);
+
+	// Only apply the cast delay, if it is longer than the act delay (set by unit_set_attackdelay)
+	ud.canact_tick = i64max(ud.canact_tick, tick + cast_delay);
+
+	// Security delay that will be removed at castend again
+	ud.canact_tick += SECURITY_CASTTIME;
 }
 
 /**
@@ -2224,8 +2317,6 @@ int32 unit_skilluse_id2(struct block_list *src, int32 target_id, uint16 skill_id
 
 	if (!combo) // Stop attack on non-combo skills [Skotlex]
 		unit_stop_attack(src);
-	else if(ud->attacktimer != INVALID_TIMER) // Elsewise, delay current attack sequence
-		ud->attackabletime = tick + status_get_adelay(src);
 
 	ud->state.skillcastcancel = castcancel;
 
@@ -2378,9 +2469,6 @@ int32 unit_skilluse_id2(struct block_list *src, int32 target_id, uint16 skill_id
 	if( casttime <= 0 )
 		ud->state.skillcastcancel = 0;
 
-	if (!sd || sd->skillitem != skill_id || skill_get_cast(skill_id, skill_lv))
-		ud->canact_tick = tick + i64max(casttime, max(status_get_amotion(src), battle_config.min_skill_delay_limit)) + SECURITY_CASTTIME;
-
 	if( sd ) {
 		switch( skill_id ) {
 			case CG_ARROWVULCAN:
@@ -2395,6 +2483,12 @@ int32 unit_skilluse_id2(struct block_list *src, int32 target_id, uint16 skill_id
 	ud->skill_id      = skill_id;
 	ud->skill_lv      = skill_lv;
 
+	// Set attack and act delays
+	// Please note that the call below relies on ud->skill_id being set!
+	unit_set_attackdelay(*src, tick, DELAY_EVENT_CASTBEGIN_ID);
+	// Apply cast time and general delays
+	unit_set_castdelay(*ud, tick, (skill_get_cast(skill_id, skill_lv) != 0) ? casttime : 0);
+
 	if( sc ) {
 		// These 3 status do not stack, so it's efficient to use if-else
  		if( sc->getSCE(SC_CLOAKING) && !(sc->getSCE(SC_CLOAKING)->val4&4) && skill_id != AS_CLOAKING && skill_id != SHC_SHADOW_STAB) {
@@ -2561,8 +2655,6 @@ int32 unit_skilluse_pos2( struct block_list *src, int16 skill_x, int16 skill_y,
 #endif
 
 	ud->state.skillcastcancel = castcancel&&casttime>0?1:0;
-	if (!sd || sd->skillitem != skill_id || skill_get_cast(skill_id, skill_lv))
-		ud->canact_tick = tick + i64max(casttime, max(status_get_amotion(src), battle_config.min_skill_delay_limit)) + SECURITY_CASTTIME;
 
 // 	if( sd )
 // 	{
@@ -2579,6 +2671,12 @@ int32 unit_skilluse_pos2( struct block_list *src, int16 skill_x, int16 skill_y,
 	ud->skilly      = skill_y;
 	ud->skilltarget = 0;
 
+	// Set attack and act delays
+	// Please note that the call below relies on ud->skill_id being set!
+	unit_set_attackdelay(*src, tick, DELAY_EVENT_CASTBEGIN_POS);
+	// Apply cast time and general delays
+	unit_set_castdelay(*ud, tick, (skill_get_cast(skill_id, skill_lv) != 0) ? casttime : 0);
+
 	if( sc ) {
 		// These 3 status do not stack, so it's efficient to use if-else
 		if (sc->getSCE(SC_CLOAKING) && !(sc->getSCE(SC_CLOAKING)->val4&4)) {