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

Display martial arts weapons by category #52638

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

dseguin
Copy link
Member

@dseguin dseguin commented Nov 5, 2021

Summary

None

Purpose of change

Just a simple QoL feature. Fixes #52614.

Describe the solution

This separates the martial arts weapons list into categories. As a side improvement, this also highlights items in the player's inventory in yellow.
categorizes_ma
categorizes_ma2

Describe alternatives you've considered

Creating a separate screen for listing weapons by category, accessible via keypress on the martial arts info screen.

Testing

Additional context

Something to keep in mind is that weapon categories exist only as JSON elements, which makes it impossible (I think) to translate via gettext in the code itself.

@Saicchi
Copy link
Contributor

Saicchi commented Nov 5, 2021

Something to keep in mind is that weapon categories exist only as JSON elements, which makes it impossible (I think) to translate via gettext in the code itself.

I'll work on that.

@Termineitor244
Copy link
Contributor

Thank you @dseguin!

@Maleclypse Maleclypse added Martial Arts Arts, Techniques, weapons and anything touching martial arts. Quality of Life QoL: degree to which players are comfortable, and able to enjoy CDDA labels Nov 5, 2021
@Termineitor244
Copy link
Contributor

Looking at your screenshots... You are displaying the categories based on the weapons used by the style, the current implementation means that, if a weapon is used in more than one category (like some of the polearms right now) then all of its categories would appear in the styles that use it, even if the other categories are not in use by the martial art in question.

Sojutsu use of weapon categories is still pending to be merged to master, and it only uses POLEARMS, but HOOKING POLEARMS appears in your screenshot, this could lead to some confusion for the player about what category is actually being used by the martial art.

Wouldn't it be better to display the weapons based by the categories defined in the martial art JSON? Sojutsu would only have a single category being showed but it would be more clear what kind of weapon should be used by the martial art.

@dseguin
Copy link
Member Author

dseguin commented Nov 5, 2021

So I've tried a few ways to address this and I think I found an acceptable solution:

For martial arts that don't specify weapon categories (ex: Fencing), list all categories from the list of valid weapons:
fencing_nocat

For martial arts that use weapon categories (ex: Sojutsu), only list the defined categories:
sojutsu_withcat

Valid weapons that are uncategorized or not in the martial art's weapon categories are classified as "OTHER".

@Termineitor244
Copy link
Contributor

@dseguin thank you! This works like a charm!

@AcidAntOnAMinefield
Copy link

Will there be an "unarmed" category, as well as having unarmed weapons displayed in the F1 Menu for the unarmed martial arts styles?

@dseguin
Copy link
Member Author

dseguin commented Nov 9, 2021

Will there be an "unarmed" category, as well as having unarmed weapons displayed in the F1 Menu for the unarmed martial arts styles?

I think that's up to the people working on martial arts. This PR doesn't add or remove categories, it just changes how the existing MA weapons are displayed in the info screen.

@Termineitor244
Copy link
Contributor

Will there be an "unarmed" category, as well as having unarmed weapons displayed in the F1 Menu for the unarmed martial arts styles?

I personally don't have any plans to add one, since they use a different system than the style weapons (They are defined by a json flag, and are already "categorized" in that sense), but I'm not against adding it per se.

This PR is about displaying martial arts categories, for discussion about the categories in question you should direct your comments at #51867.

@kevingranade kevingranade merged commit 05ecf05 into CleverRaven:master Nov 12, 2021
@dseguin dseguin deleted the ma_weapon_qol branch November 17, 2021 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Martial Arts Arts, Techniques, weapons and anything touching martial arts. Quality of Life QoL: degree to which players are comfortable, and able to enjoy CDDA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weapon division by category in the martial arts info menu
6 participants