Feat/flat modifiers to attacks 135 #189
Conversation
…ula within the roll configuration dialog. Applied all changes for both global and temp modifiers to attack rolls and general skill rolls. Also fixed global modifiers for non-attack rolls
…om/the-metalworks/cosmere-rpg into feat/flat-modifiers-to-attacks-135
|
Also, apologies about pulling in the workflow changes from main. got my target branches a bit confused at one point before I realised I had updated the feature branch. Hopefully we can cherry pick those back out when squashing or just let it lie, as it would need to have been pulled before a release anyhow? |
…tribute (attacks only atm, also possibly inelegant)
| "AdditionalFormulaDescription": "Input any extra modifiers you wish to add to this item's d20 Skill Roll. This input will be treated as a dice formula, but any die you add won't be available for advantage/disadvantage currently. Paramters you can use: @mod, @attribute & @skill.rank refer to the values of the skills selected here, but the whole list of skills and attributes can be accessed - ask on the discord for help!" | ||
| }, |
There was a problem hiding this comment.
I think it's preferrable for this to be an on-hover tooltip rather than a description that's always visible. It buys us some real estate in the sheet
There was a problem hiding this comment.
I don't disagree, I just don't know if we use tooltips for providing info like that elsewhere in any of the sheets yet, do we?
This is where something like a set of the tutorials/walkthroughs ( #149 ) or a wiki would reduce the need for wordy hints like this and we could take a review and set a standardised approach to such things
There was a problem hiding this comment.
I'm all for moving any hint infos to tooltips wherever we can. It's just cleaner UI with less overwhelming amounts of text
… rolls. Extracted "None" logic into shared code.
|
Regarding the "Noneable" stuff, feel free to suggest another name. I wanted to use Nullable but that feels like it's inaccurate and also likely to clash with potential future 3rd party libraries. |
Ok, I'll go back and check that, been focussing on the attribute for the last few sessions. TBF I don't think I changed any of the logic for skill selection as it seemed like that was already handled. Might just have to go in and add belt and braces around it. |
That was partly my reason for mentioning/bringing attention to it in my earlier comments. The model value is still set to null (see base item line 134). This is mostly for handling where the select option value is handed around the application. There is probably some way that this could be converted to a null, but technically speaking I'm not sure if using explicit null is good practice in TS and that having a separate value to identify when the user has actively selected a No Value might be cleaner. I'll have to check some articles/ask my work colleagues. |
…ll was selected (no-skill rolls are now possible). Switched from "none" to null in the code for ease. Refactored some common code into generic functions.
|
OK, so I've gone in and updated to what could be the final approach? Enabling no-skill rolls highlighted that the use of "none" as a string value was causing some problems buried deep in some of the object assignments so instead of trying to go round the houses I've switched it to rely on nulls from the earliest opportunity. I think there could be some discussion had to align our use of null vs. undefined in places, but for now happy to take this route. Word of note: Would be good to get more testing in. There seems to be a bit of flakiness to when the rolls pick up |
| // Simple utility type for easier null definitions, but general rule: only use it when you have one type that is nullable (i.e. prefer X | Y | null over Nullable<X | Y>) | ||
| export type Nullable<T> = T | null; |
There was a problem hiding this comment.
Is there a reason to use this over Type | null? Not opposed to this, more questioning for consistency
There was a problem hiding this comment.
None technically, I did look it up.
I prefer it here for the simple fact it seems like it's closer to following the Generic Utility approach kinda like Omit/Pick. We're saying here that this is something of X type or it could be null, and is readable to someone who is less confident with raw Union types. And it seems a little neater in source that having a bunch of | nulls around imo.
Again this is caveated though by what input in the comment, once you've got a few things in the list of potential types, switch back to a straight Union as it is clearer than either Nullbale<X | Y> or Nullable<X> | Nullable<Y>
But it's totally a team preference thing. If we're happy using this going forward, I'd happily just raise a separate PR doing the grunt work to refactor any existing situations where it occurs (along with swapping 'none' literals for the constant I made)
Type
Description
This adds a new input box to the item activation form (for skill roll based activations) that allows users to input a formula and that will get added to the attack roll calculations. Using the existing roll data objects, there are a range of
@tags for various modifiers and flat numbers and dice can be included to allow for manually representing global effects.Related Issue
Closes #135How Has This Been Tested?
Loaded build into my local server and tested using GM and Player accounts.
@mod/@attribute/@skill.rank, use@attr/@skills.<skill>.ranksfor any value on the character)@notations, empty field) and for rolls from items as well as weapons and straight skill rollsScreenshots (if applicable)
Global Mod:
⬇️
Temp Mods:
Checklist: