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

Medical - Add Pain Unconscious Threshold setting #8394

Merged
merged 7 commits into from
Aug 5, 2023

Conversation

severgun
Copy link
Contributor

@severgun severgun commented Aug 25, 2021

When merged this pull request will:

  • Add slider to set PAIN_UNCONSCIOUS which currently is just magic variable not even mentioned in wiki
  • Add English and Russian stringtables

Add PAIN_UNCONSCIOUS slider to ACE Medical settings
addons/medical_damage/initSettings.sqf Outdated Show resolved Hide resolved
@BaerMitUmlaut
Copy link
Member

I believe this was an intentionally hidden setting?

@severgun
Copy link
Contributor Author

severgun commented Aug 26, 2021

I believe this was an intentionally hidden setting?

Why? What is the point?
As much as I remember IRL there were episodes when people lose conscious after hit in vest even without penetration. To get this I can lower threshold or bump up caused pain. First require less effort.

@kymckay
Copy link
Member

kymckay commented Aug 26, 2021

See #7236 for the introduction of these constants.

I would tend to agree with the original reasoning, but I'm admittedly pretty picky about our settings UX. If we were to introduce settings for things like this I'd like to see us improve the presentation with a clear separate "ACE Medical (Advanced Settings)" category for things which we want to say don't need to be changed, but can be for power editors who want to really fine tune behaviour.

Also I don't like that you changed the conditional logic with no justification 😛

@severgun
Copy link
Contributor Author

severgun commented Aug 26, 2021

See #7236 for the introduction of these constants.

And? They are already moved from constants to variables with reason allows manual tweaking of ace if desired without having to build a custom pbo

I think making these settings is overkill

No reasoning on that. May be just laziness.

but can be for power editors who want to really fine tune behaviour.

Then all that magic must be described in wiki

Also I don't like that you changed the conditional logic with no justification 😛

  1. It is >= here:
    if ((_headDamage > _damageThreshold / 2) || {_bodyDamage > _damageThreshold} || {(_painLevel >= PAIN_UNCONSCIOUS) && {random 1 < EGVAR(medical,painUnconsciousChance)}}) then {

    why outer condition is more restrictive?
  2. Maybe it's just for me? When I set threshold I expect that value is included. If I set 0.5, I expect 0.5 and above or 0.5 and below

You can close this PR. But, please, mention var in wiki.

@kymckay
Copy link
Member

kymckay commented Aug 26, 2021

And?

I was providing context to this (intended to be) collaborative change proposal discussion.

No reasoning on that. May be just laziness.

It seems reasonable to infer that the reasoning is because they are not behavioural modifiers that warrant settings as they're not relevant to 99% of users (i.e. are overkill).

Then all that magic must be described in wiki

I agree that documentation is good. However, sometimes in software you want to expose things for power users without forming a "contractual agreement" (i.e. formally declaring a public API) to maintain them in future versions (knowing that power users are accepting the responsibility to keep their downstream code up to date). That's not necessarily the case here, but one possible explanation.

It is >= here: ... why outer condition is more restrictive?

Thank you for providing justification, I agree that there's an inconsistency there (although the outer condition being more restrictive makes more sense if you think about the intersection of those).

Maybe it's just for me? When I set threshold I expect that value is included. If I set 0.5, I expect 0.5 and above or 0.5 and below

Personally I think context dependent. You would call 50% the threshold for a majority right? Now would you call precisely 50% a majority?

Co-authored-by: Salluci <69561145+Salluci@users.noreply.github.com>
Co-authored-by: jonpas <jonpas33@gmail.com>
@jonpas jonpas changed the title Medical - Add PAIN_UNCONSCIOUS slider Medical - Add Pain Unconscious Threshold setting Sep 2, 2021
@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Sep 4, 2021

UX would benefit from switching the hardcoded values to the variable here:

// Indicate the amount of pain the unit is in
if (_target call EFUNC(common,isAwake)) then {
private _pain = GET_PAIN_PERCEIVED(_target);
if (_pain > 0) then {
private _painText = switch (true) do {
case (_pain > 0.5): {
ELSTRING(medical_treatment,Status_SeverePain);
};
case (_pain > 0.1): {
ELSTRING(medical_treatment,Status_Pain);
};
default {
ELSTRING(medical_treatment,Status_MildPain);

@severgun
Copy link
Contributor Author

severgun commented Sep 6, 2021

UX would benefit from switching the hardcoded values to the variable here:

Pain always in 0-1 range. I don't see who and when need to change this localization switch.
Also this PR considered undesirable.

@olimolly
Copy link
Contributor

Since the february pain system redesign update had consequences on const_painUnconscious.

This request should be reconsider to adapt players preferences.

Effect of what was at 0.5 default value is more like 0.35 or ~0.4 now. Which I recommand as default value.
It would fix #9027 issue in the same time.

@LinkIsGrim
Copy link
Contributor

I think this is valid to have as a setting. We can move it to a separate settings menu if it ends up being an issue, I've been meaning to categorize the medical settings independently anyway.

@LinkIsGrim LinkIsGrim added the kind/enhancement Release Notes: **IMPROVED:** label Jul 29, 2023
@LinkIsGrim LinkIsGrim added this to the 3.16.0 milestone Aug 2, 2023
@@ -34,6 +34,15 @@
true
] call CBA_fnc_addSetting;

[
QEGVAR(medical,painUnconsciousThreshold),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
QEGVAR(medical,painUnconsciousThreshold),
QUOTE(PAIN_UNCONSCIOUS),

technically it could be this I don't really like it and would rather keep it as is

@LinkIsGrim LinkIsGrim merged commit 96a4a8c into acemod:master Aug 5, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants