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

[cpp] Better skillup handling with sch arts active #5827

Closed
wants to merge 1 commit into from

Conversation

MowFord
Copy link
Contributor

@MowFord MowFord commented May 24, 2024

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Closes #1981 .

Steps to test these changes

at 0 skill enf with dark arts was at 246, skilled up and it stayed the same:
image

at 200 real skill, dark arts put me to 246. Skillup put me to 247:
image

image

@MowFord MowFord force-pushed the sch_arts_skillups branch from e117ab0 to dc3903a Compare May 24, 2024 16:53
@@ -3445,7 +3452,15 @@ namespace charutils

if ((CurSkill / 10) < (CurSkill + SkillAmount) / 10) // if gone up a level
{
PChar->WorkingSkills.skill[SkillID] += 1;
// Light/Dark Arts artificially boost certain skills, logic is too complex to duplicate, so just recalculate it all (similar to using the JA)
if (ArtsBonusActive(PChar, SkillID))
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont know anything about SCH, but what this says to me is that we're going to call BuildingCharSkillsTable whenever TrySkillUP is called - presumably on every action or melee swing etc. if ArtsBonusActive is on.

There's no DB calls in there, it's just a bunch of math, that seems OK, but logically: when ArtsBonusActive falls off, shouldn't we be rebuilding the table one more time to put it back at its original non-inflated state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's actually more work done when light/dark arts is gained/lost (in onEffectGain/onEffectLose)

and this is only going to do the extra math if the skillup in question is relevant to the currently-active arts buff. Perhaps that function could be renamed for clarity, but it returns true if SkiiID is one of the skills boosted by a current arts effect

@MowFord MowFord closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Incorrectly Listed Magic Skills During Skillup with Light/Dark Arts
2 participants