Add supplemental damage buff + Add Covenant skills + Limit Hollowsky ( 1 per grid)

[happypurpleduck]

• Supplemental damage logic based on: https://gbf.wiki/Supplemental_Damage
• Added Covenant Skills (Akasha/Hollowsky skill 2)
• Limit Hollowsky per combination based on Added dark opus weapon limit (1 per grid) #222

one of the solutions i have considered (done, not in this RP however) is adding a new field, "family", and have presets for like dark opus or any weapons that have some special restrictions like only able to use 1 in each party (and some placeholders for user desired input). Could help if there were no 'easy' name matching for future.

• Added a new field for Arms that only appears on specific skill for extra input.
  (currently only for Victorious Covenant and Calamitous Covenant)
• Changed expectedTurn being Infinity "solution",
  doing calculations with Infinity introduces an unwanted 'overhead'.

Uncompleted/extra details:

• Need to account for Bonus DMG in supplemental damage as it applies to it separately. (Add an echo count field for Djeeta and each character?)
• missing some JP/ZH translations (or correction)

Update: (missed): translate 'debuff resistance' in results table
Update 2: Supplemental Damage now is calculated in Graph too

[kei-gbf]

Add fields to save data.

• DjeetaBuffsCount
• debuffsCount

How about "buffsCount" and "enemyDebuffsCount" ?

• debuffsCount ... when new skill which effect by characters debuff, its not sure chara's or enemy's
• buffsCount ... better if each characters has own field.

[kei-gbf]

one of the solutions i have considered (done, not in this RP however) is adding a new field, "family", and have presets for like dark opus or any weapons that have some special restrictions like only able to use 1 in each party (and some placeholders for user desired input). Could help if there were no 'easy' name matching for future.

General logic sounds nice 👍
an unique weapons is now common case.

[kei-gbf]

• Limit Hollowsky per combination based on Added dark opus weapon limit (1 per grid) #222
  • Test the exclusion check with lv100

[kei-gbf]

@h-yasha
Can you add checkbox for list items?
This PR has several topics, I want to check the progress status of review.

- [ ] Supplemental damage logic based on: https://gbf.wiki/Supplemental_Damage
- [ ] Added Covenant Skills (Akasha/Hollowsky skill 2)
- [x] Limit Hollowsky per combination based on #222
- [ ] Added a new field for Arms that only appears on specific skill for extra input.
      (currently only for Victorious Covenant and Calamitous Covenant)
- [ ] Changed expectedTurn being Infinity "solution",
      doing calculations with Infinity introduces an unwanted 'overhead'.

[happypurpleduck]

How about "buffsCount" and "enemyDebuffsCount" ?

debuffsCount ... when new skill which effect by characters debuff, its not sure chara's or enemy's
buffsCount ... better if each characters has own field.

this is they currently just 'thrown' in Djeeta. buffsCount in this case makes total sense.
but for debuffsCount (enemyDebuffCount), I'll move it to buff, as it'd make more 'sense' than being on Djeeta and easier to access? in the future for special cases. However, I'll leave debuffCount on Djeeta, add enemyBuffCount to buff and add debuffCount/buffCount to chara and comment them out (as they have utterly no use currently).

Add fields to save data.

• DjeetaBuffsCount
• debuffsCount

These fields value are loaded from through Arm (skillxDetail) currently. and they do load fine after save/load (did not check backward compatibility)
Update: Wording. maybe to the worst.

[kei-gbf]

@h-yasha
thx. can you fix not the list has now duplicated items.
I wanted you to add only [ ] (checkbox)

[happypurpleduck]

@h-yasha
thx. can you fix not the list has now duplicated items.
I wanted you to add only [ ] (checkbox)
@kei-gbf
like this?

[kei-gbf]

it's a maintenance cost to check full data,
when change the test data source, need to update another expected data.

self reply.
in this point, values check in "test damageArray remainHP 100%,50%" can be removed.
values are not unique, since total is checked, no particular reason to check values.
it's ok currently, but when test data grow and became large test.

[kei-gbf]

how about add "label" new field? now "type" has two roles that make code complex

• "type" code not change ["other", "hp_buff", "third_hit"]
• "label" is for display info

Edit: hmm ... it needed to set two similar and different info.

[kei-gbf]

Separate filter and damage calculation.

// filtering only, type specific code should be here
if (!types.includes(supplemental.type))
    continue;
if (supplemental.type == "hp_based") {
    if (!("threshold" in supplemental)) {
        console.error("Missing HP threshold in: " + supplemental);
        continue;
    }
    if (remainHP < supplemental.threshold) {
        continue;
    }
}

// calc damage only, either if/else  or  switch/default
if (supplemental.type === "third_hit") {
    if (expectedTurn === Infinity) {
        expectedTurn = 1;
    }
    vals[0] += supplemental.damage;
    vals[1] += supplemental.damage * expectedTurn;
} else {
    vals[0] += supplemental.damage;
    vals[1] += supplemental.damageWithoutCritical;
    vals[2] += supplemental.ougiDamage;
    vals[3] += supplemental.chainBurst;
}

• sample case, type was like hp_based & third_hit
  hp_based & third_hit code are still reusable.
  • it's hard in single switch, that mixed two kind of tasks filter/calc damage.

[kei-gbf]

@h-yasha
I explain why the fall through chain is not suit for this case.

switch (type) {
case A: // fall through
case B: // fall through
case C: break;
}

the relation is B include C ... mean B is extended of C
but not ok for A. A include B and C ... A should not have relation with B.
(replace with actual types: "hp_based" should not have relation with "boss_debuff_based")
it's ok if A was super set of B and C, but not in this case.

If there are many types added in future, then fall through approach will break soon.
I think it's a easy and temporary solution.
OOP can handle this logic more flexible by extends classes. (but heavy approach for now)

---

when others want to add something B specific code.
but may not expect that effect to A. the code will be:

switch (type) {
case A: // fall through
case B:
    if (type == B) { /* do not want to effect to A case */ }
    // fall through
case C: 
    break;
}

so I think the solution is use "else" of if/else or default case of switch.
(implicit cases by default/else may have unexpected type in debug time, it's another issue)

[kei-gbf]

switch (type) {
case A: // fall through to C
case B: // fall through to C
case C: break;
}

Reader can notice B should not have type specific code.
but no much reason to use fall through approach,
they can be just default case.

other idea is:

switch (supplemental.type) {
case "third_hit": break;
case "hp_based": // fall through to default
default: break;
}

• move third_hit to top, then we can use default case for other.
• comment "fall through to ..." still match with eslint's rule,

---

• it's extended topic, JavaScript has "label" parameter for continue/break
  that can treat more complex situation with switch.
  but code can be more tricky like "goto" code. I don't know Eslint allow that rule or not.

[kei-gbf]

• unique key for list
• rearrange switch cases
• improve module dependency of the place holder function for table header

I will make PR. it's hard to explain with my English.

[kei-gbf]

just note, naming a bit conflict
"additionalVal" Motocal already use additionalDamage in global_logic.js

• additional for supplemental info
• additional for damage

if anyone had better rename idea, rename this to avoid
when search "additional" in editor, hits both.

[kei-gbf]

@h-yasha

• I think this is the last task Add supplemental damage buff + Add Covenant skills + Limit Hollowsky ( 1 per grid) #233 (comment)
  comment on empty translations
  • other
  • hp_based .... no need detail for this?

---

Update summary #233 (comment)

[happypurpleduck]

(I got a bit busy, nonetheless)

just note, naming a bit conflict
"additionalVal" Motocal already use additionalDamage in global_logic.js

• additional for supplemental info
• additional for damage

if anyone had better rename idea, rename this to avoid
when search "additional" in editor, hits both.

value2 wouldn't value be ok? since it's mainly the damage that is used for calculation. otherwise, extraValue.

[kei-gbf]

wouldn't value be ok? since it's mainly the damage that is used for calculation. otherwise, extraValue.

ok, as it replaced with {value}, plain name
supplemental skill info had "values" field that collect damage, ok if no much confuse.

I worried was mislead of the supplemental/additional,
when want to follow additional damage code.

[happypurpleduck]

I went for extraValue,
it would be a little bit confusing to do value

let djeetaBuffCount = Math.min(10, totals['Djeeta']['buffCount']);
let value = Math.ceil(djeetaBuffCount * 3000 * (1 + damageUP));
supplementalDamageArray["凱歌の誓約"] = {
    damage: value,
    damageWithoutCritical: value,
    ougiDamage: value,
    chainBurst: value,
    type: "djeeta_buff_based",
    extraValue: djeetaBuffCount,
}

[kei-gbf]

与ダメ上昇＆虚空武器第2スキル実装のレビュー完了です。
更新内容の要約は #233 (comment)

制限は以下の通り

• アビダメや奥義追撃には未対応 (Motocal が対応してない)
• 通常攻撃の追撃にも未対応
• 虚空杖 バフの数はジータを参照
• 上限の敵最大HP1% は未実装

Update: 全員共通 → 杖・琴キャラのみなので、表現を訂正

[happypurpleduck]

@kei-gbf

• 虚空杖 バフの数は全員共通

if this is reference to Victorious Covenant

Supplemental damage to staff- and harp-specialty allies' attacks based on MC's number of buffs (Damage cap: 30,000)

It is based on MC number of buffs for everyone.

[kei-gbf]

@h-yasha

Yes. it's just explain the current implementation
the amount refer to Djeeta's buff count

[kei-gbf]

@h-yasha

I got your point, Yes I should not say "everyone" that only stuff, harp allies
I will improve the expression.