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 Damage - Improve custom wound handling #9310

Merged
merged 10 commits into from
Aug 11, 2024

Conversation

johnb432
Copy link
Contributor

@johnb432 johnb432 commented Aug 6, 2023

When merged this pull request will:

  • Addresses Custom wound handlers cannot be added when given function nil during pre-init #8927 by wrapping entry in call _fnc block if _fnc in preInit and reparsing the config in postInit, when all functions should be compiled. This way slower versions are available during preInit, but after postInit the call wrap is removed to speed things up slightly.
    I'm not sure how well this new way can account for code in preInit specficially (see example below), but I'm not sure if this system was ever meant for handling code directly.
class ACE_Medical_Injuries {
    class damageTypes {
        class woundHandlers;
        class bullet {
            class woundHandlers: woundHandlers {
                ADDON = QUOTE({systemChat str _this; _this});
            };
        };
    };
};
  • If a wound handler returns an invalid return, it will be ignored. However it will no longer stop checking other wound handlers; it will continue on until it has gone through all wound handlers. No longer the case.
  • Invalid wound handler return now includes nil.
  • Minor code cleanup.

IMPORTANT

  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
  • Development Guidelines are read, understood and applied.
  • Title of this PR uses our standard template Component - Add|Fix|Improve|Change|Make|Remove {changes}.

@LinkIsGrim
Copy link
Contributor

However it will no longer stop checking other wound handlers; it will continue on until it has gone through all wound handlers.

I'm not sure about this. I don't think many mods have modified wound handling yet, but I still don't think it's a safe change.

@johnb432
Copy link
Contributor Author

However it will no longer stop checking other wound handlers; it will continue on until it has gone through all wound handlers.

I'm not sure about this. I don't think many mods have modified wound handling yet, but I still don't think it's a safe change.

I don't understand why it was designed this way in the first place: Why should the evaluation of all wound handlers be stopped if a single one fails?
A third party mod might easily return something invalid, which would prevent another 3rd party mod from having their wound handler from running.

In fact, as it is now it's safer imo: It checks isNil, which it didn't before, leading to possibly unpredictable behaviour.

@LinkIsGrim
Copy link
Contributor

@pterolatypus your code. Thoughts?

@johnb432 johnb432 added the kind/enhancement Release Notes: **IMPROVED:** label May 27, 2024
@pterolatypus
Copy link
Contributor

pterolatypus commented Jun 19, 2024

Appreciate I'm very late to this, been out of the game for a long time, but for the sake of information:
Having at least some way to stop handling was intentional, so that a custom handler can tell the system "this event has been dealt with" and not duplicate handling. Think of the return as "damage still left to be handled".
An example of this is fnc_woundsHandlerBurning which deals with the incoming small damage ticks and takes responsibility for passing them on later. Because it "handles" all of the damage, there is no damage left afterwards.
I suppose you could return something like [_unit, [], _damageType] which would have a similar effect, but I feel like the intention of returning [] is pretty clear and the current implementation lets us just skip subsequent handlers that probably wouldn't have done anything anyway.

If you're worried about someone's bad code causing a problem, you could make it only terminate when it gets exactly [] and continue in any other invalid case. Personally, though, I'd just tell the offending modder to read the docs and fix their code.

As to the bug fix: the current implementation does handle arbitrary expressions but I doubt anyone uses it as they're only actually called at PreInit, only the return value is stored. If you wrap it like that, the whole expression would get called on every wound event. I think that's fine, but you should probably just wrap it in every case so that it's consistent.

Either way, your example should be more like:

ADDON = "{systemChat str _this; _this}";

because this entry is supposed to be an expression that returns a handler function.

@johnb432
Copy link
Contributor Author

https://ace3.acemod.org/wiki/framework/medical-framework#44-wound-handler-function says that [] does indeed make it stop, so I've removed that change. I'm not going to change anything further, apart from considering nil as an invalid return.

@johnb432 johnb432 added this to the 3.18.0 milestone Jul 8, 2024
@LinkIsGrim LinkIsGrim merged commit c8e7b63 into acemod:master Aug 11, 2024
5 checks passed
@johnb432 johnb432 deleted the custom-wound-handlers-fix branch August 12, 2024 06:54
blake8090 pushed a commit to blake8090/ACE3 that referenced this pull request Aug 18, 2024
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.

Custom wound handlers cannot be added when given function nil during pre-init
3 participants