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

[Cleanup] Correct mobskill magical redundancies #5821

Merged
merged 1 commit into from
May 27, 2024

Conversation

Xaver-DaRed
Copy link
Contributor

@Xaver-DaRed Xaver-DaRed commented May 22, 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?

Mobskill/Summon magical multipliers where stupidly divided among several functions, resulting in several multipliers being repeatedly applied, or not applied at all, depending on the amount of functions used.

This:

  • Makes calculations used the magic rewrite new functions.
  • Makes 1 function handle all multipliers, which is the function used for all magical skills, another the magic burst, which councidentally is the function used for the only skills that CAN magic burst, and the final one the final operations (phalanx, shadows, one for all, stoneaskin, etc)
  • Audits several petskills. Including a magical petskill being treated as physical.

Steps to test these changes

Play, summon, use magical skills and take cover

To review:

  • The important changes are located in the global scripts. The individual skill scripts are mostly style and consistency

@Xaver-DaRed Xaver-DaRed force-pushed the magic-refactor branch 2 times, most recently from c22f45f to e9b7745 Compare May 22, 2024 20:22
finalDamage = math.floor(finalDamage * dayAndWeather)
finalDamage = math.floor(finalDamage * magicBonusDiff)
finalDamage = math.floor(finalDamage * targetMagicDamageAdjustment)
finalDamage = math.floor(finalDamage * damageModifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like (I may have missed one) all these damage modifiers are 1? Do we want to just remove this completely. Not even sure what this is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I havent checked all 250+ scripts that use this function modifier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR is already big enough as it is already though. If it turns out to always be 1, I'll remove it in a different PR.

For now, the main focus was to fix the pressing issue: MAB and TMDA being applied twice while, at the same time, TMDA and weather multipliers not being used for most magical mobskills

Copy link
Contributor

@MowFord MowFord left a comment

Choose a reason for hiding this comment

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

Changes look sane, scanned over all files but didn't pull locally to test.

@claywar claywar merged commit 4444b12 into LandSandBoat:base May 27, 2024
11 checks passed
@UmeboshiXI UmeboshiXI mentioned this pull request Jun 1, 2024
4 tasks
@Xaver-DaRed Xaver-DaRed deleted the magic-refactor branch June 11, 2024 21:25
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.

None yet

5 participants