2-2 jobs skill rebalance - Bard/Dancer (2018 Patch/Renewal)

[guilherme-gm]

Pull Request Prelude

• I have followed proper Hercules code styling.
• I have read and understood the contribution guidelines before making this PR.
• I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

The main purpose of this PR is to introduce the rebalance of Bard/Dancer job skills. This change affects Renewal-only.

On official servers this came along with rebalances of 1st, 2nd jobs and transclass too.

I am not sure whether this could be moved like that, because some skills from transclasses will not play well with the rebalanced ones.

The implementation in this PR is based on kRO and kRO zero patch notes, iRO Wiki, rAthena and divine pride info, along with some in-game testing. I can't say everything is 100% accurate because there were discrepancies between different sources, and I could not test everything in kRO, but should be quite close.

I won't list all the rebalance changes in the PR description, but it may be checked in each commit text. Also, the commits are in the same order as they appear in the references below.

Notes

1. Musical Strike and Slinging Arrow has different info about the new Cast Time in each source. (See commit for info). I am using Divine Pride data.
2. FrostJoker Cooldown: Sakray notes and Divine Pride says 5s. Main notes and iRO Wiki says 4s. Following 4s.
3. Bragi duration 60s -> 180s is from 2019-12-04 and was intentionally left out: https://ro.gnjoy.com/news/notice/View.asp?BBSMode=10001&seq=7191&curpage=1
4. Some SP Cost changes is not mentioned in iRO wiki / DP / kRO notes, but it is set in kRO Lua and divine pride.
5. I am not 100% sure on how confusion on Rockisveil works
6. I am not 100% sure if drum battle ATK bonus really is equip atk like MC_LOUD. but It does come as SP_ATK2.
7. From my tests, attack bonus granted by DC_DANCINGLESSON shows in the UI right side of ATK. But in herc it does not. Since this part was not changed in the renewal rebalance, I am unsure whether Mastery was supposed to always show in the UI and we have a long term bug. I won't touch this for now and open an issue for further investigation.

Affected skills

