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

[Chore] Make TryInterruptSpell more understandable #5601

Merged
merged 1 commit into from
May 5, 2024

Conversation

Xaver-DaRed
Copy link
Contributor

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?

  • Reorganizes and rewrites the function to make it easier to follow.
  • Fixes an issue where spells are always interrupted.

See: PR #5598

Steps to test these changes

Cast spells with 101+ spell interruption rate redction: NEVER get resisted no matter lvl or skill.
Cast spells against high lvl mobs (set them to inmortal). Dont get interupted except VERY RARE ocasions.
Cast spells against same lvl mobs at skill lvl under cap. Get interupted frequently.
Cast spells aganst same lvl mobs at skill cap. Get interrupted half of the times.

// SIRD reduces the interrupt after all the calculations are done -- as evidenced by the infamous "102% SIRD" builds.
// Anything less than 102% interrupt results in the ability to be interrupted.
// Note: the 102% is probably an x/256 x/1024 nonsense -- sometimes 101% works.
check *= interruptRate;
float SIRDRatio = (101.0f - meritReduction - (float)PDefender->getMod(Mod::SPELLINTERRUPT)) / 100.0f;
Copy link
Member

Choose a reason for hiding this comment

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

I suppose 101% implements what retail in effect does, but I wonder if we should actually care? we don't for most other things that have similar problems like Haste (25% = 25% and not 26% = 25%)

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 will trust your judgement here. I placed a 101 independently from what we do in other places preciselly because a 100% does not, in fact, ever give immunity to spell interruption whereas 101/102+ does. Retail accuracy is our go-to, after all.

So whatever you decide in this matter is ok with me. Just say.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should be consistent. Their programmers clearly intended 100% = 100%, so we should abide by that. It's not their fault x/256 or x/1024 exists because of PS2 devkits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 100.0f

@claywar claywar merged commit 206ed91 into LandSandBoat:base May 5, 2024
11 checks passed
@Xaver-DaRed Xaver-DaRed deleted the interrupt-rate branch May 5, 2024 22:14
@cocosolos
Copy link
Contributor

cocosolos commented May 5, 2024

Won't this make uninterruptible SIRD sets (102%) easier to achieve than retail? Idk how SIRD mods are handled but haste uses more precision to accommodate the switch to floats, but for SIRD it would still need to check for 102% unless the actual int values are all known, which I imagine is a lot harder than figuring out haste values.

I'm all for making numbers easier for development and stuff but this results in a visible difference from retail for players.

@WinterSolstice8
Copy link
Member

Won't this make uninterruptible SIRD sets (102%) easier to achieve than retail? Idk how SIRD mods are handled but haste uses more precision to accommodate the switch to floats, but for SIRD it would still need to check for 102% unless the actual int values are all known, which I imagine is a lot harder than figuring out haste values.

I'm all for making numbers easier for development and stuff but this results in a visible difference from retail for players.

It does, but it was already done for haste values. That would mean we need to figure out every x/256 or x/512 or x/1024 value for SIRD that means we have to do it for every haste item. Ultimately, nobody is interested in emulating PS2 floating point truncation. setting it to 102% just means that it's sometimes different than retail, because it's possible to cap SIRD at 101% and not just 102%, it really does just depend on what your gear is, pointing to the floating point crap.

@cocosolos
Copy link
Contributor

cocosolos commented May 5, 2024

It does, but it was already done for haste values. That would mean we need to figure out every x/256 or x/512 or x/1024 value for SIRD that means we have to do it for every haste item.

I think haste values are already all pretty well known and easy to test either way because all it requires is autoattacking and a clock. I think if keeping int SIRD mods and comparing to a % it should at least check for 101% since that's closer to what retail actually does.

Ultimately, nobody is interested in emulating PS2 floating point truncation.

I understand this point of view, but it is more opinionated than the haste changes and actually affects game mechanics. I don't think this is even an issue of float truncation, they just use integers while we would prefer a float because that's what we see in the client. This really comes down to trying to implement what the client shows rather than what the server is doing, which I don't agree with (I was against the haste change too but at least that's easy enough to convert).

Edit:
Just to expand a bit, item help text is probably using something like 10/1024=1%, with some items giving slightly more or less which causes the variance between 101-102% (e.g. an item says 1% but gives 12 or 8). Assuming that's correct, the current solution gives players up to 2.4% extra SIRD. An ideal solution would be to actually get those raw int values and convert them to floats, but since that would be difficult that free SIRD can be reduced by checking 101%. Going up to 102% is too far because of that unknown variance, but the real actual cap is probably a straight 102.4%.

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

4 participants