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

Add Domains to EV Macro #180

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Domains to EV Macro #180

wants to merge 1 commit into from

Conversation

mysurvive
Copy link
Owner

Changes from Check.roll to skill.roll, making the code a bit more brief and adds the Domains to the roll automatically.

@mysurvive
Copy link
Owner Author

Resolves #179

@xyzzy42
Copy link
Contributor

xyzzy42 commented Nov 5, 2024

I tried a little test at the console, because I thought the flavor text would be more of an issue, and it seems like it loses the target information:
image
image

None of the target roll options appear either.

The underlying cause of this is an issue I've already identified that also effects the system's RK action (I fixed it in the workbench RK macro), foundryvtt/pf2e#15214

The problem is that in order to have a target of a skill check it's necessary to also pick a DC from the target by name. It must be a statistic like ac, or will, and can't just be a number.

@mysurvive
Copy link
Owner Author

mysurvive commented Nov 6, 2024

I see, that's pretty annoying. I was just about to work on the RK macro and rework it like this one.
It doesn't seem like adding domains straight to the roll data does anything either. They don't seem to be evaluated during the roll for Check.roll(), though they show up in the roll inspector.
Example:
I have added domains: skill.check.domains to the roll in exploitVuln() to line 204 here
The roll goes through and roll inspector shows:
image

However, Assurance wasn't evaluated and domain specific roll options aren't in the roll inspector.

This is the reason I was going to go with the skill.roll(args) route, it seemed to actually evaluate the data rather than just pasting it on. I see now that it's not grabbing target roll options... :(

@xyzzy42
Copy link
Contributor

xyzzy42 commented Nov 6, 2024

There a ton of code that gets done in StatisticCheck#roll() before it eventually calls Check.roll() with it. Adding roll options for the domains, adding proficiency level roll options, running any REs that have a beforeRoll() handler, etc.

Using the skill.roll() interface is much better, except you can't specify the flavor text and it has this strange limitation of only allowing a target if the roll is against a named DC on the target.

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.

2 participants