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

Fix copying own skills via RG_PLAGIARISM or SC_REPRODUCE causing the skill to be deleted and skills that required it eventually. #3298

Merged
merged 8 commits into from
Jun 30, 2024

Conversation

skyleo
Copy link
Contributor

@skyleo skyleo commented May 25, 2024

Pull Request Prelude

Changes Proposed

We address this by implementing logic to use SKILL_FLAG_REPLACED_LV_0 in SC_REPRODUCE and RG_PLAGIARISM.
We also reduced some code-duplication shared with pc_jobchange and skill_attack logic of those two skills.
SKILL_FLAG_PERM_GRANTED skills are no longer copyable as there's no clearly defined implementation for those when copied.

Note: A skill that one has the prerequisites for but one didn't level will appear as SKILL_FLAG_PERMANENT and have it's .id entry set in an sd's status.skill[] entry.
Note: Well skills that you didn't learn, nor can learn, will also appear as SKILL_FLAG_PERMANENT, but can be distinguished via unset id.

Vaguely inspired by HeraclesHub/Heracles#14

Issues addressed: #3289

List of commits in my private-fork that this PR is based on:

dfbdbc0335fd05e5317f1efbefb16cac3b7a9050
6e29dc9c1e92453edfe5930d154687200a1e658f
87ba9544695c4e4d332c2f24504d0cb76a6161a3
9d15215176a525ed3aabfcfdbb89ba877b94c60c
a6ecba8539ab3178d8748c098e27bf808e0420cf
a4bd3b1240274da0650cfa2832a82fa2f15f2c3e
4cea95b2391740e9a3f002ac56d0132608bd3f9c
b82d6ca0d7e890a6d25271c7ecf4081d4654e531

@skyleo skyleo added type:bug Issue is a bug or describes an incorrect behavior that should be fixed severity:4-high Crashes and general instability complexity: 2-moderate labels May 25, 2024
Copy link
Member

@guilherme-gm guilherme-gm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is some missing packet (at least for "newer" clients), on 2019-12-24 main, when a new skill is copied by Plagiarism (Rogue one) the old one stays in the tree as "unlearned":

image

In this example, I plagiarized Magnum Break and then Bash. Magnum Break stays there as disabled until I log out.

This does not happen in master, so I am guessing it is a bug.

src/map/skill.c Show resolved Hide resolved
@skyleo
Copy link
Contributor Author

skyleo commented May 30, 2024

I think there is some missing packet (at least for "newer" clients), on 2019-12-24 main, when a new skill is copied by Plagiarism (Rogue one) the old one stays in the tree as "unlearned":

Did you test this on a rogue / stalker and a character that doesn't own skills outside of his skill tree (such as GMs with all skills)

Cause this new logic should not touch these skills that you copied unless you are using this artificial case o-o

@guilherme-gm
Copy link
Member

Hmm I did use @allskills (using default herc configs, where only my class skill tree gets filled -- so no extra skill in my character).

My test was more or less like that:

  • @job Shadow Chaser T.
  • @allskills

then, on a pvp map, use another character to hit me with Magnum Break, which got copied. Then hit by Bash, which got copied but did not erase Magnum Break there.

@skyleo
Copy link
Contributor Author

skyleo commented May 30, 2024

Hmm I did use @allskills (using default herc configs, where only my class skill tree gets filled -- so no extra skill in my character).

My test was more or less like that:

* `@job Shadow Chaser T.`

* `@allskills`

then, on a pvp map, use another character to hit me with Magnum Break, which got copied. Then hit by Bash, which got copied but did not erase Magnum Break there.

Please don't test stuff like this with @allskills, try to test it on a somewhat realistic character. @job and @jlvl should be enough.

If you can reproduce that way I'll try to research.

@skyleo
Copy link
Contributor Author

skyleo commented May 31, 2024

Problem discovered, fixed and accordingly rebased the commits. Introducing a new function pc->is_own_skill now as first commit.

Copy link
Member

@guilherme-gm guilherme-gm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

working fine now; even with allskills

@MishimaHaruna MishimaHaruna added this to the Release v2024.06 milestone Jun 30, 2024
@MishimaHaruna MishimaHaruna merged commit 9b9c8be into HerculesWS:master Jun 30, 2024
346 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity: 2-moderate severity:4-high Crashes and general instability type:bug Issue is a bug or describes an incorrect behavior that should be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants