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

[#1401, #1620, #1626] Item Choice Advancement #1627

Closed
wants to merge 21 commits into from

Conversation

arbron
Copy link
Collaborator

@arbron arbron commented Jul 3, 2022

Introduces the item choice advancement for choosing one or more options when you reach a level, usable for things like Fighting Style, Metamagic, or Signature Spells.

The player can select an option from a predefined list like Fighting Style:

Item.Choice.-.Basic.webm

Or they can drop items onto the advancement such as with Magical Secrets:

Item.Choice.-.Drops.webm

If a choice is available over multiple levels like Metamagic, the previously made choices will be shown:

Item.Choice.-.Previous.Choices.webm

Configuration is handled through an interface similar to Grant Items, but with the level list allowing to specify how many choices are available at which levels:

Item Choice configuration interface

This also adds the ability to configure spells that are added through Item Grant or Item Choice advancements, setting ability, preparation mode, and limited uses:

Item Grant - Spell Config

Resolves #1401
Resolves #1620
Resolves #1626

@arbron arbron added this to the D&D5E 1.7.0 milestone Jul 3, 2022
@arbron arbron requested review from Fyorl and akrigline July 3, 2022 18:50
@arbron arbron self-assigned this Jul 3, 2022
@arbron arbron changed the title Item Choice Advancement [#1401, #1626] Item Choice Advancement Jul 3, 2022
@arbron arbron changed the title [#1401, #1626] Item Choice Advancement [#1401, #1620, #1626] Item Choice Advancement Jul 5, 2022
@MaxPat931
Copy link
Contributor

Warlock Invocations provides the option to swap out previous choices for a new one (and Tasha's added a few Optional Class Features that allow for this as well), would it be feasible to build in a Modify Choices button for the Previous Choices view, or some other way to modify them after making the choice?

@aaclayton aaclayton force-pushed the 1.7.x branch 2 times, most recently from c171493 to a272b83 Compare July 9, 2022 20:25
@arbron arbron force-pushed the advancement/item-choice branch from 87cd6a6 to 2c306c2 Compare July 19, 2022 20:03
@arbron arbron changed the base branch from 1.7.x to v10-dev July 19, 2022 20:03
@arbron arbron force-pushed the advancement/item-choice branch 3 times, most recently from d862c19 to 82560e0 Compare July 25, 2022 16:47
@arbron arbron modified the milestones: D&D5E 2.0.0, D&D5E 2.1.0 Jul 26, 2022
@arbron arbron force-pushed the advancement/item-choice branch 3 times, most recently from fa28103 to f3d4179 Compare August 22, 2022 18:09
@Fyorl Fyorl deleted the branch foundryvtt:2.1.x September 1, 2022 11:23
@Fyorl Fyorl closed this Sep 1, 2022
@Fyorl Fyorl reopened this Sep 1, 2022
@Fyorl Fyorl changed the base branch from v10-dev to 2.0.x September 1, 2022 13:21
@arbron arbron force-pushed the advancement/item-choice branch 2 times, most recently from b479709 to 1c00659 Compare September 10, 2022 18:39
@arbron arbron changed the base branch from 2.0.x to 2.1.x September 24, 2022 02:10
@arbron arbron force-pushed the advancement/item-choice branch 2 times, most recently from 204ca41 to 6843852 Compare October 19, 2022 18:21
@arbron arbron force-pushed the advancement/item-choice branch from 6843852 to 26ef789 Compare December 21, 2022 00:42
@arbron arbron force-pushed the advancement/item-choice branch from 26ef789 to c0a3f81 Compare December 29, 2022 16:27
…h dropping existing items, localize chosen text
…ckgrounds can't be added by ItemGrant or ItemChoice advancements
…hoice pool if type restriction or spell level are changed
@arbron arbron force-pushed the advancement/item-choice branch from c0a3f81 to 436d335 Compare December 30, 2022 17:32
@Fyorl
Copy link
Contributor

Fyorl commented Dec 31, 2022

Rather than this being a new type of advancement, I believe we planned to add this functionality to the existing ItemGrant advancement. Conceptually they are the same: An item grant where you are given X options and must take all X of them, vs an item grant where you are given X options and must take Y of them.

@arbron
Copy link
Collaborator Author

arbron commented Dec 31, 2022

Rather than this being a new type of advancement, I believe we planned to add this functionality to the existing ItemGrant advancement. Conceptually they are the same: An item grant where you are given X options and must take all X of them, vs an item grant where you are given X options and must take Y of them.

I believe there are enough differences, particularly with choices that span multiple levels. We'd either have to graft the level code onto ItemGrant where it doesn't have any use and have it exist for all item grants or force users to duplicate ItemChoice advancements for different levels, potentially leading to a large data duplication (see Warlock invocations which have dozens of entries across nearly as many levels).

The ItemChoice advancement also has a bunch of code to handle user-dropped choices during advancement and validating spell levels which are unnecessary for ItemGrant.

@Fyorl
Copy link
Contributor

Fyorl commented Dec 31, 2022

Is it not the case that ItemChoice is a strict superset of ItemGrant though? As in, any existing ItemGrant could be implemented as an ItemChoice instead? Maybe there would have to be some small tweaks to make that true, like a checkbox for 'optional' where, if unchecked, all the available items are granted instead of offering a choice.

It seems like for the Fighting Style example, which has only a single item in its level table, it could be equally implemented as an ItemGrant with optional: true, and an additional field like choices: 1.

If that's the case and they are interchangeable in this way, or where one is a strict superset of the other, then I don't think it makes sense to have both of them. It feels like it would be preferable to combine them in some way in order to keep code reuse to a maximum and reduce our API surface.

I think that's what our goal originally was with ItemGrant, which is why it has the optional field at all. But it seems we've deviated from that trajectory somewhat here.

@arbron arbron closed this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants