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

[#342] Add Race type, update Movement & Type config to work with items #2503

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

arbron
Copy link
Collaborator

@arbron arbron commented Oct 20, 2023

Add Race item type with data structure to handle advancement, movement, darkvision, and creature type. This required some changes to the ActorTypeConfig and ActorMovementConfig to handle editing items.

Screenshot 2023-10-20 at 11 29 14

@arbron arbron self-assigned this Oct 20, 2023
@arbron arbron requested review from aaclayton and Fyorl October 20, 2023 18:30
@arbron arbron mentioned this pull request Oct 20, 2023
@MaxPat931
Copy link
Contributor

The different display of Darkvision being its own line instead of a title and box, like Type and Movement looks just a little off to me. Even though all official races only provide Darkvision as an additional sense, perhaps it should be a title of Senses, then the specified sense in a box, for more consistency/flexibility for homebrew?

Copy link
Contributor

@aaclayton aaclayton left a comment

Choose a reason for hiding this comment

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

Looking good, some suggested changes from my end. Will defer final review and approval to @Fyorl

template.json Outdated Show resolved Hide resolved
module/data/item/race.mjs Outdated Show resolved Hide resolved
module/data/item/race.mjs Outdated Show resolved Hide resolved
module/data/item/race.mjs Show resolved Hide resolved
units: new foundry.data.fields.StringField({initial: "ft", label: "DND5E.MovementUnits"})
}),
type: new foundry.data.fields.SchemaField({
value: new foundry.data.fields.StringField({initial: "humanoid", label: "DND5E.CreatureType"}),
Copy link
Contributor

Choose a reason for hiding this comment

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

type.value field should specify a valid set of choices I think. While we're here, do we like value as this attribute path? I know we've used it before, but maybe something like type.primary or type.category would be more accurate?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's unfortunately a bit of tension with regards to putting explicit choices in the DataModel while still allowing the system to be configurable. If we set choices to something like CONFIG.creatureTypes, and a module adds something to that, any Actors that are subsequently configured with that creature type will become invalid if the module is ever disabled.

There are a few things we've thought about doing here: Deferring to preparation to strip out 'unconfigured' values, or writing custom validators to do something similar. At the moment we've opted to just not use choices for now, until we either implement some of the aforementioned options, or there is some core API solution in place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem with setting a choices is it easily leads to entire actors/items being invalid. If a module or script adds a choice and is then disabled, any actors or items that use that choice will just disappear entirely from the world and the only way a user can know what went wrong is through the inspector. I'd prefer to avoid choices and just degrade more gracefully when creating labels and whatnot.

Renaming would work, but would we also want to migrate the NPC data structure to match?

module/data/item/race.mjs Outdated Show resolved Hide resolved
module/applications/item/item-sheet.mjs Outdated Show resolved Hide resolved
module/applications/actor/type-config.mjs Outdated Show resolved Hide resolved
module/applications/actor/movement-config.mjs Outdated Show resolved Hide resolved
templates/items/race.hbs Show resolved Hide resolved
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

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

Just caught one errant logging statement, otherwise I think this is good to go so I am marking it as approved, but please hold off on merging this just yet until we can decide whether we want to incur the migration burden when the 'race' game term changes.

templates/apps/senses-config.hbs Outdated Show resolved Hide resolved
@arbron arbron changed the base branch from 2.4.x to race October 31, 2023 21:05
@arbron arbron merged commit 73a7185 into foundryvtt:race Oct 31, 2023
@arbron arbron deleted the types/race-data branch October 31, 2023 21:06
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