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 Treatment - Add setting to rollover bandage effectiveness #8426

Merged
merged 20 commits into from
Sep 10, 2023

Conversation

LinkIsGrim
Copy link
Contributor

@LinkIsGrim LinkIsGrim commented Sep 5, 2021

When merged this pull request will:

  • Change bandaging to use leftover bandage effectiveness on other wounds in same limb.
  • Controlled by ace_medical_treatment_bandageRollover setting. False maintains old behavior.
  • Add CBA Setting controlling overall bandage effectiveness.

Example: Applying Packing Bandage on limb with 1 Medium Velocity Wound (bandage effectiveness of 1.5) and 1 Small Velocity Wound (bandage effectiveness of 2) in the same limb.

Medium velocity will be closed but bandage still has 33% of its effectiveness (1.5 - 1 / 1.5). 0.66 (2 * .33) Small Velocity will be closed afterwards. Repeat while bandage still has left over effectiveness or there are still wounds in the same limb.

LinkIsGrim and others added 2 commits September 5, 2021 11:52
Co-authored-by: Kyle Mckay <5459452+kymckay@users.noreply.github.com>
@LinkIsGrim LinkIsGrim force-pushed the bandage-useleftovereffectiveness branch from 3feef8b to 3d5c288 Compare September 5, 2021 15:01
@LinkIsGrim
Copy link
Contributor Author

Anything outstanding that needs to be addressed with this?

@jonpas
Copy link
Member

jonpas commented Oct 12, 2021

I haven't seen this PR before, thanks for bringing it to attention.

I am not sure I like this. If I have 2 separate wounds on my arm I wouldn't expect 1 bandage to cover both, they probably are not close enough.

Maybe a setting, but then it seems to be too close to just general effectiveness factor at that point.

@LinkIsGrim
Copy link
Contributor Author

LinkIsGrim commented Oct 12, 2021

By that logic, shouldn't bandage effectiveness be capped at 1x for any wound? I understand 2x Velocity Wounds as being two separate gunshot wounds, not one really large wound, and I suspect the same for most of the userbase.

If the _amountOf property of wounds is to be interpreted as amount of wounds and not severity, then this is an extension of current functionality. If that's not the case, then you'd be right, but I'd say some variable names need to be changed around the codebase. My justification for this PR was the comment on line 30 of fnc_bandageLocal, which implies effects of this PR were originally intended, but either weren't finished/figured out on time for the Medical Rewrite's release.

@jonpas
Copy link
Member

jonpas commented Oct 12, 2021

Hmm, I see, you are probably right. I'll bring this up internally and see if we can get a positive answer from someone more familiar with medical specifically. Your justification seems correct to me though.

@xfunnypigx
Copy link

I am not sure I like this. If I have 2 separate wounds on my arm I wouldn't expect 1 bandage to cover both, they probably are not close enough.
Maybe a setting, but then it seems to be too close to just general effectiveness factor at that point.

By that logic, shouldn't bandage effectiveness be capped at 1x for any wound? I understand 2x Velocity Wounds as being two separate gunshot wounds, not one really large wound, and I suspect the same for most of the userbase.

Well, you could argue that these wounds could be close together. If we go by severity, maybe a bandage capable of treating 1 large wound could then be applied over 3 small ones. Improbable, but not impossible. I believe that this could be a setting, especially useful for those seeking a more softcore experience, who still wish to use ACE's depth without the.. uhh.. involved labor, if that makes any sense.
I feel that both approaches make sense and could be argued in their own ways. If we go by realism, I would go by the 1x cap, for playability, accessibility, and choice, a setting for the multi-use could be argued for, as well.

Of course, as a non-coder, I genuinely have no idea how easy or hard it would be to implement this, take it with a grain of salt.

@PabstMirror
Copy link
Contributor

We need to test how this effects ace_medical_treatment_fnc_getBandageTime as it uses the results of findMostEffectiveWound
If we are "doing more" with the same bandage we should have to increase bandage time.

@PabstMirror PabstMirror added the kind/enhancement Release Notes: **IMPROVED:** label Oct 12, 2021
@PabstMirror PabstMirror added this to the Ongoing milestone Oct 12, 2021
@LinkIsGrim
Copy link
Contributor Author

We need to test how this effects ace_medical_treatment_fnc_getBandageTime as it uses the results of findMostEffectiveWound If we are "doing more" with the same bandage we should have to increase bandage time.

Bandage time returns same as it would without this PR. If logic is "more wounds = more time", then that logic should be changed to be (_amount min _effectiveness) * _bandageTime, with _bandageTime being the time for each size category (4, 6 and 8 seconds) instead of current implementation which uses linearConversion with range _effectiveness.

@Drofseh
Copy link
Contributor

Drofseh commented Oct 12, 2021

IRL if the wounds are close enough together and the banage is large enough you could cover multiple wounds with one bandage.
For us the problem is that there's no way to know if a wound is adjacent to another wound.

I wouldn't mind capping effectivemess at 1, but I think we would also have to look at capping the number of each size of wounds that each body part can receive, and combining smaller wound types into larger ones when that makes sense.
For example, give arms something like a max of 8 small, 4 medium, and 2 large wounds. If a unit already has a 2 medium wounds and would normally get another medium wound, have a chance to change one into a large wound instead, giving them 1 medium and 1 large wound.

@LinkIsGrim
Copy link
Contributor Author

What I'd rather do is refactor wounds entirely with HashMap, do away with the size categories and use severity based on the damage that caused the wound instead (ie, 5.56 makes a single Velocity Wound with severity 2.3, while .50 BMG makes a single Velocity Wound with severity 6), esp. since with #8278 high damage is more likely to cause multiple wounds. This'd allow for mostly keeping current getBandageTime logic, simplify iteration over wounds where it's needed (stitching, bandaging, and reopening) and deal away with the need to even have this PR, since then we could just say that a bandage addresses a single wound and that's it.

@kymckay
Copy link
Member

kymckay commented Jul 14, 2022

_amountOf is intended to represent percentage of wound taking effect, not a count of sub-wounds or similar.

I think we can go ahead with this change, bandaging and wounding involves some abstraction and I know for sure when we rewrote medical we intended to allow bandages to effect multiple wounds as part of that abstraction.

Edit: There's almost certainly discussion of this somewhere in old PRs. There should be one for when I first introduced the bandage time function somewhere that might have relevant discussion.

@LinkIsGrim
Copy link
Contributor Author

git why

@LinkIsGrim LinkIsGrim closed this Jul 1, 2023
@LinkIsGrim LinkIsGrim reopened this Jul 1, 2023
@LinkIsGrim LinkIsGrim changed the base branch from master to BFT July 1, 2023 00:46
@LinkIsGrim LinkIsGrim changed the base branch from BFT to master July 1, 2023 00:46
@LinkIsGrim LinkIsGrim changed the title Medical - Use leftover bandage effectiveness Medical Treatment - Add setting to use rollover bandage effectiveness Jul 1, 2023
@LinkIsGrim LinkIsGrim requested a review from kymckay July 1, 2023 00:51
@LinkIsGrim LinkIsGrim changed the title Medical Treatment - Add setting to use rollover bandage effectiveness Medical Treatment - Add setting to rollover bandage effectiveness Jul 1, 2023
*
* Return Value:
* [Wound, Index, Effectiveness] <ARRAY, NUMBER, NUMBER>
* [Wound, [Index, Effectiveness, Impact]] <HASHMAP> of <ARRAY>:<NUMBER, NUMBER, NUMBER>
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
* [Wound, [Index, Effectiveness, Impact]] <HASHMAP> of <ARRAY>:<NUMBER, NUMBER, NUMBER>
* [Wound, [Effectiveness, Index, Impact]] <HASHMAP> of <ARRAY>:<NUMBER, NUMBER, NUMBER>

[LSTRING(bandageRollover_DisplayName), LSTRING(bandageRollover_Description)],
[ELSTRING(medical,Category), LSTRING(SubCategory_Treatment)],
true,
false // server can force if necessary, otherwise client decides
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why, this means everyone can decide how effective their bandages are, that doesn't make sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a tradeoff, bandage slower in exchange for not wasting your bandage's effectiveness (if you're even bandaging more than one kind of wound at once, which is relatively uncommon). You're not getting more effectiveness than whatever's in the config, overall.

I see it as no different as choosing to only use my Elastic Bandages on Large Avulsions or wounds with count > 1.

Copy link
Member

Choose a reason for hiding this comment

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

Aah that makes sense. So if I leave this as default it will just take longer to bandage when it will rollover, and if I disable it rollover won't happen but bandaging would be faster.

@LinkIsGrim LinkIsGrim modified the milestones: Ongoing, 3.16.0 Sep 10, 2023
@LinkIsGrim LinkIsGrim merged commit de8940a into acemod:master Sep 10, 2023
5 checks passed
@LinkIsGrim LinkIsGrim added kind/feature Release Notes: **ADDED:** and removed kind/enhancement Release Notes: **IMPROVED:** labels Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Release Notes: **ADDED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants