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

Get spell dice rolling #497

Merged
merged 5 commits into from
Mar 29, 2024
Merged

Conversation

apewall
Copy link
Contributor

@apewall apewall commented Jan 18, 2024

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 #382.

Old spell Card
image

New spell card
image

Old ability card
image

New ability card
image

@apewall apewall force-pushed the roll-spell-chat-card branch from 3630028 to 8fc4491 Compare January 18, 2024 21:03
@apewall apewall force-pushed the roll-spell-chat-card branch from 711998f to 637ae26 Compare February 9, 2024 02:07
@apewall apewall requested a review from wyrmisis February 13, 2024 01:04
<div class="blindable" data-blind="{{data.roll.blindroll}}">
{{#if result.details}}<div class="chat-details">{{{result.details}}}</div>{{/if}}
{{#if result.isFailure}}<div class='roll-result roll-fail'><b>{{localize 'OSE.Failure'}}</b> ({{result.target}})
</div>{{/if}}
{{#if result.isSuccess}}<div class='roll-result roll-success'><b>{{localize 'OSE.Success'}}</b>
({{result.target}})</div>{{/if}}
{{#if rollOSE}}<div>{{{rollOSE}}}</div>{{/if}}
{{#if data.save}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this come from rollData.roll.save or rollData.save?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from RollData.save, I've removed rollData.roll.save as it is not needed.

@apewall apewall requested a review from wyrmisis February 15, 2024 19:13
Copy link
Collaborator

@wyrmisis wyrmisis left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@anthonyronda
Copy link
Member

Thanks for your review!

@all-contributors add @Henrik-Bonsmann for review

@anthonyronda
Copy link
Member

@all-contributors add @Henrik-Bonsmann for review

Copy link
Contributor

@anthonyronda

I've put up a pull request to add @thanks! 🎉

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

Copy link
Contributor

allcontributors bot commented Mar 13, 2024

@anthonyronda

@Henrik-Bonsmann already contributed before to review

EDIT: Nope! First time

@anthonyronda
Copy link
Member

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

@apewall
Copy link
Contributor Author

apewall commented Mar 26, 2024

@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
Copy link
Contributor

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
Copy link
Contributor Author

apewall commented Mar 26, 2024

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 apewall force-pushed the roll-spell-chat-card branch from b756e01 to 4e2f094 Compare March 26, 2024 12:54
Copy link
Member

@anthonyronda anthonyronda left a comment

Choose a reason for hiding this comment

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

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

@apewall
Copy link
Contributor Author

apewall commented Mar 29, 2024

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 anthonyronda merged commit 9e36bca into vttred:main Mar 29, 2024
@anthonyronda
Copy link
Member

@all-contributors add apewall for code

Copy link
Contributor

@anthonyronda

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

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.

4 participants