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

Added bonus/feature by category for Skills #117

Merged
merged 3 commits into from Jul 17, 2019

Conversation

@crnormand
Copy link
Contributor

commented Jul 13, 2019

Added bonuses by category for Skills. This one is kind of messy because of the necessity of passing the categories around.

  1. The "getBonus" methods in GURPSCharacter now accept a category string (actually, a single string of all categories separated by commas), and when they are comparing the bonus to see if it matches, they now also check the category criteria.

  2. To shorten some of the criteria checking code, they can now respond directly if they are "an exact match" kind of criteria (Is), or an "anything goes" kind (isAnything).

  3. SkillBonus gained a category criteria and all of the internal methods were updated to include it (object creation, copy, XML read/write, etc.). Also, a SkillBonus can now determine if it matchesCategories().

  4. The SkillBonusEditor gained a category criteria row.

  5. Skill was updated to include its category in all of its level calculation methods.

  6. And the SkillEditor was modified to pass in the category when determining level.

  7. And finally, Technique and the TechniqueEditor were modified to include the categories as well, since they subclass from Skill.

7a. And I forgot a tooltip method on the TechniqueEditor, so it will show tooltips just like the SkillEditor.

I am sorry this one is hairier than the previous PRs. I tried to whittle out as much as possible, but except for #2 and #7a (above), everything is necessary to make this work.

And unfortunately, the following PRs (for Spells and WeaponDamage) are going to be about the same.

@richardwilkes
Copy link
Owner

left a comment

I realized this morning that you're passing around the categories as a string and then decomposing them to check them... yet we store them already decomposed. You should be passing around the Set<String> instead to avoid work that is unnecessary... on top of this, you probably should be using the hasCategory() method you created (or create an equivalent if you can't get to that method everywhere), since it handles some specific cases that we probably want to support, right?

src/com/trollworks/gcs/character/GURPSCharacter.java Outdated Show resolved Hide resolved
src/com/trollworks/gcs/character/GURPSCharacter.java Outdated Show resolved Hide resolved
src/com/trollworks/gcs/character/GURPSCharacter.java Outdated Show resolved Hide resolved
src/com/trollworks/gcs/feature/SkillBonus.java Outdated Show resolved Hide resolved
src/com/trollworks/gcs/skill/Skill.java Outdated Show resolved Hide resolved
@crnormand

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

I kept the categories as a string because of the editors. While in the editors, the category is just a string, so I would have to encode it into the set of categories. To me it seemed 6 of one, 1/2 dozen of the other.

I do agree that if we keep the "categories as string" parameter, that I should consolidate all of the category composition/decomposition/matching/etc. login in the same class. Which is what I have done with this latest commit.

I would like to keep it this way (as a String vs. Set) with your permission. All of the code for composing and decomposing the categories is kept in ListRow, now. If you feel strongly against, I can refactor and have the editors call the composition method to make the Set instead.

Concerning the two category matching methods, they actually support slightly different functionality. hasCategory() is solely for the TextTemplate output (which has the funky logic of being able to match on the category name, or the name up to ":"). The other (now implemented in ListRow.matchesCategories()) is used by the criteria system, which could include any kind of "is/is not/begins with", etc.

I could change the hasCategory() method to accept a StringCriteria insead, but I wasn't sure we would want to include the funky logic check with our generic criteria matching. It was specifically designed for a FantasyGrounds feature where are can split advantages and equipment by category. When we want to split "coins" from the rest of the inventory, there was a problem because many of the coins in the data libraries have the category "Money: GURPS Standard Medieval Coins", but the DFRPG library has the category set to "Money". So being able to match on "Money" was the only way, with the data as is, catch as many of the coin instances as possible.

Hence why there are 2 different "matching" methods.

@richardwilkes
Copy link
Owner

left a comment

I would prefer to store and use the categories exclusively as a Set<String>. The fact that the editors need it as a string isn't terribly important, because they are single operations where the time required to transform it will never become significant. However, everywhere else they are used, such as in skill level calculations, the transforms may end up being called many thousands of times, so therefore efficiency is important and anything we do should strive to make those calculations stay as fast as possible.

src/com/trollworks/gcs/character/GURPSCharacter.java Outdated Show resolved Hide resolved
src/com/trollworks/gcs/character/GURPSCharacter.java Outdated Show resolved Hide resolved
@crnormand

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Understood, and I will get on it this evening.

@crnormand

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Done. The latest commit passes the categories as Set instead of string.

@richardwilkes richardwilkes merged commit d722579 into richardwilkes:master Jul 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.