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 item variant's base type (if relevant) #78442

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

moxian
Copy link
Contributor

@moxian moxian commented Dec 9, 2024

Summary

Interface "Display item variant's base type (if relevant)"

Purpose of change

The motivation for this change is a confusion stemming from the crafting recipe of butchering kit.

20241209-020251-cataclysm-tiles

The recipe calls for a "huge kitchen knife", a "large kitchen knife", and a "cleaver", but I have never encountered any of those items in my playthrough. I thought that I was simply unlucky until one day I noticed one of those components colored green as available. Then I spent the next 10 minutes trying to figure out which of the multiple knives around me is the "large knife" (and which is a "huge knife"), to middling success.

This whole experience was not something I would like to revisit, hence the PR. This adds a short note about the "base type" of the variant item, i.e. it tells you that, say, a "chef knife" is actually "a kind of a large kitchen knife".

20241209-020853-cataclysm-tiles

Describe the solution

Add an extra line (and a separator) to the item info block, right under description.

Describe alternatives you've considered

N/A

Testing

Variant items:
20241209-020853-cataclysm-tiles
20241209-015608-cataclysm-tiles
Nonvariant item:
20241209-023056-cataclysm-tiles

crafting view:
20241209-023423-cataclysm-tiles
admittedly, sometimes it looks silly ("yes, i know that it's a type of cotton apron - that's what i want to craft!"), but probably not too bad? (the preview says that a blue apron is getting crafted, so maybe this extra line still helps?)
20241209-023519-cataclysm-tiles

Additional context

@moxian moxian marked this pull request as ready for review December 9, 2024 10:39
@moxian moxian marked this pull request as draft December 9, 2024 10:54
@moxian
Copy link
Contributor Author

moxian commented Dec 9, 2024

Hold on, that apron crafting screenshot does not look right...
The xl/xs prefixes are getting lost, and i'm not sure of the proper way to get them back yet. I'll take another stab at it in the coming days.

@moxian moxian marked this pull request as ready for review December 9, 2024 10:57
@moxian moxian marked this pull request as draft December 9, 2024 10:57
@github-actions github-actions bot added Info / User Interface Game - player communication, menus, etc. [C++] Changes (can be) made in C++. Previously named `Code` labels Dec 9, 2024
@PatrikLundell
Copy link
Contributor

I would probably rephrase the description to be more akin to "This is classified as a(n) ..." or "This item falls into the ... category" to make it clearer that it's a classification description rather than a description of the item itself.

Anyway, making the classification explicit is useful, especially since the classifications themselves have to be short so they don't bloat out crafting recipes.
Something that might be possible to do on the crafting recipe end would be to color classification entries differently from item entries in recipes (light green and light red?). While that's out of scope for this PR, it might be useful to color the classification entry in this PR differently to make it clearer it's not just a random description but something that has a meaning elsewhere (light grey?).

@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 9, 2024
@RenechCDDA
Copy link
Member

RenechCDDA commented Dec 9, 2024

Personally I'm not a huge fan of displaying the variant's base type anywhere in the UI (why should the player need that info? this failing in the crafting UI should be solved in the crafting UI etc). I'm also not 100% sure we send those names out for translation...

I think one possible better approach would be to identify tools/components that have variants and silently replace them with a fake requirements group consisting of each possible variant on load. (There may need to be some adjustment to the crafting UI logic for this)

So instead of saying that it needs a huge knife it would instead say butcher knife OR [other huge knife variant] OR [yet another huge knife variant]. Then the player could simply look at the list and be like "Oh yeah a butcher knife I have one of those!". Instead of looking at all their knives and checking if the fine print says "Huge knife" for each specific one.

This could be done in addition to your PR, and I'm not trying to say this is what you should/must do. I am simply offering an approach that I think would be better.

@moxian
Copy link
Contributor Author

moxian commented Dec 10, 2024

I think one possible better approach would be to identify tools/components that have variants and silently replace them with a fake requirements group consisting of each possible variant on load. (There may need to be some adjustment to the crafting UI logic for this)

I have considered that approach, but it doesn't work too well either, since you kinda really don't want to list 20 different types of blanket patterns and colors when looking at the crafting UI.

(I actually do want to do something similar, but instead to collapse requirements in the crafting view and present it as "meat sandwich requires: 2 any bread (cornbread/white bread/...) + 1 any meat(bologna/cooked meat/cooked fish filet/..)". But that's a topic for another PR altogether, when and if I'm able to handle that)

I'm also not 100% sure we send those names out for translation...

If we don't then we already have a problem with butchering kit components (huge kitchen knife, large kitchen knife, cleaver) not being translated in the crafting GUI.

Perhaps the real issue here is that kitchen knives specifically are implemented as variants, but they would work better if we split them back into distinct items (with some copy-from)?
It is already weird that the three large kitchen knives (chef's, carving, bread) descriptions imply that they have very different stats, but the actual stats are literally identical (22 butchery for bread knife?)

I haven't actually had any issues whatsoever with other variants, since they are mostly cosmetic and their name reflects their base type well enough.

Chesterton's fence: Knives were originally moved to variant system in #73993 with the justification of "@I-am-Erk wanted this". Relevant discord quotes from him:

when it comes to variants I have a pretty high threshold
I think we need to migrate some more of our standalone items to be come variants but I'd love to see more cosmetic variants of existing mundane items. I think they add a lot of flavour at close to no cost.
big one right now is one I added myself, kitchen knives, which I think should be migrated to use the variant system so that eg. a steak knife and a paring knife are just variants of a "small knife" template, "carving knife" and "chef knife" are both 'large knife'

Which... I agree in principle actually! The problem we meet here is that the knife variants are no longer cosmetic and have noticeable gameplay impact. If bread knife and chef knife are both cosmetic variations of the same, then i would expect chef knife and vegetable cleaver to also be cosmetic variations of the same, but they aren't.

Yet another option here is to change butchering kit recipe itself to make it more readable, but I'm struggling to come up with a good practical solution. At this point outright removing it doesn't sound too bad...

I'm at a loss here, honestly, all the options feel bad. So I'll probably let this PR sit and collect dust until either I end up liking one of the options more than the others, or someone with Strong Opinions would nudge me in the right direction.

@PatrikLundell
Copy link
Contributor

I think this PR is useful, because the player will have trouble guessing whether something is a VARIANT of some intuitive common concept or another IMPLEMENTATION of one. Take blankets, for example, where there now is a bazillion variants (floral, etc.), but also implementations (weighted, etc.), and there is no way to determine which is which when it comes to requirements fulfillment (both in crafting and in quests), except to either list all accepted variants of all accepted implementations, unless you actually define base versions of everything.
It can be noted that listing every variant gets extreme if you need a flag (don't know if the number is above or below 200, but it's huge), or socks (again, a huge number of patterns, colors, and political messaging), etc..

@RenechCDDA
Copy link
Member

With some more thinking on it, I think reverting #73993/splitting the knives back into distinct items with copy-from is a possible valid solution, no offense to anyone involved.

@Brambor
Copy link
Contributor

Brambor commented Dec 14, 2024

Hold on, that apron crafting screenshot does not look right... The xl/xs prefixes are getting lost, and i'm not sure of the proper way to get them back yet. I'll take another stab at it in the coming days.

I am looking into it. However, crafting doesn't always show them for variants too:
image

And variant is sometimes also wrong:
image

I wonder if this is a bug with variants of apron, or with crafting menu displaying variants.

I will file this separately later.

@Brambor
Copy link
Contributor

Brambor commented Dec 14, 2024

I tried a couple of ways:

                info.emplace_back( "DESCRIPTION", string_format( _( "It is a kind of a %s, %s, %s, %s." ),
                                   type->nname( 1 ), item::nname( type->get_id() ), item( type ).tname( 1 ),
                                   item( type ).display_name( 1 ), 1 ) );

image

But none of them give me what I want:

  • no ++
  • XS/XL
  • correct variant

The documentation needs to be improved.

const std::string var_name = itype_variant().id;
if( var_name != "" && typ_name != var_name ) {
insert_separation_line( info );
info.emplace_back( "DESCRIPTION", string_format( _( "It is a kind of a %s." ), type->nname( 1 ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: if anyone nows how to generate the correct article a/an.

Example: It is a kind of a apron. is not right.

@moxian
Copy link
Contributor Author

moxian commented Dec 17, 2024

Quick update

I thought about this more, and I agree with @PatrikLundell here. Variants are neat, but they are a bit of a leaky abstraction, and the player has no way to know whether "green socks" are a valid substitute for "socks", since that depends on the implementation details which are hidden.

A great example (that is not implemented right now, but can be) is giving "chunk of meat" variants of "pork"/"beef"/"veal"/"poultry"/etc. They all act absolutely the same and there's no reason to implement them as separate items, yet the player can't guess that from the UI alone. So we do need a way to display this somehow.

It is not only for crafting UI, it is neccessary to give player the correct intuition about the game world.

"This is classified as a(n) ..." or "This item falls into the ... category" to make it clearer that it's a classification description rather than a description of the item itself.

I like the "This is a kind of" wording, so I'll try to stick to that one. I can differentiate it from fluff description and make it more "official" looking by sprinking in some color, i.e. <c_dark_gray>This is a kind of</c_dark_gray> <c_white>cotton apron</c_white>.

reverting #73993 the knives back into distinct items

I still don't have as strong of an opinion on that one. It can be done in addition to this PR. I don't know.

XL/XS not being displayed

This is not technically a blocker, but it bothers me a bit too much.
So I'll keep this PR in draft until #78574 gets fixed (perhaps by me, perhaps not) (thanks for filing btw!), then adjust the colors here and reopen it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants