-
Notifications
You must be signed in to change notification settings - Fork 740
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 Vitals - Extract update logic to separate functions #10566
base: master
Are you sure you want to change the base?
Conversation
On another note:
Drop 'em? |
Co-authored-by: Jouni Järvinen <[email protected]>
}; | ||
} forEachReversed _ivBags; | ||
|
||
(GVAR(deferredEvents) getOrDefault [_unit, [], true]) pushBack ([QEGVAR(medical,consumedIVs), [_unit, _consumedIVs]]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key can't be an object, you need to use hashValue
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally not a huge fan of having a global hashmap for this kind of thing:
- If the unit is deleted, you need to handle the removal from the hashmap.
- If the unit switches locality, this can't handle it. Idk how important that is though.
Why not use an object variable instead? Handles 1) and no longer requires hashValue _unit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hashmap is slightly faster (might not be with hashvalue), that was the only incentive. If we don't have to worry about keeping 3rd-party code out of the counter, I'd rather just raise the events outright.
locality change and deletion won't (shouldn't) happen mid-execution, deferred events are still executed on the same frame, so it'll be either before or after handleUnitVitals is executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather just raise the events outright
Do that then, makes more sense.
When merged this pull request will:
Event deferral is just to keep any code from the events out of the vitals counter, but they can just be raised straight with no problems.
IMPORTANT
Component - Add|Fix|Improve|Change|Make|Remove {changes}
.