Reworked how crafting skill requirement display is generated. #36089
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
curstwist
added
<Bugfix>
BevapDin
reviewed
Dec 14, 2019
BevapDin
reviewed
Dec 14, 2019
BevapDin
reviewed
Dec 14, 2019
BevapDin
reviewed
Dec 14, 2019
ishtatann
force-pushed
the
ishta-skill-requirement-display
branch
from
bdccd52 to
86b82e4
Compare
jbytheway
reviewed
Dec 16, 2019
src/recipe.cpp
Outdated
| std::string recipe::required_skills_string() const | ||
| // Format a std::pair<skill_id, int> for the basecamp bulletin board. | ||
| // skill colored white with difficulty in parenthesis. | ||
| template<typename _FIter> |
There was a problem hiding this comment.
Note that I based this off existing code:
https://github.com/CleverRaven/Cataclysm-DDA/blob/master/src/output.h#L690
ishtatann
force-pushed
the
ishta-skill-requirement-display
branch
from
11a3a60 to
d0f132d
Compare
ishtatann
force-pushed
the
ishta-skill-requirement-display
branch
from
d0f132d to
4a92f3a
Compare
Co-Authored-By: BevapDin <[email protected]>
Co-Authored-By: BevapDin <[email protected]>
Co-Authored-By: BevapDin <[email protected]>
Removed the includes I had added into recipe.h for the templates. Moved the templates out of the recipe class since the templates didn't access any member variables. Renamed the templates from required_skills_string to required_skills_as_string to avoid confusing the compiler.
Co-Authored-By: John Bytheway <[email protected]>
…rators. Use copy instead of for loop. Addressing PR comments.
ishtatann
force-pushed
the
ishta-skill-requirement-display
branch
from
4a92f3a to
157857f
Compare
|
We obviously want the bugfix, but it's not clear to me that the other changes are a meaningful improvement, and I'm definitely not touching it before 0.E is out. |
This was referenced Feb 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: Bugfixes "Reworked crafting skill requirement display to be more consistent while fixing a bug"Purpose of change
Fixes #35336
While fixing the bug I found a number of inconsistencies on how generating the text for crafting skill requirements were being generated.
I cleaned it up and fixed the original bug.
There are some additional oddities around basecamp recipes and their use of the primary skill used but those are separate issues that should be handled in a different PR.
Describe the solution
Turned the text generation into a pair of templates for the crafting gui and basecamp.
These templates will take any sort of container that is a pair<skill_id, int>.
Reworked the helper functions to use the new templates.
Testing
Loaded up a save with a basecamp that has recipes.
Verified that the basecamp recipes and crafting recipes display as expected.
Verified that the oddities I saw in basecamp recipes around skills with 0 difficulty or duplicated skills were actually configured that way in the json.