-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Inventory view update - Show color for food in containers, show worst rot status for containers with multiple items #46329
Conversation
… Show # stacks of item inside container and color if food/rotting
Dang, looks like I can't really add in (2) the number of items in the container (gallon jug > milk (3) can't show the 3) because it appears to cause issues for ammo display. I'll remove it for now, and find a way to put that in at a later time. |
src/item.cpp
Outdated
} | ||
return VisitResponse::NEXT; | ||
} ); | ||
for( const map_cursor &checkp : nearby ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, that's gonna hurt inventory performance immensely.
Try checking the following scenario:
- open debug menu -> spawn item ->
E
verything - open inventory
This is a really cool change, and I like how it looks, however, due to how the item hierarchy is currently implemented, we probably would need to postpone parts of this PR, specifically, checking nearby map squares to find item's parent. We discussed the possibility to add backlinks to the items (which will allow to have some nice things, like a hierarchical caches). If backlinks are added, implementing changes like this PR would be painless. But realistically that could happen only in the next dev cycle. |
Adding backlinks for an item to its parent container would probably be the most elegant way to implement this; I'd be fine with waiting on implementing these changes until that's done (or trying to do that myself). The performance hit when viewing inventory is my biggest concern for this, and is why I really don't like this solution of having to look through everything to find a parent, although there really isn't a built-in way for an object upon being placed in a sealed container to inherit a sort of "I'm in a sealed container" flag or "this container is my parent" flag. For now I'll remove the sealed item color indicator so that there isn't a performance hit, and just retain the color of the part after " > [item inside]". |
src/item.cpp
Outdated
contents_suffix_text = string_format( npgettext( "item name", | ||
|
||
int worstrot = 0; | ||
if( contents.get_sealed_summary() == item_contents::sealed_summary::all_sealed ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content of sealed container may still rot if the container has a spoil multiplier larger than 0. For example bottle_plastic
has "spoil_multiplier": 0.5
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright; can get the spoil multiplier in that case, will fix
…ng indicator. Removed sealed indicator for multiple items.
src/item.cpp
Outdated
const item_pocket* const parent_pocket = contained_where(contents_item); | ||
colorprefix = "<" + string_from_color( contents_item.color_in_inventory() ) + ">"; | ||
colorprefix.replace( 1, 1, "color" ); // changes <c_cyan> to <color_cyan> | ||
if( contents.get_sealed_summary() == item_contents::sealed_summary::all_sealed && parent_pocket->spoil_multiplier() == 0 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if( contents.get_sealed_summary() == item_contents::sealed_summary::all_sealed && parent_pocket->spoil_multiplier() == 0 ) { | |
if( parent_pocket->sealed() && parent_pocket->spoil_multiplier() == 0 ) { |
sealed_summary
checks all of the container's pockets, but in this case only the pocket containing the content item matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it seems this only checks for sealed container but not rot progress, whereas the code below in the multiple-item case only checks for rot progress but not sealed container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The top checks for rot progress by calling contents_item.color_in_inventory(), although sealed will supercede it.
The bottom only currently checks rot progress without checking sealed since multiple items in sealed containers isn't a thing right now(?) (see lower comment, if not I can put it in!)
And yes, I think checking sealed and the parent pocket are redundant, the parent pocket by itself is probably (?) fine, will fix!
src/item.cpp
Outdated
contents_suffix_text = string_format( pgettext( "item name", | ||
//~ [container item name] " > [inner item name]" | ||
" > %1$s" ), | ||
" > %1$s%2$s" ), | ||
/* with_contents=false for nested items to prevent excessively long names */ | ||
colorprefix, contents_item.tname( contents_count, true, 0, false ) | ||
/* with_contents=false for nested items to prevent excessively long names */ | ||
contents_item.tname( contents_count, true, 0, false ) ); | ||
) + string_format( "%s", colorsuffix ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please store the color instead of the color prefix/suffix strings and use colorize( text, color )
(from color.h
) to add color to the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I was not aware of that function, but will use it now!
src/item.cpp
Outdated
// If container has multiple food items, take the worst rot state and use that as the color for the "# items" text. | ||
// 0 = No food; 1 = Non-perishable (cyan); 2 = Perishable (light cyan); 3 = Old (yellow); 4 = Rotten (brown) | ||
for( const item *it : contents.all_items_top( item_pocket::pocket_type::CONTAINER ) ) { | ||
if( worstrot < 4 && it->rotten() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the check for sealed container here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it currently possible to seal a container with multiple items in such a way that it is no longer rotting, and for the container to have multiple "pockets" in which one is sealed and no longer rotting while other pockets are not sealed and actively rotting?
I removed it due to complexity of handling the above case (and that sealing a container with multiple items in it is not something I think is supported right now (?))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually would be happy to write sealed (non-rotting) support for multiple items if you have a quick way to generate a sealed + spoil modifier = 0 container with multiple items that are perishable inside so I can test it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can write a unit test that creates one such item and check the name of that item, if you want.
src/item.cpp
Outdated
contents_suffix_text = string_format( pgettext( "item name", | ||
//~ [container item name] " > [count] item" | ||
" > %1$zd item", " > %1$zd items", | ||
contents.num_item_stacks() ), contents.num_item_stacks() ); | ||
" > %1$s" ), colorprefix ) + | ||
string_format( ngettext( "%1$zd item", "%1$zd items", contents.num_item_stacks() ), | ||
contents.num_item_stacks() ) + | ||
string_format( pgettext( "color suffix", "%1$s" ), colorsuffix ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please store the color instead of the color prefix/suffix strings and use colorize( text, color )
(from color.h
) to add color to the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Oh huh, that's odd, didn't realize this comment was duplicated; addressed it in the latest commit!)
Hi. A week ago, I got the idea to paint items the same way. But I gave it up. The selected items are highlighted in a very strange way. I see you have the same problem. And I don't really like whats it looks. I tried remove all color tags in the text output function first, but that didn't help. I approve your solution, but it is not 100% complete. I was poking around in the highlighting function and noticed that there are very few color combinations that can be highlighted. Perhaps adding color combinations there will be solve a problem. |
Yeah, I don't like the fact that the display isn't clean either. I have a feeling it's the order in which the advanced inventory menu colors items with the gray (unselected) versus selected screen, as this issue doesn't seem to appear in the normal inventory menu, which handles it fine. I may have to take a look at that; I might instead close this temporarily until I can push a fix. |
Nevermind, was able to find the fix after looking through the advanced_inv code. Had to strip the color tags in the same way they are stripped in the inventory_ui.cpp code using the remove_color_tags() function, similar to what you had tried; I guess it was just a matter of where the function was called. Thanks for encouraging a fix to the display bug! NOTE: You might notice it also "fixes" (not sure if it was a problem) the green "||" still being green when in the inactive window, and probably any other instances of item coloring not graying out in the advanced inventory's inactive window. |
Please check the highlighting in the pickup window (g) and in the look around items window (shift-v). |
Didn't realize those could be an issue, checking now! |
Just saw Qrox's comment about testing the sealed / spoil = 0 container with multiple items; I think I can probably just edit the jsons to make the zipper bag or something have a spoil modifier of 0 and increase its volume capacity to test multiple 'sealed' non-rotting items. May hold off on committing this until I write in the sealed color for containers with multiple items. |
You might also want to check containers within containers. |
Oh, containers within containers were mentioned in the first post, this only goes one container deep at the moment; should I recursively go through all inner containers instead? I can see justification for doing it, since people have to carry liquids inside of a container within their primary clothing/backpack pocket, and if a liquid spoils and it's in a bag filled with other items it'd be useful to know. I might try to add that in too. |
…ten items in containers with multiple items
…andles multiple pockets
As a comment on the latest commit, when looking at the code itself, it's quite messy; it would be much easier if there were a function that allowed an item to backlink to the container or pocket it was in (sigh, it always comes back to that when handling sealed items). The code as it currently is essentially is a workaround for the lack of a backlink. It's nowhere as much of a performance hit as it would be to check all items in surrounding squares for every item in the inventory, but it is a recursive check through contents of each container (if it has things inside) in a given area of "visible" inventory. Pseudocode style, this is the logic the edited code follows:For Single items
For Multi Items
The below 9 test cases were used on a test container where I edited the zipper bag to instead have two pockets of much larger size and weight capacity: Test Zipper bag (in containers.json):
Test cases below;
The current code as it is now passes all nine of the test cases (the code became as complex as it is in order to handle all the scenarios properly). That should cover everything regarding adding color indicators for consumables within pockets/containers (hopefully!) |
If possible, it would be great to have these test cases enshrined in the unit tests. |
@NorseFTX could you please resolve conflicts? |
I am marking this project as "abandoned" but, since it is doing something we want, I have put it in an Abandoned PR list to hopefully be reinitiated at some point. |
Summary
SUMMARY: Interface "Add Blue color for inventory display of food inside sealed containers + Show 'glass jar > [Color of rot status]pickles'"
Purpose of change
These changes are for inventory interaction convenience, with four changes made as outlined below:
[i] Inventory:
[E] Eat dialogue:
glass jar > pickles (1)
Describe the solution
Comestible (Nonperishable) < Perishable < Old < Rotten
All of the above are superceded by Sealed
Note that allergy / uncooked / nausea status are not shown when there are more than 1 item in the container, although support for those can be added as well.
Describe alternatives you've considered
Testing
Testing the coloring - Load the game:
Sample of how the [E] Eat dialogue currently looks:
Same of the [/] Advanced Inventory dialogue:
NOTE Notice that when a container with an item is highlighted in adv inventory, the second half doesn't get highlighted, although this doesn't happen in [i] inventory view (?), not sure if there's an easy fix for this. They also do not properly grey out when in the inactive window.
Testing multiple items in containers:
Cardboard box with a rotten item:
Cardboard box with at most a perishable item:
Additional context
The display bug with the advanced inventory dialogue (see testing section's "NOTE" in bold) might need fixing, suggestions are welcome.