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

[RDY] Customize Consume Item menu #20676

Merged
merged 20 commits into from Aug 5, 2017

Conversation

Projects
None yet
3 participants
@codemime
Copy link
Member

commented Mar 26, 2017

Follows #19952 and #18681.

Overview

  • The usable information and various reasons why you can't consume stuff are shown.
    01

  • CBMs that turn items into energy became more UI friendly:
    03
    04

  • All the "queries" are now asked at once. No more annoying repetition of "Really eat that?":
    reasons

  • "Smart" sorting order:
    05

    • Rotten food goes last unless you're a saprophagous (then it goes first).
    • Perishable food goes before long-lasting one (preservation and freshness were taken into account).
    • If you're thirsty, food gets sorted by QUENCH, otherwise by NUTRITION.
    • Then by what remains (either NUTRITION or QUENCH).
    • Then by JOY.
    • Then by ENERGY (in case of active CBMs).

@codemime codemime changed the title [CR] Customize Consume menu [CR] Customize Consume Item menu Mar 26, 2017

@@ -215,191 +227,157 @@ morale_type player::allergy_type( const item &food ) const
return MORALE_NULL;
}

bool player::is_allergic( const item &food ) const
edible_rating player::can_eat( const item &food, std::string *err ) const

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Mar 26, 2017

Contributor

The interface would be cleaner if the return value included the error - return by parameter is C-ish.
For example, if you returned it as a const std::pair<edible_rating, std::string> &.

This comment has been minimized.

Copy link
@codemime

codemime Mar 26, 2017

Author Member

The interface would be cleaner if the return value included the error - return by parameter is C-ish.

Agreed. For now, I've simply repeated the signature of player::can_eat

For example, if you returned it as a const std::pair<edible_rating, std::string> &

I was thinking about that too and came up with a template class. Something like this:

template<typename T = bool, T no_error = true, T default_error = false>
class ret_val
{
    public:
        static_assert( no_error != default_error, "Error codes must differ." );

        ret_val() = delete;

        static ret_val success( const std::string &msg = std::string() ) {
            return ret_val( no_error, msg );
        }

        static ret_val failure( T code = default_error, const std::string &msg = std::string() ) {
            return ret_val( default_error, msg );
        }

        operator bool() const {
            return code == no_error;
        }

        T operator *() const {
            return code;
        }

        const std::string &str() const {
            return msg;
        }

        const char *c_str() const {
            return msg.c_str();
        }

    protected:
        ret_val( T code, const std::string &msg ) : code( code ), msg( msg ) {}

    private:
        std::string msg;
        T code;
};

I think this should be done in a separate PR for all related cases.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

I'm not sure if that sorting is a good idea, at least not without a way to turn it off.
One very important quality of menus is that they do not change if they don't need to: that they keep dimensions and contents as consistent as possible.

Is there a different item menu that is sorted by default? If not, this one should only be sorted once the player presses some sort/filter button.

@codemime

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2017

I'm not sure if that sorting is a good idea, at least not without a way to turn it off.

Neither am I. That's mostly why I put the CR tag.

One very important quality of menus is that they do not change if they don't need to: that they keep dimensions and contents as consistent as possible.

They'd be more stable if sorted by NUTRITION only. Or even solely by freshness/spoilage (as an extreme). I do think that the latter is a good idea.

Is there a different item menu that is sorted by default?

Gunmod menu is sorted using the installation odds.

If not, this one should only be sorted once the player presses some sort/filter button.

As for now, there's no way we can toggle sorting. This probably needs to be implemented.

@Coolthulhu Coolthulhu self-assigned this Mar 27, 2017

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

The display is generally fine on a sane setting, but our defaults are not sane: at 80 screen width, the new display columns force inventory columns (from map, from vehicle etc.) below the inventory ones, halving the number of items displayed.
Now, 80 screen width is generally rather painful and I wouldn't recommend playing with such a narrow viewport, but it is the default and thus we're supposed to support it.
Is there a way to compress the display to force the 2 columns if there is only one?

If not, I'll wait for others to approve. It's fine by me, but not necessarily for everyone else.

As for the smart sorting:
This one sounds like it could be painful in some cases (finding specific joy item in a huge stash etc.). I'd add an option (as in, options.cpp option) to disable it, at least at first. It can be the default, just that it shouldn't be the only option until we can sort it all out.

One thing could really help with everything here:
Being able to filter the inventory in a similar way all uimenus are filtered. If this was implemented, the sorting would be totally fine in pretty much all cases.

@Coolthulhu Coolthulhu removed their assignment Mar 27, 2017

@codemime

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2017

Is there a way to compress the display to force the 2 columns if there is only one?

Doable, but I doubt that makes sense. Long rows would either overlap each other or simply won't fit. BTW I didn't change column management in this PR.

This one sounds like it could be painful in some cases. I'd add an option (as in, options.cpp option) to disable it, at least at first. It can be the default, just that it shouldn't be the only option until we can sort it all out.

That would be equally painful with lists sorted alphabetically, but the option is an option.

Being able to filter the inventory in a similar way all uimenus are filtered.

I thought about that too and I'm going to implement that.

@codemime

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2017

I have resolved the merge conflict and removed all additional sorting except by freshness.

@codemime codemime requested a review from Coolthulhu Jul 23, 2017

if( food.rotten() ) {
const bool saprovore = has_trait( trait_id( "SAPROVORE" ) );
if( !saprophage && !saprovore ) {
consequences.emplace_back( ROTTEN, _( "This is rotten an smells awful!" ) );

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 27, 2017

Contributor

Typo

if( has_active_mutation( trait_id( "HIBERNATE" ) ) ) {
if( ( get_hunger() >= -60 && get_thirst() >= -60 ) &&
( temp_hunger < -60 || temp_thirst < -60 ) ) {
consequences.emplace_back( TOO_FULL, _( "You're adequately fueled. Prepare for hibernation?" ) );

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 27, 2017

Contributor

What will be the context of this message?
You removed questions from other messages, why not this one?

This comment has been minimized.

Copy link
@codemime

codemime Jul 29, 2017

Author Member

I probably wasn't careful enough during merging.

req << _( "Consume anyway?" );

if( !query_yn( "%s", req.str().c_str() ) ) {
return res;

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 27, 2017

Contributor

Why return the first one? Does it matter?

This comment has been minimized.

Copy link
@codemime

codemime Jul 29, 2017

Author Member

It does matter mostly for the colors in the inventory menu. Another option is choosing the most severe out of multiple choices.

This comment has been minimized.

Copy link
@codemime

codemime Jul 29, 2017

Author Member

I have reordered the consequences by severity (the order is arguable):

Type Consequences
Rotten Food poisoning, health damage
Cannibalism Long lasting morale penalty
Nausea Losing eaten food, hunger, thirst, morale penalty
Allergy Morale penalty that lasts half as long as for cannibalism
Too full Risk of sudden vomiting
Won't eat No ill effects
edible_rating can_eat( const item &food, bool interactive = false,
bool force = false ) const;

/** The food can be (theoretically) eaten. No matter the consquences. */

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 27, 2017

Contributor

Is this regarding the return value?
Would be cleaner with "Can" in front.

@Coolthulhu Coolthulhu self-assigned this Jul 30, 2017

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2017

That prompt for "really burn x for energy" is really annoying. I assume it is to prevent burning weapons and such, but this needs to be special cased for "quality" items, otherwise it will lead to common annoyance to prevent a rare edge case.
I don't see a good way of sneaking that prompt in somehow.

Also double prompt when eating something burnable for fuel that is edible. Double Y/N queries (other than "are you really, really sure? this will probably have horrible results") are pretty much always wrong and can only ever be lesser evil, not the proper solution.
After removing the "really burn" prompt, this could be fixed by simply not allowing burning edible things. Edible things are generally bottles and boxes - if someone really wants to eat one, they'll empty it and then eat the bottle, not burn a bottle filled with water.

When the furnace is turned on, burnable food containers are colored as if inedible.
Would be fixed by simply not treating them as burnable.

@Coolthulhu Coolthulhu removed their assignment Jul 30, 2017

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2017

Also a bug, but I'm not sure if it's introduced here:

  • Spawn contained fast food french fries
  • Drop all worn containers so that your free carrying capacity is zero
  • Try to eat the fries

This will result in "Too big to pick up", even though you are trying to eat them, not hold them in the inventory.
Eating isn't a long action that requires eaten items to reside in inventory. Eating from the table is perfectly fine.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2017

Fails bionic test:

bionics_test.cpp:56: FAILED:

  REQUIRE( p.can_consume( it ) == when_none )

with expansion:

  false == true

with messages:

  no power capacity at first

  adding Power Storage CBM only increases capacity

  'battery' is count-by-charges

  consume 'battery' with 0 charges
@codemime

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

The test was failing because it's assumed that discharged batteries can still be used to recharge the power storage. Seems quite impossible to reproduce with the count_by_charges item, so no big deal.

@codemime codemime changed the title [CR] Customize Consume Item menu [RDY] Customize Consume Item menu Aug 2, 2017

@codemime

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

All set.

@kevingranade kevingranade merged commit ad0eee6 into CleverRaven:master Aug 5, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 22.877%
Details
gorgon-ghprb Build finished.
Details

@codemime codemime deleted the codemime:ref_inv_n13 branch Aug 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.