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

More advantage mode coverage #4890

Merged
merged 3 commits into from
Jan 2, 2025
Merged

Conversation

krbz999
Copy link
Contributor

@krbz999 krbz999 commented Dec 16, 2024

A continuation of #4838.

Modes:

  • Add: Adds one source of advantage (1) or disadvantage (-1).
  • Upgrade: Makes it impossible to have disadvantage (0, 1).
  • Downgrade: Makes it impossible to have advantage (-1, 0).
  • Multiply: Does nothing.
  • Override: Forces one advantage mode regardless of other sources, downgrade, or upgrades.
  • Custom: is custom.

@krbz999 krbz999 mentioned this pull request Dec 16, 2024
@roth-michael
Copy link
Contributor

roth-michael commented Dec 16, 2024

I don't know how I feel about what I'm about to suggest, but might it make sense to do a ui warn notification if someone uses upgrade/downgrade/multiply/override for the time being, informing them that Add is the only functional change mode currently? On the one hand, I kind of hate that. On the other, it would definitely ensure that it gets fixed since it'd pop up every time data prep happens.
Probably not worth it, but I dunno. If nothing else maybe a console.warn

@arbron arbron added this to the D&D5E 4.2.0 milestone Dec 16, 2024
@arbron arbron requested a review from Fyorl December 27, 2024 18:12
@krbz999 krbz999 force-pushed the more-advantage-coverage branch 3 times, most recently from 54f81e8 to 01a089d Compare December 28, 2024 23:48
* @param {string} path The path to the property affected.
* @param {boolean} [isAdvantage] Is this an instance of advantage or disadvantage?
*/
static addSource(actor, path, isAdvantage=true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The static model schemas do make accomplishing this a bit more difficult. We do have an instance available to us though, if we don't want to do everything in static scope with maps like this. Active Effects are already a part of data preparation, so I don't think there's any conceptual issue with us applying some additional prepared data that isn't the exact property that was targeted.

For example, where we wanted an instance-level advantage property, we could store it on the model instance itself, as a sibling to the targeted property.

const roll = foundry.utils.getProperty(model, change.key.substring(0, change.key.lastIndexOf(".")));
roll.advantage++;

The property will be removed/reset when the Actor is re-prepared, like all derived properties. This does rely somewhat on a predictable data structure though.

We could also put these maps on the model instead, and track them via fieldPath in a similar way to this PR. Something like actor.system.advantages, similar to the original proposal in #809.

I think I prefer keeping the data closer to the targeted field though, which allows the PR to function mostly as it did before.

I did take a look at your original commit in this PR, and the logic looks good to me. So we would want to integrate whichever of the above proposals we settle on with your original logic. Sorry that I was not clearer in my comments on your other PR.

Also, apologies if this is feeling like too much whiplash. I think I have enough that I could make any final changes myself now if you'd prefer. Any thoughts you have on the above are welcome though.

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'll force a commit and we'll see what comes of it.

@krbz999 krbz999 force-pushed the more-advantage-coverage branch from 01a089d to 05a6982 Compare December 31, 2024 23:55
@Fyorl Fyorl merged commit 7b3bba1 into foundryvtt:4.2.x Jan 2, 2025
Fyorl added a commit that referenced this pull request Jan 2, 2025
@krbz999 krbz999 deleted the more-advantage-coverage branch January 2, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants