[WIP] [CR] Adding grid view option for inventory #16519

sctjkc01 · May 4, 2016

One thing I was interested in doing was creating a new view for the inventory; while it's still very much a work-in-progress as of right now, I was hoping to implement some form of a grid view option for people who have increased the size of their window.

My line of thinking is, people who increase their window / terminal size might see the inventory being paginated, and see an inefficient use of the new width. The game simply prints out two (or three, for multidrop) columns of your gear and calls it good.

I've planned out a grid view, which (hopefully) makes better use of a larger screen width, while still being mostly friendly to the default 80w x 24h. The hope is producing a better* way of seeing one's inventory. The planner I've produced has both 80w x 24h and 128w x 48h (the latter being a 1024x768 resolution) planned; the solid white line indicates where the smaller resolution ends.

( * "better" is subjective, original list view still available )

In addition, I've added a pair of bars, allowing for at-a-glance gauging of how much weight and volume you're carrying.

Thoughts? Concerns? Complements or jeers?

(Additionally, if anyone knows how one could add a single option without breaking compatibility with older saves, I'm all ears; currently I've blindly duplicated the previous enum options, see the edit in cdfb407, options.cpp, but that seems to break saves.)

Get the header right
Get items visible
Correct issues with item rendering
Get the selector to move as intended
Get the other keyboard commands to function as intended

illi-kun · May 4, 2016

illi-kun reviewed May 4, 2016

View changes

illi-kun · May 4, 2016

src/inventory_ui.cpp

+
+    } else if(OPTIONS["INV_DISPLAY"] == "grid") {
+        if (multidrop) {
+            if(TERMX > 100)


{} are mandatory even for one-line conditions. More general note, you have to follow official code style.

Fixed this issue on my working copy; I'll add that to the next big commit.

illi-kun · May 4, 2016

illi-kun reviewed May 4, 2016

View changes

illi-kun · May 4, 2016

src/item.cpp

+
+    if (is_food()) {
+        if(rotten()) {
+            ret << _(parens ? " (rotten)" : " ROTTEN");


_() marks strings for translation, so you need to modify this line to embrace only string " (rotten)"

Even if the all-caps version should be translated too?

Oops, sorry, it should looks like:
ret << parens ? _( " (active)" ) : _( " ACTIVE" ) );

Understood, making corrections.

illi-kun · May 4, 2016

illi-kun reviewed May 4, 2016

View changes

illi-kun · May 4, 2016

src/item.cpp

+    }
+
+    rtn = ret.str();
+    ret.str("");


Strange line. Is it actually necessary?

Was emptying ret before returning to calling function; not 100% certain if it was necessary. Removed on working copy.

illi-kun · May 4, 2016

illi-kun reviewed May 4, 2016

View changes

illi-kun · May 4, 2016

src/item.cpp

+std::string item::get_item_tags(bool parens) const
+{
+    std::string rtn;
+    std::stringstream ret;


I think it's better to use std::ostringstream here.

illi-kun · May 4, 2016

illi-kun reviewed May 4, 2016

View changes

illi-kun · May 4, 2016

src/item.cpp

+{
+    std::string rtn;
+    std::stringstream ret;
+    ret.str("");


This line is not necessary.

Converted ret to type std::ostringstream, removed ret.str(""); line, and removed unnecessary use of the std::string rtn variable, instead simply returning ret.str().

Rather than an option I'd prefer for the new menu to be good enough to be the only menu, supporting multiple menu layouts adds significant maintenance overhead. That said, please take some time to outline how the new menu is better. It's difficult to review your code for correctness if we don't know your specific goals.

sctjkc01 · May 4, 2016

kevingranade:
Rather than an option I'd prefer for the new menu to be good enough to be the only menu, supporting multiple menu layouts adds significant maintenance overhead.
That said, please take some time to outline how the new menu is better. It's difficult to review your code for correctness if we don't know your specific goals.

The intent of the new inventory view is to produce a more space-efficient display of the items within a user's inventory. As the inventory stands now, expanding beyond the original 80x24 creates a large, unused hunk of space down the middle:

(Apologies for the odd item naming, I'm currently working that out.) Using the grid view instead would allow for a much more populated listing, which not only reclaims otherwise blank space on the horizontal vector, but also would allow for scrolling instead.

Category separations would remain the same, and instead of everything being listed in two columns, the currently equipped stuff appears at the very top.

Navigation through the inventory is intended to become more intuitive: all four arrow keys would be used to move the selector (in a text-edit fashion: moving down a row to a spot where no item exists should push the selector to the last item of the row below), only Enter would be used to examine or mark. The current system has the left arrow key switching columns (less than intuitive, as you start in the left column) and the right arrow key to interact.

The sheet I've been using to plan out the grid view is available on Google Drive, if anyone is interested. This link will take you to it.

In addition, I've (separately) implemented the display of two at-a-glance meters on the top right that lets you get a general idea of how much stuff you're carrying, ideal if you want a quick answer to "can I pick this up?"

illi-kun · May 4, 2016

It's much easier to read the existing menu (with vertical lists of items). Maybe it's enough to add more columns to existing menu and not to rebuild it from scratch?

BTW, I like these bars for weight/volume.

sctjkc01 · May 4, 2016

The question then becomes, where do you make the column breaks? How would you decide "here's a good place to break the column"?

The only spot I can think of to make the break is where a typical page break would be, however that would create an issue where one category continues to the next column. The items after the break could appear to be a part of the category at the top of the previous column.

Night-Pryanik · May 4, 2016

How about that?

Let the first column always stay on the screen, and the other columns shift to other categories when you press left or right.

Nevertheless, the "WEAPON" column in your variant will always show only one item, this means one cell and a lot of space next to it.
(Maybe we should rename it from "weapon" to something like "item wielded").

sctjkc01 · May 4, 2016

I suppose that's true; Wielded / Weapon would always have one item (unless dual-wielding is implemented at some point). In that case, we could play off your idea, and have Wielded and Items Worn share the same column of space, Wielded atop Items Worn. This eliminates all the wasted space otherwise given to a full-fledged Wielded area (admittedly the same thing the current view does now).

-- Wielded -- .... -- Tools -- .... -- Books -- .... -- Food --
n || longbow++ ... k Hammer
-- Items Worn --
c || pants (fits)
d || t-shirt (fits)
etc

The big change here would be, then, having several columns instead of just two or three.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

-        wprintz(w_inv, c_ltgray, "%3d", vol_carried);
+void inventory_selector::print_bar(int y, int x, int carried, int capacity) const {
+    wmove(w_inv, y, x);
+    int barsize = static_cast<int>((carried * 24.0) / capacity);


Can vol_capacity can be 0 (for a naked character), this should be handled here.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+            weight_color = c_red;
+        }
+        wprintz(w_inv, weight_color, "%6.1f", convert_weight(weight_carried) + 0.05 ); // +0.05 to round up;
+        wprintz(w_inv, c_ltgray, "/%-6.1f %s", convert_weight(g->u.weight_capacity()), _(weight_units()));


The result of weight_units is already translated, no need to translate it again.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+    if(barsize > 18) { color = c_red; }
+
+    std::stringstream bar;
+    bar.str("");


This seems useless, a new stringstream is automatically empty.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+            bar << "|";
+        } else if(barsize == (i*2)-1) {
+            bar << "\\";
+        } else break;


Please read doc/CODE_STYLE.txt and apply the styling from there. Especially here.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+            }
+            const std::string name = cur_entry.category->name;
+            if(curr_line >= 3 && curr_line < TERMY) {
+                center_print(w_inv, curr_line, c_magenta, _("--- %s ---"), name.c_str()); // Center print header


The comment here is a bit pointless, the called function is named center_print, one would assume that it "center prints" its arguments.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+            // End Cell Border Stuff
+        }
+
+        std::string item_name = it.label(); // Get object's display name


The comment is lying. item::label returns either a the (item instance specific) label (if it has one) or the name of the item type.

Why are you not using item::display_name?

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+        }
+
+        if(curr_line >= 2 && curr_line + 1 < TERMY) {  // Second line
+            // Cell Border Stuff


Instead of wrapping coding between "foo begin" and "foo end" comments, you could move it into a separate function (with a proper name) and call it from here.

void print_cell_border( ... ) { ... } ... { if(curr_line >= 2 && curr_line + 1 < TERMY) { print_cell_border( ... );

It does not need to be a free function, it could be a lambda.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+            mvwprintz(w_inv, curr_line + 1, 10 + (row_i * 25), fill, it.get_mod_string().c_str()); // Show mod count (if any)
+        }
+
+        std::string temp[2];


Consider using std::array<std::string, 2> as type instead of a raw array. split_names should accept that one instead of raw pointer. When looking at split_names, one can't see what the pointer is supposed to be, no documentation that it should point to an array with at least 2 elements. Using std::array documents that requirement and triggers a compiler error if the argument don't match that requirement.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+            }
+        }
+    }
+    return rtn;


What's the point of unconditionally returning the parameter? The caller already has that value. Returning void should be fine.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+                mvwputch(w_inv, curr_line + 3, x + (row_i * 25), fill, ' ');
+            }
+            mvwputch(w_inv, curr_line + 3, 26 + (row_i * 25), c_white, LINE_XOXO);
+            // End Cell Border Stuff


I haven't counted it, but this block ("Cell Border Stuff") seems the be repeated several times. It should go into a function / local lambda.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/inventory_ui.cpp

+    total_height += 2;
+
+    // Then the carried stuff
+    for(size_t a = 0; a < inv.size(); a++) {


Is the body of this loop a direct copy of the body of the loop above? Why would you do such a thing? Move the code into a function and call it from within both loops.

BevapDin · May 5, 2016

BevapDin reviewed May 5, 2016

View changes

BevapDin · May 5, 2016

src/item.cpp

-                auto const &nc_text = get_item_hp_bar(damage);
-                damtext = "<color_" + string_from_color(nc_text.second) + ">" + nc_text.first + " </color>";
-
+                if(!grid) {


Out of curiosity: why is the item health bar not added here when in grid view? The code in inventory_ui.cpp seems to add it anyway. This adds quite some dependency between this function and the inventory code.

sctjkc01 · May 11, 2016

Pull Request closed, starting fresh in PR #16632.

sctjkc01 added some commits May 4, 2016

Half of InvRevamp; getting PR started

cdfb407

Bringing InvRevamp up to date.

d861b8d

sctjkc01 changed the title ~~[WIP] [RC] Adding grid view option for inventory~~ [WIP] [CR] Adding grid view option for inventory May 4, 2016

Several corrections pointed out by illi_kun

04b0804

With luck, this should finish off item cell rendering
Keyword "should". Corrects a few issues seen in explanatory "after" screenshot.

2e0a6b4

sctjkc01 referenced this pull request May 11, 2016
Closed
[WIP] [CR] Inventory Revamp Mk2. #16632

sctjkc01 closed this May 11, 2016

CleverRaven/Cataclysm-DDA

Join GitHub today

[WIP] [CR] Adding grid view option for inventory #16519

Conversation

sctjkc01 commented May 4, 2016 • edited

sctjkc01 added some commits May 4, 2016

sctjkc01 changed the title [WIP] [RC] Adding grid view option for inventory [WIP] [CR] Adding grid view option for inventory May 4, 2016

illi-kun reviewed May 4, 2016 View changes

This comment has been minimized.

This comment has been minimized.

illi-kun reviewed May 4, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

illi-kun reviewed May 4, 2016 View changes

This comment has been minimized.

This comment has been minimized.

illi-kun reviewed May 4, 2016 View changes

This comment has been minimized.

illi-kun reviewed May 4, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kevingranade via email May 4, 2016

This comment has been minimized.

sctjkc01 commented May 4, 2016

This comment has been minimized.

illi-kun commented May 4, 2016

This comment has been minimized.

sctjkc01 commented May 4, 2016

This comment has been minimized.

Night-Pryanik commented May 4, 2016

This comment has been minimized.

sctjkc01 commented May 4, 2016

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed May 5, 2016 View changes

This comment has been minimized.

sctjkc01 referenced this pull request May 11, 2016

This comment has been minimized.

sctjkc01 commented May 11, 2016

sctjkc01 closed this May 11, 2016

sctjkc01 commented May 4, 2016 •

edited

illi-kun reviewed May 4, 2016

View changes

illi-kun reviewed May 4, 2016

View changes

illi-kun reviewed May 4, 2016

View changes

illi-kun reviewed May 4, 2016

View changes

illi-kun reviewed May 4, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes

BevapDin reviewed May 5, 2016

View changes