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

Events - Add CBA_fnc_addBISPlayerEventHandler for adding EHs to just the player (from ACE3) #1670

Merged
merged 8 commits into from
Aug 10, 2024

Conversation

LinkIsGrim
Copy link
Contributor

@LinkIsGrim LinkIsGrim commented Jun 18, 2024

When merged this pull request will:

```
if (_ignoreVirtual && {(unitIsUAV focusOn) || {getNumber (configOf focusOn >> "isPlayableLogic") == 1}}) exitWith {};
```

@@ -0,0 +1,70 @@
#include "script_component.hpp"
/* ----------------------------------------------------------------------------
Function: CBA_fnc_addPlayerEngineEvent
Copy link
Member

Choose a reason for hiding this comment

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

The function adds an Event Handler so the name should reflect that.

We have addBISEventHandler so something like addBISPlayerEventHandler would be fitting. I don't like how the capitalized letters merge though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the capitalization either, but it's better than addPlayerEventHandlerBIS imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

anyone have a different name suggestion?
otherwise I think it's fine as is

@PabstMirror
Copy link
Contributor

I think we can replace ace_player with call CBA_fnc_currentUnit
iirc focusOn might be funny when controlling a uav, not sure

LinkIsGrim and others added 2 commits June 20, 2024 15:59
Co-authored-by: PabstMirror <pabstmirror@gmail.com>
addons/main/script_mod.hpp Outdated Show resolved Hide resolved
@commy2
Copy link
Contributor

commy2 commented Jun 21, 2024

Just a few thoughts.

  • It stylistically correctly omits a version to remove the event handlers.

  • It is not thread-safe (spawn-pseudo threads), like most the rest of CBA tbf.

  • This seems related to common component addPlayerAction, more so than events component addPlayerEventHandler. The names are unfortunate.

I don't personally like CBA_fnc_addPlayerAction and always thought of it as legacy code. I would always recommend to simply add user actions (and by extension event handlers) to every unit and then to quit early via guard clause. Is the mental overhead required for using such an obscure framework justified considering one could simply write:

if (_unit != player) exitWith {};

?

LinkIsGrim and others added 4 commits June 21, 2024 10:14
Co-authored-by: commy2 <commy-2@gmx.de>
Co-authored-by: commy2 <commy-2@gmx.de>
Co-authored-by: commy2 <commy-2@gmx.de>
@LinkIsGrim
Copy link
Contributor Author

LinkIsGrim commented Jun 21, 2024

This is only ever intended for internal use, to replace the polling in player events (hence, no removal option) and so the mental overhead is a one-time thing during implementation IMO. I don't like the idea of adding the EHs to every unit, due to the overhead (though it might still be better than polling). unit playerEH doesn't fire that often (basically only on respawn) for a non-curator on the other hand, and that's already laggy, so it doesn't matter.

I can make this exit on canSuspend, but again, this should only ever be called by CBA itself a few times at the start of a mission.

@PabstMirror
Copy link
Contributor

There is some performance savings by only adding events you need.
e.g. FiredNear could almost be considered n^2 based on unit count

We can make it safe by wrapping in a CBA_fnc_directCall; like https://github.com/CBATeam/CBA_A3/blob/master/addons/events/fnc_addEventHandler.sqf

@LinkIsGrim
Copy link
Contributor Author

LinkIsGrim commented Jun 22, 2024

Implemented as public function. Added removal, thread-safety, and multiple events of the same type can be added now.

Do I need to touch anything myself for docs?

@PabstMirror PabstMirror changed the title Events - Port addPlayerEH from ACE3 Events - Add addBISPlayerEventHandler for adding EHs to just the player (from ACE3) Aug 8, 2024
@PabstMirror PabstMirror changed the title Events - Add addBISPlayerEventHandler for adding EHs to just the player (from ACE3) Events - Add CBA_fnc_addBISPlayerEventHandler for adding EHs to just the player (from ACE3) Aug 8, 2024
@PabstMirror PabstMirror merged commit 6454723 into CBATeam:master Aug 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants