vttred / ose

Old-School Essentials – Foundry VTT Edition
https://ose.vtt.red
GNU General Public License v3.0
96 stars 57 forks source link

Get spell dice rolling #497

Closed apewall closed 6 months ago

apewall commented 8 months ago

This change will make spells roll-able from the character sheet when spells have a roll parameter set. It also adds saves, the footer, and tags to generated cards for both spells and abilities. Spells will show their descriptions when rolled, but abilities will not.

This brings spells and abilities in line with how weapon attacks rolls function, and should meet the expectations of https://github.com/vttred/ose/issues/382.

Old spell Card image

New spell card image

Old ability card image

New ability card image

anthonyronda commented 6 months ago

Thanks for your review!

@all-contributors add @Henrik-Bonsmann for review

anthonyronda commented 6 months ago

@all-contributors add @Henrik-Bonsmann for review

allcontributors[bot] commented 6 months ago

@anthonyronda

I've put up a pull request to add @Thanks! :tada:

I've put up a pull request to add @Henrik-Bonsmann! :tada:

allcontributors[bot] commented 6 months ago

@anthonyronda

@Henrik-Bonsmann already contributed before to review

EDIT: Nope! First time

anthonyronda commented 6 months ago

@apewall I'm manually testing this PR right now. I'll send the review shortly

apewall commented 6 months ago

@apewall I'm manually testing this PR right now. I'll send the review shortly

Added b756e01 to prevent a spell that is not memorized from being cast and display an error to the user.

Henrik-Bonsmann commented 6 months ago

Added b756e01 to prevent a spell that is not memorized from being cast and display an error to the user.

While I think that this should be something that's flagged to the user, I don't like the idea of outright preventing the cast. It enforces the use of the automation too much and has the potential to introduce a lot of annoyance for players.

If the preparation was the mistake (instead of the casting), you'd have to open the sheet, navigate to spells, find the correct one, prepare it and then redo (or at least check) all the steps you need to take to cast the spell in the first place.

A warning here is a lot better UX-wise. The best solution for my money would be a "cast anyway" button.

apewall commented 6 months ago

Added b756e01 to prevent a spell that is not memorized from being cast and display an error to the user.

While I think that this should be something that's flagged to the user, I don't like the idea of outright preventing the cast. It enforces the use of the automation too much and has the potential to introduce a lot of annoyance for players.

If the preparation was the mistake (instead of the casting), you'd have to open the sheet, navigate to spells, find the correct one, prepare it and then redo (or at least check) all the steps you need to take to cast the spell in the first place.

A warning here is a lot better UX-wise. The best solution for my money would be a "cast anyway" button.

Thinking it over I largely agree. This should probably be a larger discussion about how we display warnings and allow users the option override the automation via a popup instead. Ill revert for now.

apewall commented 6 months ago

This does not fulfill #382 specifically when it comes to Show spell: it still shows an Automated Animations animation for Show spell

I'm happy approving this, as fixing this behavior wasn't specifically what you intended to do, but it doesn't close #382

I only used the discussion in #382 as a reference for what information should be provided on spell cards when rolled. It's not meant to address the Automation Animation module (Which seems to be in maintenace mode anyway).

anthonyronda commented 6 months ago

@all-contributors add apewall for code

allcontributors[bot] commented 6 months ago

@anthonyronda

I've put up a pull request to add @apewall! :tada: