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

1289: Fixed issue with item value being taken from another item #1309

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

kwitczak
Copy link
Collaborator

@kwitczak kwitczak commented Mar 10, 2023

Motivation and Context

The goal of this change was to fix a problem described in #1289 - skills in the Character sheet had different values when the sheet was in a locked vs unlocked state. That was happening for specialised skills named "Any", for example, "Pilot (Any)" or "Art/Craft (Any)".

Root cause

This problem is caused by the fact that we don't fully support skills with non-unique names.
Skills as items are stored in this.system.skills object, and there are two keys used to identify them:

  • skillName
  • id

And they live next to each other in the same object, this.system.skills.

In some cases, skillName is used to obtain a skill, in some cases it's id.
As a result, if two skills with the same name exist, their rawValue will be displayed correctly, while value will be taken from the this.system.skills by skillName, thus picking the last object that was saved with that non-unique key name, causing the problem.

Types of Changes

I feel id is a new thing and I don't know if using only this property (which would be best) won't break old systems...
So I added a method which, for a given item, should either store it using id or, if id does not exists, as skillName.
It's also used for obtaining the item the same way.

Tests Done

  • Adding a skill and rolling it
  • Editing a skill and rolling it
  • Adding occupation to a character and rolling these skills
  • Doing battle roles and dodge rolls
  • Doing creature rolls

Screenshots

@kwitczak
Copy link
Collaborator Author

@snap01 , @HavlockV I added quite a big description because I'm uncertain if this change won't break backwards-compatibility due to id usage and I'm slowly learning the system. I plan to do some more testing later during the week.

@kwitczak
Copy link
Collaborator Author

@snap01 I did some testing and it seems fine.

@kwitczak kwitczak changed the title WIP: 1289: Fixed issue with item value being taken from another item 1289: Fixed issue with item value being taken from another item Mar 15, 2023
@snap01 snap01 merged commit d88e38c into Miskatonic-Investigative-Society:develop Mar 16, 2023
@HavlockV HavlockV mentioned this pull request May 24, 2023
3 tasks
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.

Locked character sheet skill totals don't match the unlocked sheet or edited values
2 participants