#Modifier Bugfix Stuff

1 messages · Page 1 of 1 (latest)

ripe tulip
#

Creating a thread to contain talk about the modifiers bug

#

Modifier Bugfix Stuff

#

@golden thorn hope you've been able to fix your PC issues :/
In parallel with what you've been doing, I'm going to start going through the roll code/modifiers stuff and adding documentation/trying to clean it up, with the goal of making it a bit easier to follow and/or rewrite later. Don't know if it will be, but figured I'd share here in case it's helpful at all to your current efforts.
https://github.com/ian612/sfrpg/tree/roll-code-rewrite

golden thorn
ripe tulip
#

The starship stuff, I'm just referring to it as the modifiers bug since the code affected seems to be generic modifier processing functions as opposed to something starship specific (even though the bugginess primarily shows up in starship rolls)

#

Sorry to hear about the motherboard :/

golden thorn
#

Okay yeah. It is definitely shared code and changes to fix it impact other rolls.

golden thorn
#

Looks like I will have a new motherboard on the 30th. Can start taking a look again then.

#

It's annoying because I feel like I was on the cusp.

ripe tulip
#

I've traced and think I understand what Lebomb meant when he said that some conditions like Shaken with modifiers for both attack rolls and ability checks are getting double applied

#

e.g. in my sample character who has a +2 strength, +4 bab, and +1 from modifiers (weapon focus), applying the sickened condition results in the formula changing to +0 strength, +4 bab, and -1 from modifiers

#

Also @knotty steppe I noticed that in module/actor/sheet/base.js, _prepareAttackString() line 435 there is a line

const preparedFormula = `${formula} ${modifiersTotal > 0 ? "+" + String(modifiersTotal) : ""}`;

It seems to me like it should instead read as below, is there a reason it's set up to ignore modifiers with negative values?

const preparedFormula = `${formula} ${modifiersTotal > 0 ? "+" + String(modifiersTotal) : String(modifiersTotal)}`;
knotty steppe
#

I could've sworn I did that deliberately but I can't remember why

knotty steppe
#

Yeah no that's just completely erroneous 😂

#

I'll fix

ripe tulip
#

Cool, same thing also looks like it appears in the damage string prep function right below it

knotty steppe
#

I think my intent was "only prepend the total with a + if it is positive", but that isn't really needed since this is an internal formula and doesn't need to look pretty

ripe tulip
#

Yeah that makes sense

ripe tulip
# ripe tulip I've traced and think I understand what Lebomb meant when he said that some cond...

What I was thinking about re: this, is that it might make sense to include both the parent item/actor ID in the modifier data, and add an effect group number field to modifiers.

The idea is that when a roll context is generated, we check all valid modifiers being applied to it and see if any have the same parent item/actor. If so, then check the group number. If two items have the same group number, then only one of them gets applied. Maybe a default precedence could be

  1. Specific Actions & Values (Attack, Damage, AC, BAB, Initiative Modifiers, CMD, Speed, etc.)
  2. Skills
  3. Abilities

I think this could be done with a "filterModifiers" function within createItemRollContext(). Does this make sense, though, or is there an easier way to do it?

knotty steppe
# ripe tulip What I was thinking about re: this, is that it might make sense to include both ...

Nothing like that is at all necessary. Lemme just clarify a few things.

Modifiers contain their parent actor/item's UUIDs, and are accessible anywhere you have full modifier objects, one notable time you don't have those is during rolls (just one aspect of the roll code I hate).

Second, roll contexts aren't "directly" related to modifiers at all, roll contexts are great, but all they really are is a glorified way to glue lots of objects together, and them present them in a neat way the roll parser can understand.

As for the solution to the problem, compare an older version of calculate-ability-check-modifiers.js to now. It's calculating the actual modifier of the ability, rather than the value to be used to roll with

image.png
#

Not too hard a fix 😛

ripe tulip
#

Ah yep that’s a much simpler solution lol

#

Ty for the explanation, that makes the modifier/context stuff a bit more clear

golden thorn
#

AAAAAAAAAAAAAAAAAAA

#

I had an old gpu lying around and apparently it was my gpu not my mobo

#

also this thing makes me want to cry, every time I plug one hole another opens up. I'm SOOO close though!

ripe tulip
#

Nice. It’s definitely the most complicated thing I’ve dealt with in the system. Not surprised it’s taken a while to get it sorted out

golden thorn
#

It's just so not obvious what things are supposed to be doing what downstream

ripe tulip
#

Yeah the code probably needs more comments

golden thorn
#

unbelievably close

golden thorn
#

@ripe tulip @knotty steppe if you could take a look over the changes I'd love someone else's point of view.

ripe tulip
golden thorn
#

Also I think I just saw it accessing .data which I think is going away soon isn't it?

ripe tulip
#

If it's in the rollContext object, I noticed that too. I think that's just a data structure specific to starfinder, though, and not a document of any kind, so the fields are all custom. Not managed by core at all

ripe tulip
golden thorn
#

How about taking the second from last part of formula paths as the name?

#

lot easier to read than

image.png
ripe tulip
#

@golden thorn Can you open it as a PR on the main Starfinder repo (development branch)? Then Lebomb can comment on it and/or edit it directly if needed

ripe tulip
#

I've opened it up and merged your changes with the latest dev branch changes in my own repo for testing. I'll just post things here as I find them

#

Just to confirm, you've only been testing with starship stuff, right? No regular character rolls?

golden thorn
#

I've been doing character rolls too

#

Tried most things from the main page and attack rolls.

#

Have not tried DC rolls.

#

Just pushed up the name change

#

I will create a pr later. I want to test something first.

ripe tulip
#

I'm trying out a character with the Shaken condition, since I know that's something that's not been working. It still seems to be applying the modifier to the roll twice, while adding the negative of the modifier value to the character's ability modifier in the breakdown. This results in the correct final value for the roll modifier, but the formula breakdown is wrong

image.png
ripe tulip
golden thorn
#

I think what I'm doing ATM fixes it

ripe tulip
#

Cool, nice

#

Seems to work fine for characters with skill checks and saving throws

golden thorn
#

Try getting latest

#

Oh nvm lol

ripe tulip
golden thorn
#

Didn't realize it was just attacks

ripe tulip
#

For that same character as gunner in a starship, with both Shaken and Sickened conditions, it's giving me this result. Seems like it's doing the same thing where it adds the negative of the modifier total to the first node of the roll in the formula

image.png
golden thorn
#

It is

#

It's trying to break out pieces for the explanation

ripe tulip
#

It looks to me like it's working for skill checks, this is the same character again but in the Science Officer Role. All these values look correct

image.png
ripe tulip
golden thorn
#

Requires further testing obviously lol

#

I will review later with these cases

#

Going back to sleep for a bit

ripe tulip
#

Sounds good, again thanks a ton for taking this on. I feel like a bit of a negative nancy just pointing out bugs and not contributing much code, but your work is really appreciated by all of us who use the system (and like starship combat 😂)

golden thorn
#

All good lol

golden thorn
#

hmm

#

I'm not sure the shaken thing is exactly related

#

oh

#

because it applies to ability checks

#

it's adding it to the calculation but then finding it also in the str modifier.

knotty steppe
#

Pull from dev, that's fixed

#

I still notice some weirdness with roll breakdowns occasionally though

golden thorn
#

ah, yeah I'm using released version with just my changes. I've never actually build the project I just copied the js straight into my foundry folder lol

#

I've been doing this the hard way because I was too lazy to look into or ask how to setup a proper dev environment

#

everything else I did was like just one liner fixes lol

#

@knotty steppe you got a sec to hop in channel and help set me up?

knotty steppe
#

Sorry I'm busy rn

golden thorn
#

no worries I will take a look at the contributing doc

golden thorn
ripe tulip
golden thorn
#

Ah

ripe tulip
#

I'm currently trying to figure out why...

ripe tulip
#

@knotty steppe what’s the difference between a modifier of type Formula vs. one of type Constant?

knotty steppe
ripe tulip
#

Gotcha

#

I've decided I'm going to go through the roll code from the point of d20Roll() in dice.js and clean up as much as I can and add explanatory comments, as I feel like that'll be helpful moving forward (I may also rename some variables to be more self-explanatory). This is going to be a long-term thing, though, I think, and it might take me a while

ripe tulip
#

@knotty steppe does registerMathFunctions() in roll.js also need to include floor and ceil? I think I found an issue with those sometimes not being evaluated properly during starship rolls but it was a long time ago

knotty steppe
ripe tulip
# ripe tulip <@211956677149327361> does `registerMathFunctions()` in `roll.js` also need to i...

Ah I found it. I described it in this issue, in the part beginning "2) There also seems to be an issue with resolving some formulas in modifiers. The modifier formula..." (https://github.com/foundryvtt-starfinder/foundryvtt-starfinder/issues/1266#issuecomment-1721852151)

I'll see if I can replicate it again more repeatably and file a separate issue for it

GitHub
Incorrect Bonuses on Starships · Issue #1266 · foundryvtt-starfinde...

Describe the bug Bonuses are inaccurate on piloting checks for gunner and pilot roles. This includes the weapon attack rolls in the weapons tab of the starship sheet. To Reproduce I created a new S...

golden thorn
golden thorn
#

So comparing the development branch to my branch, mine seems to be a big improvement

#

Seems I didn't introduce those other issues they're just ones I didn't notice in my first 5 rounds of looking.

#

And it just seems to be an issue with attack rolls

golden thorn
#

@ripe tulip @knotty steppe if you get a chance give my branch another go. I'm reviewing the changes in a potential PR. It SEEMS like it's all working now.

ripe tulip
ripe tulip
#

Looks great to me!

#

@golden thorn I've tested with:
a character (ability check, skill check, save, weapon attack, weapon damage),
a character on starship (weapon attack and starship action),
an NPC (ability check, skill check, save, weapon attack, weapon damage).

#

It all seems to be working correctly

#

Please PR this so I can stop having dreams about rollnodes at night lol

golden thorn
#

lol I will get it in. I'm just reviewing it because it's fucking hacky at some points and I feel like one of my juniors who has identified where an issue is but suggested a fix that is stupid because they don't actually understand the whole context.

#

Things are falling into place though.

#

I think I know what the actual proper code is to replace that TODO that started this all

ripe tulip
#

Sweet. I'm still going to keep at adding documentation in comments, but for now I'll wait until your PR gets folded into the main branch

golden thorn
#

Sounds good. I'm going to give it a quick go after I get everyone supper and add comments to my changes.

#

if it doesn't pan out I will leave the stupid band aid.

ripe tulip
#

Cool, even if it needs to be just a bandaid for now, at least the roll numbers will be correct again. Sounds like a more comprehensive rewrite might still follow anyways at some point, but I'll be really glad to have this bug fixed

golden thorn
#

Okay I've pushed up something that doesn't have a stupid hard coded if the formula includes base attack bonus just weirdly don't do things!

#

So it needs one more round of testing

#

@ripe tulip if you still say it looks good I will make the PR in a bit.

ripe tulip
golden thorn
#

damnit so close, it's increasing dexterity from ship rolls

ripe tulip
#

Damn. Good that you caught it at least

golden thorn
#

@ripe tulip I think I need to change the starship-actions compendium to have a different gunnery formula?
max(@gunner.attributes.baseAttackBonus.value, @gunner.skills.pil.ranks, @gunner.skills.gun.mod) + @gunner.abilities.dex.abilityCheckBonus
instead of
max(@gunner.attributes.baseAttackBonus.value, @gunner.skills.pil.ranks, @gunner.skills.gun.mod) + @gunner.abilities.dex.mod

ripe tulip
#

I'm pretty sure it should be @gunner.abilities.dex.mod

#

Otherwise it might start adding in penalties for things that affect ability checks (in theory anyway?), which technically a gunnery check isn't

golden thorn
#

hmm

#

so the populate code is pulling out things like shaken inside the @gunner.abilities.dex.mod check

ripe tulip
#

Yeah, shaken shouldn't affect dex.mod at all, just abilityCheckBonus

golden thorn
#

oaky then I could just change the getCalculatedModifiers to ignore mods if it's skill ranks or abilities mod

ripe tulip
#

Would that not stop ability checks from working properly? Or would this be only if the main context is starship, or something like that?

golden thorn
#

ability checks use .abilityCheckBonus

#

which is why they look correct

ripe tulip
#

Oh, yep duh

#

Is there anything weird happening on the actual item.js file in _rollStarshipAttack()?

golden thorn
#

define weird?

#

I have changes to it

ripe tulip
#

In the dev branch (not yours) I found some code in the standard item attack roll method rollAttack that looked like it was adding in extra modifiers and resulting in the double "Shaken" issue, but I didn't really follow it very far

#

I think it was this function

const addModifier = (bonus, parts) => {
    if (bonus.modifierType === SFRPGModifierType.FORMULA) {
        rolledMods.push(bonus);
        return;
    }
    const computedBonus = bonus.modifier;
    parts.push({score: computedBonus, explanation: bonus.name});
    return computedBonus;
};
#

It seemed like it was creating a list of modifiers, but some of them would get added again by the populate code. I was thinking it might be a remnant of the old mods system that should just be stripped out

golden thorn
#

I put in logic that prevents adding rollmods that are already in the formula

#

which is why it doesn't have double

#

lol

#

avoiding shaken for skill rank and ability mod worked

ripe tulip
#

Cool, then even if something like this did exist your code would remove any problematic mods

ripe tulip
golden thorn
#

but now it's excluding shaken from starship weapons entirely 😛

ripe tulip
#

Lol damn

golden thorn
#

Honestly I don't know if it's supposed to apply

ripe tulip
#

I was also thinking about that earlier. Do character conditions apply in Starship combat?

#

Maybe that's a @knotty steppe (or AoNSRD) question

golden thorn
#

this change also removed the other mods from coming in

#

operatives edge for the attack roll

#

which is probably correct also

ripe tulip
#

That feels like it should apply to me, but I'm not sure tbh

golden thorn
#

why would a skill modifier apply to a gunnery check?

#

Gunnery Check = 1d20 + the gunner’s base attack bonus or the gunner’s ranks in the Piloting skill + the gunner’s Dexterity modifier + bonuses from computer systems + bonuses from the captain and science officers + range penalty

#

I was curious why we even had those in there

ripe tulip
#

So yeah, probably should not apply

golden thorn
#

Just pushed up the change

ripe tulip
#

It should probably apply to other actions though, like Engineer and Science Officer stuff

golden thorn
#

seems like it's working to me

#

Those rolls don't use skill rank as the variable

#

they use skill.mod

#

so they pick up the modifiers

ripe tulip
#

Cool

golden thorn
#

yeah this feels better

#

if a variable says its a skill rank that's really static

#

same with the ability mod

ripe tulip
#

Yeah makes sense. I was just perusing the list of all modifier effect types and there isn't one for ability modifier even in the system. It's just ability score and ability check bonus

golden thorn
#

I tossed up a PR

ripe tulip
golden thorn
golden thorn