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

Player needs to have painkiller effect on him to install bionics #20892

Merged
merged 10 commits into from Jul 13, 2017

Conversation

Projects
None yet
8 participants
@Night-Pryanik
Copy link
Member

commented Apr 22, 2017

As per @kevingranade's suggestion in #17698.
Number of charges for painkillers are open to discussion.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2017

Hardcoding the painkillers by ID is a giant no-go here.
What happens here is that installation of bionics magically consumes painkillers of specific types without actually having anyone ingest them.

The simplest way to actually sensibly do it would be to check pkill stat. Then it would actually involve painkiller consumption.
But then, the message needs to be sensible, to avoid overdose. All the good ideas for messages I see here involve revealing current pkill level directly.

It would be better to just cause a lot of pain right before the installation check and adjust the displayed chance to use the heightened pain. Then the pain would naturally affect ability to install bionics by penalizing intelligence.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2017

This also leaves an obvious lore concern: the bionic modules are being sold in cutting-edge electronics stores, and also outright claim to be self-contained surgery kits. What kind of surgery kit would not at least include some local anesthetics.

Unless the future is one where advanced, potentially dangerous, bionics can be freely brought, but the drugs needed to safely install them are still heavily regulated.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 22, 2017

Unless the future is one where advanced, potentially dangerous, bionics can be freely brought, but the drugs needed to safely install them are still heavily regulated.

Sounds about right :P

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 22, 2017

The, "you need painkillers" approach does seem rather nice, it also has a nice side effect that you can install several in a row without having to take a new batch of painkillers.

Rather than dumping a number, it'd be better to be a bit vague about it.
pkill = 0 : "You need to take some painkillers to make installing tolerable."
pkill < 50 : "You need to be a lot more numb to tolerate installing your ."
pkill < 100 : "You aren't quite numb enough to tolerate installing your ."

Also it needs to be mutation aware, there's at least one pain immunity mutation, and cennobite would be fine with it.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2017

The problem with pkill messages is that there should be some measure of painkillers not yet active.
At least a single special case for effects that cause pkill, to avoid the situation where players take 3+ heroins in a row and die thinking that they still need more to install the bionic.

Having "dumb" messages special cased for painkiller effects would be enough to be workable, but the message would not be very informative.
Something like "The painkillers you've taken haven't fully started working yet".

A much better way would be to calculate it from actual effects (not special cased by ids) and give messages like "the painkillers you took won't be enough for installation" and "the painkillers you took are strong enough but aren't fully acting yet" basing it on caps on the effects.

@Zilenan91

This comment has been minimized.

Copy link

commented Apr 22, 2017

What items in particular are classed as painnkillers that would work with this?

@Night-Pryanik

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2017

What items in particular are classed as painnkillers that would work with this?

All painkillers: aspirin, codeine, oxycodone, heroin, tramadol.

@Night-Pryanik Night-Pryanik changed the title Player needs a sufficient amount of strong painkillers to install bionics [WIP] Player needs to have painkiller effect on him to install bionics Apr 23, 2017

@Cyrano7

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

I would be in favor of taking this a step further even. Like needing disinfectant and a blade like a scalpel for install. The cbm descriptions would need to changed though. As it is now you can home make cbms or cut them out of dead zombies and just pop them in your body like nothing. Basically taking them down from all including auto surgery kits to just body enhancements would be better I think.

@Cyrano7

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

Even adding a after pain effect would be good. Currently you can chain install as many cbms in a row as you want with no recovery time needed. The body can only take so much surgery at once. Having a pain resistant, toughness, or fast healing mutation should help counter act some of this though.

@Night-Pryanik

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

Maybe we shouldn't force player to take painkillers, but instead ask him like "Installing bionics is very painful, you should take a lot of painkillers prior to installation. Continue yes/no?"?
Though that would give two popups in a row if player chooses" yes"...

@Chezzo

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2017

All painkillers: aspirin, codeine, oxycodone, heroin, tramadol.

How about pot or booze?

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2017

Though that would give two popups in a row if player chooses" yes"...

It could (should) be just integrated in the query. Preferably with a big red warning, with rest of the text having neutral colors.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

Maybe we shouldn't force player to take painkillers

This is the rationale:

  • Installing bionics is invasive surgery.
  • Invasive surgery causes pain.
  • Pain interferes with player faculties necessary to complete installation.
  • Installing bionics without painkillers will cause installation to fail (outside of specific situations, like pain-ignoring mutations).
  • Allowing the player to attempt an impossible action is a bad user experience.
  • Painkillers should be mandatory.
@Chezzo

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2017

Wouldn' t pain_killers_ also interfere with player faculties necessary to complete installation?

I assumed the CBMs themselves contained local anesthetic.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

Yes, but to a much lesser extent than pain.
Nope, especially if extracted from a corpse.

The problem with having them included with the CBM is you could end up ODing on them if you installed several in a row, or else wasting anesthetic because it's not needed.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2017

Overdosing on local anesthesia seems highly questionable, compared to general anesthesia.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

I'm answering questions, but as far as I'm concerned the direction is set as of #20892 (comment)
We're not seriously considering local anesthesia (for one thing we don't have a system in place for it, and it would also be inadequate for a major surgery like this) or packaging painkillers with the cbm or making painkillers optional.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2017

We're not seriously considering local anesthesia

If we are considering the need for pain management for realism purposes, then local anesthesia is a prerequisite.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

It's abstracted, we don't have local vs general anesthetic, and adding something that acts like local anesthetic is well outside the scope of this PR.

@Night-Pryanik

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

A much better way would be to calculate it from actual effects

I'm stuck on finding a good way to find a cap for not yet fully active painkiller effect.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

You can get the painkiller cap of an effect by:

effect_here.get_max_val("PKILL", reduced)

You can assume reduced to be false. This won't always be true, but for unmodded game that's totally enough.

Then you want to add a getter to Creature::effects. The structure of effects is a bit awkward because it is a map of maps, but since you only want to iterate over all of them it should be easy enough.

Characters with pain resistance can take less painkillers
Characters with pain immunity can take no painkillers at all.
@Night-Pryanik

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

int pkill_cap = get_effect(effect_pkill2).get_max_val("PKILL", false);

Is this right (for pkill2)? pkill_cap is showing 0 in this case...

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

Looks like max pkill is equal to 0 if there is no cap.
So you need to check min pkill too, with get_min_val, and treat anything with min > 0 && max == 0 as infinite.
If get_min_val isn't it, then it could be get_mod. Effects code is convoluted and messy, I can't tell which one will work, but I'm pretty sure there is a getter that returns the correct field.

But don't hardcode the check for individual effects. Check all effects currently on player.

Different messages for different pk levels
Also, cenobites don't care for pain in process.
@Night-Pryanik

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

I wasn't able to find a good way to show actual levels of not fully active painkillers to implement @Coolthulhu suggestion, so I just added a vague messages as per @kevingranade proposed, and a reminder for players to watch for their painkiller level.

@Night-Pryanik Night-Pryanik changed the title [WIP] Player needs to have painkiller effect on him to install bionics Player needs to have painkiller effect on him to install bionics Apr 26, 2017

const int pk = get_painkiller();
int pain_cap = 100;
if( has_trait( "PAINRESIST_TROGLO" ) ) {
pain_cap = roll_remainder( pain_cap * 0.5f );

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 26, 2017

Contributor

Drop the roll remainder thing: this is an UI restriction, meaning that attempting it takes no time. There should be no rolls in UI.

if( has_trait( "PAINRESIST_TROGLO" ) ) {
pain_cap = pain_cap / 2;
} else if( has_trait( "PAINRESIST" ) ) {
pain_cap = pain_cap / 3;

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 26, 2017

Contributor

Just 33%?

Night-Pryanik added some commits Apr 26, 2017

@kevingranade kevingranade merged commit 4705bbb into CleverRaven:master Jul 13, 2017

3 checks passed

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

@Night-Pryanik Night-Pryanik deleted the Night-Pryanik:patch-4 branch Jul 13, 2017

@Treah

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

So I wanted to comment on this as I have seen quite a lot of comments on this. Why was this PR approved to be merged? It had several gameplay problems as noted even by the author who stopped working on it back in April. There have been several comments even by some of the devs that painkillers are buggy and give limited feedback to the user to prevent YASS. Requireing them to install something should give more then just a vague feedback to the user to prevent them from overdosing. I know this change was implemented because of realism reasons but because there is no feedback it makes for a bad gameplay experience.

Kevin you even noted that there are issues with localized pain killing effects in your comment here

It's abstracted, we don't have local vs general anesthetic, and adding something that acts like local anesthetic is well outside the scope of this PR.

Why use a general mechanic to commit this PR when we have a limited system to accurately simulate the surgery being performed?

Again Kevin your below quote causes issues here.

Allowing the player to attempt an impossible action is a bad user experience.

Not having the correct pain killer level does allow them to attept it tho and says you dont have sufficant amount? However were not showing the user that amount but a vauge message. You cant have it both ways, Vauge message and another one asking for a specific amount. Esp with painkillers that have effect over time values... Unless its generally known how long each pain killer takes to become fully effective ( such as a tool tip ) then the user will not know exactly how much or how little is needed with out just blindly trying. This is a bad user experience as well.

The point of this is that this PR should not have been merged without some of the problems being fleshed out and corrected. I'm not saying you shouldn't need them but if the author was going to require something they need to fix that something before requiring it if that something has other underlying issues.

A better approach to this problem would have been to have all bionics cause massive pain when installing them. This would require the user to have to take pain killers in order to stay effective in combat or to have a successful roll in installing them. Reducing the chance of a successful installation when the player is under the influence of painkillers would have been a better approach and more realistic rather then just preventing it.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

I am afraid I must agree, This is just the latest example of a pull request where the merging developer approved a change that had not been adequately tested, in addition to ignoring all feedback offered.

@Night-Pryanik

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2017

It had several gameplay problems as noted even by the author who stopped working on it back in April.

It had only information/feedback problems, if any, not gameplay ones. I stopped working on it because I considered it completed, not because I thought that these problems are significant enough not to merge it.

The point of this is that this PR should not have been merged without some of the problems being fleshed out and corrected.

Everyone had plenty of time to test and criticize this PR before it was merged. But most of the players' feedback popped up and problems flashed out after it was merged. All the complaints like "it was not thoroughly tested" isn't very appropriate, I think. I tested it (obviously), @kevingranade tested it, maybe someone else. We are not perfect, this PR is not perfect, but at the moment of writing and at the moment of merging this PR looked good enough.

This branch of development called "Experimental" not without reason. Changes are added, tested, sometimes considered bad and reverted. This PR was merged, then some issues with it was found. I tried to address these issues (#21458). I think this is a normal development process. And improving PR that wasn't perfect at the moment of merge, after receiving players' feedback, is normal too, and IMO is much better than cries like "Remove it immediately!".

@DangerNoodle

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

Everyone had plenty of time to test and criticize this PR before it was merged. But most of the players' feedback popped up and problems flashed out after it was merged.

You are incorrect.

Also it needs to be mutation aware, there's at least one pain immunity mutation, and cennobite would be fine with it.

You neglected full implementation of this, as pull request #21433 indicates.

The problem with pkill messages is that there should be some measure of painkillers not yet active.
At least a single special case for effects that cause pkill, to avoid the situation where players take 3+ heroins in a row and die thinking that they still need more to install the bionic.

You neglected full implementation of this, as pull request #21458 indicates.

If we are considering the need for pain management for realism purposes, then local anesthesia is a prerequisite.

Disregarded by merger, while you neglected to address it.

@CleverRaven CleverRaven locked and limited conversation to collaborators Jul 25, 2017

@kevingranade

This comment has been minimized.

Copy link
Member

commented Jul 25, 2017

This has crossed a line from argument to harassment, and it stops now.
@Treah, @DangerNoodle your concerns have been noted, and they were noted when the issue was open, but I disagree that they are valid.

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