• BA_APPLEIDUN (Song of Lutie)
• BA_ASSASSINCROSS (Impressive Riff)
• BA_DISSONANCE (Unchained Serenade)
• BA_FROSTJOKE (Unbarring Octave)
• BA_MUSICALLESSON (Music Lessons)
• BA_MUSICALSTRIKE (Melody Strike)
• BA_POEMBRAGI (Magic Strings)
• BA_WHISTLE (Perfect Tablature)
• BD_ADAPTATION (Amp)
• BD_DRUMBATTLEFIELD (Battle Theme)
• BD_ENCORE (Encore)
• BD_ETERNALCHAOS (Down Tempo)
• BD_INTOABYSS (Power Chord)
• BD_LULLABY (Lullaby)
• BD_RICHMANKIM (Mental Sensing)
• BD_RINGNIBELUNGEN (Harmonic Lick)
• BD_ROKISWEIL (Classical Pluck)
• BD_SIEGFRIED (Acoustic Rhythm)
• DC_DANCINGLESSON (Dance Lessons)
• DC_DONTFORGETME (Slow Grace)
• DC_FORTUNEKISS (Lady Luck)
• DC_HUMMING (Focus Ballet)
• DC_SERVICEFORYOU (Gypsy's Kiss)
• DC_THROWARROW (Slinging Arrow)
• DC_UGLYDANCE (Hip Shaker)

References:

• kRO 2018.10.31 patch note
• kRO 2018.11.28 patch note / translated
• Divine Pride

Issues addressed:
Part of #2727

[guilherme-gm]

I have been wondering whether we could/should/want to move these vals to SkillData2 and SkillData3 (yes, this would need to be created) instead of defining them here. (For every skill, not only bragi)

[guilherme-gm]

I tried to start making the idea of "separate function for each skill" a thing here, since most dance/songs are very similar. But I ended up with:

1. this one having a if for lullaby
2. skill_castend_nodamage_id_ugly_dance having a duplicated code for most of it
3. the caller function needing to give the target

I am open for suggestions on how I could improve that, I had a few random thoughts but didn't try too much:

1. Moving chance to SkillData2 (but except for 1 case, it is always 100%...) -- solves the lullby part, but not ugly dance
2. Making more functions, with their exclusive logic and those would call this one. Something like: -- solves both, but creates more functions
   • song_friend_sc --> sc_song w/ Party + chance = 100%
   • song_enemy_sc --> sc_song w/ enemy + chance = 100%
   • song_lullaby --> sc_song w/ enemy + chance = 15%
   • song_ugly_dance --> sc_song w/ enemy + special logic for flag 1

example:

static void skill_castend_nodamage_id_song_splash(struct block_list *src, struct block_list *bl, uint16 skill_id, uint16 skill_lv, int64 tick, int flag)
{
   // ... logic for (flag & 1) == 0 ... (set used song + splash on targets)
}


static void skill_castend_nodamage_id_sc_song_on_party(struct block_list *src, struct block_list *bl, uint16 skill_id, uint16 skill_lv, int64 tick, int flag)
{
	if ((flag & 1) == 0) {
		skill_castend_nodamage_id_song_splash(src, bl, skill_id, skill_lv, tick, flag | BCT_PARTY);
	} else {
		enum sc_type sc = skill->get_sc_type(skill_id);
		sc_start(src, bl, sc, 100, skill_lv, skill->get_time(skill_id, skill_lv), skill_id);
	}
}

static void skill_castend_nodamage_id_sc_song_on_enemy(struct block_list *src, struct block_list *bl, uint16 skill_id, uint16 skill_lv, int64 tick, int flag)
{
	if ((flag & 1) == 0) {
		skill_castend_nodamage_id_song_splash(src, bl, skill_id, skill_lv, tick, flag | BCT_ENEMY);
	} else {
		enum sc_type sc = skill->get_sc_type(skill_id);
		sc_start(src, bl, sc, 100, skill_lv, skill->get_time(skill_id, skill_lv), skill_id);
	}
}

static void skill_castend_nodamage_id_lullaby(struct block_list *src, struct block_list *bl, uint16 skill_id, uint16 skill_lv, int64 tick, int flag)
{
	if ((flag & 1) == 0) {
		skill_castend_nodamage_id_song_splash(src, bl, skill_id, skill_lv, tick, flag | BCT_ENEMY);
	} else {
		enum sc_type sc = skill->get_sc_type(skill_id);
		sc_start(src, bl, sc, 15, skill_lv, skill->get_time(skill_id, skill_lv), skill_id);
	}
}

static void skill_castend_nodamage_id_ugly_dance(struct block_list *src, struct block_list *bl, uint16 skill_id, uint16 skill_lv, int64 tick, int flag)
{
	if ((flag & 1) == 0) {
		skill_castend_nodamage_id_song_splash(src, bl, skill_id, skill_lv, tick, flag | BCT_ENEMY);
	} else {
		int chance = skill->get_time2(skill_id, skill_lv);

		if (rnd() % 100 < chance) {
			int drain_rate = skill->get_time(skill_id, skill_lv);
			int drain_amount = status_get_max_sp(bl) * drain_rate / 100;

			status_zap(bl, 0, drain_amount);
		}
	}
}

[guilherme-gm]

I am not sure whether I should add some toggle to rollback to previous music/dance behavior for RE. The new effects requires 2018-11+ clients or characters will show no visual effect.

But making a toggle for that much of changes is... complicated. I don't know where to set the boundaries, and we have several conf files to change, not only skill_db.

If I change songs, I also need to change adaptation (since after rebalance it no longer ends songs). and then we have things like dissonance that could work in older clients (but I guess I would want to revert all songs, right?)

and well, this leaves us with the musical/dance lessons, which no longer affects songs. But if we are reverting, they should now affect it again? but then we also stop giving the new effects? and if I revert everything, does it make sense to keep only throw arrow rebalance?

[MishimaHaruna]

While I was originally in favor of keeping the option to revert these rebalance changes to support older clients, I realize it's too much of a burden (in both implementation and future maintenance), so I'll take it back.

Considering that I expect a lot of people running renewal servers to have adopted 2018 or newer clients by now (if anything, for the UI improvements they bring), and that it's completely out of the question to have a third "pre-rebalance" build mode and db folder to maintain, I'm in favor of requiring a 2018-11+ client for renewal.

People that wish to run a renewal server on an older client (or want to avoid the rebalance changes), can revert these changes on their fork (or patch their clients if someone comes up with client backport patches). And people that want to include some of these rebalance changes in their pre-renewal servers, may also do so in their fork.

Let's keep it rolling - no reverts for renewal, while pre-renewal stays as it is without the rebalance changes

[guilherme-gm]

@skyleo can you check if this part about buff_ratio / buff_job_ratio is correct on the general exp calc? I find it very weird that job exp boost is buff_ratio + buff_job_ratio: (near the end of the function)

	bexp += apply_percentrate64(bexp, buff_ratio, 100);
	jexp += apply_percentrate64(jexp, buff_ratio + buff_job_ratio, 100);

maybe @hemagx remembers something... but this change is from 8 years ago 😅

This line was introduced on #1328

[skyleo]

A bit odd since this means the bonuses from SC_CASH_PLUSEXP and SC_OVERLAPEXPUP are applied twice for job exp.

Also since when does SC_RICHMANKIM affect players? The experience calculation for SC_RICHMANKIM is in mob.c and basically only mobs that are inside Richmankim, will get SC_RICHMANKIM on them and thus give more experience.

From briefly reading iro wiki it seems that this hasn't changed. I suggest you introduce your change in mob.c at the Experience Calculation around line 2580 in function mob_dead.

[guilherme-gm]

Yeah, I found odd that SC_CASH_PLUSEXP and SC_OVERLAPEXPUP applies twice, I did not test it in-game to check, but looks like a bug to me... not exactly a re-balance issue though.

---

Regarding RICHMANKIM:

After Renewal re-balance, SC_RICHMANKIM is a player buff just like Angelus/etc, there is no longer ground effects (monsters does not get SC_RICHMANKIM now).

From my in-game tests, the EXP is applied per-player.

Consider the following examples for a party of 2 players with even sharing, killing a monster where penalty is applied equally to both members (to make explanation simpler)

Example 1: Player A and Player B NOT buffed
- Result: Player A and Player B gets the same amount of EXP

Example 2: Player A Buffed / Player B NOT buffed
- Result: Player A always gets more EXP than Player B (regardless of who performed the kill)

Example 3: Player A and Player B Buffed
- Result: Player A and Player B gets the same amount of EXP (increased by the effect)

And MVP exp does not get boosted.

I did try to keep it in mob.c, since we are talking about mob exp, but doing it there is... complicated. EXP at mob_dead did not consider the party EXP split yet, and since my tests have shown that a non-buffed player will get less EXP, I would need to change the party sharing code.

[skyleo]

Ah, okay. Sounds good.

[guilherme-gm]

@skyleo another question, is it correct that in Pre-RE, when we cast an Ensemble skill, the partner doesn't pay the initial SP requirement?
Example: Ring Nibelungen costs 50 SP to be cast + 1 SP/3 second.

If player A casts it, and player B is forced to join it, then:

1. Player A pays 50 SP
2. After 3 seconds: Player A + Player B pays 1 SP
3. ...

Is that right for Pre-RE? or should Player B also pay 50 SP to start the ensemble skill? In Renewal after rebalance, I could observe that this is the behavior, but the existing code didn't consume the requirement from Player B.

[skyleo]

Yeah this seems to be the behavior in Pre-RE from what I've seen in Ghidra.

Only one of them pays the initial cost.

(Although that part was kind of hard to read.)

[guilherme-gm]

@violent01 thanks for checking, fixed the missing values