LandSandBoat
[core, sql] Fix KSNM trial weapon crit mods to only apply to themselves
I affirm:
- [x] I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
- [x] I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
- [x] I have read and understood the Contributing Guide and the Code of Conduct.
- [x] 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?
This PR adds a mod (and associated code) for cases where a weapon has a crit rate modifier that only applies to hits by that weapon (when dual wielding). The main case for this are the KSNM trial weapons like Senjuinrikio (which all give crit rate).
I am not sure if there are other cases where the mods should only apply to one hand, if so might need a more general solution, rather than a simple additional mod.
This is a fix from ASB coming upstream.
Steps to test these changes
Use unlocked Senjuinrikio and a diff katana and track crit rate with and without this PR.
AFAIK crit mods on weapons only apply to that weapon in general. In addition to this, the Thunder Staff and Jupiter's Staff is currently applying crit rate in general on LSB, which is making it boost the crit rate of ranged attacks. The Fudo only applies its 3% crit rate to itself as well, etc.
Or at least, that's what I've been led to believe, and none of the English wikis are being useful to provide a source.
AFAIK crit mods on weapons only apply to that weapon in general. In addition to this, the Thunder Staff and Jupiter's Staff is currently applying crit rate in general on LSB, which is making it boost the crit rate of ranged attacks. The Fudo only applies its 3% crit rate to itself as well, etc.
Or at least, that's what I've been led to believe, and none of the English wikis are being useful to provide a source.
AFAIK crit mods on weapons only apply to that weapon in general. In addition to this, the Thunder Staff and Jupiter's Staff is currently applying crit rate in general on LSB, which is making it boost the crit rate of ranged attacks. The Fudo only applies its 3% crit rate to itself as well, etc.
Or at least, that's what I've been led to believe, and none of the English wikis are being useful to provide a source.
Yeah, I am seeing some conflicting information about Fudo. Though JP wiki says that basically all crit hit mods on weapons only apply to those weapons.
says that basically all crit hit mods on weapons only apply to those weapons.
This is typically true of cases where the crit was latent effect, and not true in several other cases. It may be we need 2 different mods to cover it all but then there is still the problem of figuring out which items(and augments!) need which mods. If it were up to me alone, I'd have both mods but assume "applied to this item" until each item case was proven other wise, swapping over in the sql row on a case by case basis.
Which makes your PR fine, since the items you changed are all latent anyway I think?
says that basically all crit hit mods on weapons only apply to those weapons.
This is typically true of cases where the crit was latent effect, and not true in several other cases. It may be we need 2 different mods to cover it all but then there is still the problem of figuring out which items(and augments!) need which mods. If it were up to me alone, I'd have both mods but assume "applied to this item" until each item case was proven other wise, swapping over in the sql row on a case by case basis.
Which makes your PR fine, since the items you changed are all latent anyway I think?
Yeah, they are all the latent KSNM ones. I asked siknoz to test Fudo on retail to get a case for a non-latent item. However if we need to change more then can make additional PRs.
Yeah, they are all the latent KSNM ones. I asked siknoz to test Fudo on retail to get a case for a non-latent item. However if we need to change more then can make additional PRs.
Ok, siknoz did a test and Fudo crit hit rate only applies to Fudo hits, so might be quite widespread. The data was: control group of 998 samples and rate of 24.35% vs. Fudo group of 1035 samples with rate of 25.99%, which is close to the estimated 1.5%. Should I add to this PR?
Yeah, they are all the latent KSNM ones. I asked siknoz to test Fudo on retail to get a case for a non-latent item. However if we need to change more then can make additional PRs.
Ok, siknoz did a test and Fudo crit hit rate only applies to Fudo hits, so might be quite widespread. The data was: control group of 998 samples and rate of 24.35% vs. Fudo group of 1035 samples with rate of 25.99%, which is close to the estimated 1.5%. Should I add to this PR?
can either tack it onto this one or in a followup pr. I imagine theres going to be a lot like this once items get tested. Probably a majority are self-only.
Another situation to consider: Grips will probably only add crit to the mainhand slot, being as they don't hit for themselves and can only occupy subslot. They likely don't affect ranged crit rate.
Yeah, they are all the latent KSNM ones. I asked siknoz to test Fudo on retail to get a case for a non-latent item. However if we need to change more then can make additional PRs.
Ok, siknoz did a test and Fudo crit hit rate only applies to Fudo hits, so might be quite widespread. The data was: control group of 998 samples and rate of 24.35% vs. Fudo group of 1035 samples with rate of 25.99%, which is close to the estimated 1.5%. Should I add to this PR?
can either tack it onto this one or in a followup pr. I imagine theres going to be a lot like this once items get tested. Probably a majority are self-only.
Another situation to consider: Grips will probably only add crit to the mainhand slot, being as they don't hit for themselves and can only occupy subslot. They likely don't affect ranged crit rate.
Yep, I added Fudo to this PR, will make another PR for crit hit impacting ranged attacks in a bit.
Forewarning: I read none of the previous comments
I also personally verified that Fudo's crit +% works only on the hand its on with the Abyssea test. I don't know if I have the parse data anymore, but the parse proved beyond a shadow of a doubt that's how it works.
Forewarning: I read none of the previous comments
I also personally verified that Fudo's crit +% works only on the hand its on with the Abyssea test. I don't know if I have the parse data anymore, but the parse proved beyond a shadow of a doubt that's how it works.
Yep, Siknoz parses confirm the same.
Rebase, please, to fix the conflict.
Ok, thanks, I think it looks good now.