Skip to content

[Refactor] Convert RollConfig to an object to reduce duplication - #2441

Open
CarlosFdez wants to merge 28 commits into
mainfrom
roll-config
Open

CarlosFdez wants to merge 28 commits into
mainfrom
roll-config

Conversation

@CarlosFdez

Copy link
Copy Markdown
Contributor

No description provided.

@CarlosFdez
CarlosFdez marked this pull request as draft September 29, 2026 10:24
@CarlosFdez
CarlosFdez force-pushed the roll-config branch 2 times, most recently from 0edfd1f to b7b5d19 Compare September 29, 2026 21:09
@WBHarry

WBHarry commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Looks good. I did some testing, and found one issue.
The damage formula split based on Hope/Fear doesn't work in this branch - I tested, and it works on main.

@CarlosFdez

Copy link
Copy Markdown
Contributor Author

Thanks for helping me test, it was not fully tested yet. This means I should focus my attention on hope/fear split.

Hope/fear splits work when damage automation is set to never on main, but fails in all other cases, so the entire thing was already quite fragile.

@CarlosFdez

Copy link
Copy Markdown
Contributor Author

I fixed the regression. The error with hope/fear results when damage autoroll is enabled still persists, but after digging into it it seems the issue is that the damage split results relies on config.roll being the actual roll itself. In the case of the damage autoroll, its a plain object. So its likely something that'll be fixable once we refactor the roll parts of it (which I have no idea what to do there yet).

@CarlosFdez

CarlosFdez commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

The diff will reduce if the movement of ResourceUpdateMap and character sheet types moves into a new PR, and then this gets rebased on top. Let me do that separately, since a smaller diff will make a project of this complexity easier to deal with.

PR here #2442

@CarlosFdez
CarlosFdez force-pushed the roll-config branch 5 times, most recently from 0a3d6fa to bcd0dbf Compare October 3, 2026 23:14
@CarlosFdez
CarlosFdez marked this pull request as ready for review October 4, 2026 08:20
@CarlosFdez CarlosFdez added the PR: Delay Merge PR Creator wishes to delay the merge. Request Creator for timeline. label Oct 4, 2026
@CarlosFdez CarlosFdez changed the title Start work on RollConfig [Refactor] Convert RollConfig to an object to reduce duplication Oct 4, 2026
@WBHarry

WBHarry commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

I made a fix for deathmoves, can check the thing I changed in the commit.


Found two issues that I'm not sure about atm.

  1. Tested Rain of Blades domain card action, and it generates 2 chat messages now. It should only create the initial roll message.
image
  1. Damage rolls are failing. The config's this.item ends up set as the Actor data. Actor data isn't item though, so it crashes on checks that happen.

@CarlosFdez

Copy link
Copy Markdown
Contributor Author

Can you tell me what damage roll is failing? They're working for me, so its probably a specific scenario. That or I accidentally fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Delay Merge PR Creator wishes to delay the merge. Request Creator for timeline.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants