Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign up[WIP] [CR] Inventory Revamp Mk2. #16632
Conversation
sctjkc01
referenced this pull request
May 11, 2016
Closed
[WIP] [CR] Adding grid view option for inventory #16519
illi-kun
reviewed
May 11, 2016
| if(barsize > 13) { color = c_ltred; } | ||
| if(barsize > 18) { color = c_red; } | ||
|
|
||
| std::stringstream bar; |
This comment has been minimized.
This comment has been minimized.
illi-kun
May 11, 2016
Member
You can use std::ostringstream instead of std::stringstream since you only use << here.
Or even more: you can replace this stream stuff & wprintz() by using of wputch() (which puts a single character to the current position set by wmove() in the beginning of current function.
illi-kun
reviewed
May 11, 2016
| @@ -466,7 +502,8 @@ void inventory_selector::display( const std::string &title, selector_mode mode ) | |||
| } | |||
| remove_dropping_items(tmp); | |||
| print_inv_weight_vol(tmp.weight_carried(), tmp.volume_carried(), tmp.volume_capacity()); | |||
| mvwprintw(w_inv, 1, 0, _("To drop x items, type a number and then the item hotkey.")); | |||
| mvwprintw(w_inv, 0, 11, _("To drop x items, type a")); | |||
This comment has been minimized.
This comment has been minimized.
illi-kun
May 11, 2016
Member
it's bad to split a phrase manually, it's better to use fold_and_print() function.
This comment has been minimized.
This comment has been minimized.
|
It's may be better to discuss your idea about redesign on the forum before you spend too much efforts for its implementation because you're trying to redesign the most important menu in the whole game and it's should be really good. |
codemime
reviewed
May 11, 2016
| print_bar(1, TERMX - 14, vol_carried, vol_capacity); | ||
| } | ||
|
|
||
| void inventory_selector::print_bar(int y, int x, int carried, int capacity) const { |
This comment has been minimized.
This comment has been minimized.
codemime
May 11, 2016
Member
Please don't "reinvent" things. There's the get_labeled_bar() function in output.h which does what you want. Also, it's less rigid.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sctjkc01
May 11, 2016
Author
Contributor
I looked it over, and there's three things that I'm seeing it lacks: conditional coloration, a backdrop (ie, fill the remainder of the bar with .), and having a different character for half a stage.
This comment has been minimized.
This comment has been minimized.
codemime
May 11, 2016
•
Member
Then you should probably look it over over again. It lacks only dots (which could be added easily), but I wonder why would you need them? UI design should be consistent and bars should look alike until there's a strong reason for a unique design.
This comment has been minimized.
This comment has been minimized.
sctjkc01
May 11, 2016
Author
Contributor
I feel the coloration would be important as it would help add another visual clue as to the at-a-glance "how full is my inventory" in addition to how full the gauge is. Background and half-stage characters probably isn't entirely necessary.
I'll see about rejiggering get_labeled_bar so it can accept a color as well, defaulting to whatever it currently is now.
This comment has been minimized.
This comment has been minimized.
codemime
May 11, 2016
•
Member
I'll see about rejiggering get_labeled_bar so it can accept a color as well
Once again: reread the function and examine its usage before inventing the wheel. It does accept both color and custom characters. Edit: sorry, my bad. I followed my own advise and I see that it doesn't accept color yet. Feel free to extend it.
This comment has been minimized.
This comment has been minimized.
sctjkc01
May 11, 2016
Author
Contributor
I can see where it accepts custom characters (use one character for part of bar, use another for next part, etc) however I'm missing the part where get_labeled_bar will color the characters.
Perhaps you're thinking I'd like to color the entire bar one color, including the two end-cap brackets? I was hoping to color the brackets separately. And from the looks of things get_labeled_bar only returns a std::string... perhaps in that case, print out what I get from the function in color, then overwrite the brackets in c_ltgray.
Nevermind, I'll leave the thing untouched.
This comment has been minimized.
This comment has been minimized.
sctjkc01
added some commits
May 11, 2016
This comment has been minimized.
This comment has been minimized.
|
A new post has been made showing the latest commits to the forum. |
This comment has been minimized.
This comment has been minimized.
|
I'm against considerable impacts on inventory menu before it's properly refactored. It's still way too coupled and spaghettified. No offence, but this PR increases the amount of mess there. |
This comment has been minimized.
This comment has been minimized.
|
@codemime - In other words, you'd prefer this PR be put on the backburner until the entire code backend is cleaned up? I will admit my C++ might not be as strong as others, however I tried to mimic what I've seen in other spots, adding what I had to. |
This comment has been minimized.
This comment has been minimized.
No. I like the idea, but the place you want your code to be put is not ready for it yet. I plan to improve it pretty soon (column handling) keeping multiple columns in mind. They should be independent.
I didn't mean your C++ skills. Any additions to that very code (apart from cleaning up the existing mess and fixing bugs) would be harmful for now. |
This comment has been minimized.
This comment has been minimized.
|
Would need rebase and resubmission |
sctjkc01 commentedMay 11, 2016
Attempt 2 of Inventory revamp introduced in #16519.
Starting with rejiggering header: