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

add '_thisArgs', '_thisId', '_thisType' and '_thisFnc' and to CBA events #375

Merged
merged 5 commits into from
Jun 13, 2016

Conversation

commy2
Copy link
Contributor

@commy2 commy2 commented Jun 11, 2016

Closes: #374

Adds the same meta data CBA_fnc_addBISEventHandler has to the CBA events.

  • adds optional parameter to CBA_fnc_addEventHandler to assign arguments to the event.

_thisArgs - Arguments passed when adding the event handler.
_thisId - Id that can be used to remove the event via CBA_fnc_removeEventHandler
_thisType - the name of the event ("testEvent" in ["testEvent", ...] call CBA_fnc_addEventHandler)
_thisFnc - function that is executed when the event happens

Note: If the arguments passed to CBA_fnc_addEventHandler are an array, they are stored as array reference and not as copy. That means you can modify these arguments even after the event was added.

Needs some more testing, will do later ...

@commy2 commy2 added the Feature label Jun 11, 2016
@commy2 commy2 added this to the 2.4.1 milestone Jun 11, 2016
@@ -33,7 +33,9 @@

#define CALL_EVENT(params,event) {\
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the macro argument "params" be renamed so as to not collide with the reserved word (command) params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That collided with the params command I added ...

@commy2
Copy link
Contributor Author

commy2 commented Jun 11, 2016

One time event handler printing "hello world"

["test1", {
    systemChat str _thisArgs;
    [_thisType, _thisId] call CBA_fnc_removeEventHandler
}, "hello world"] call CBA_fnc_addEventHandler;

...

"test1" call CBA_fnc_localEvent

Problem is that you are modifing an array of GVAR(eventsNamespace) while iterating through it via forEach which is undefined ... Searching for a work-around.

@commy2
Copy link
Contributor Author

commy2 commented Jun 11, 2016

done

@commy2 commy2 modified the milestones: 2.4.2, 2.4.1 Jun 11, 2016
@PabstMirror
Copy link
Contributor

I'm really not sure if any of this is necessary, my main concern is that there will be a performance cost to adding these args.

For 10x localEvent calls for something with 3 added ehs:

Old: 0.218436 ms 0.216403 ms 0.217628 ms
New: 0.326691 ms 0.327654 ms 0.324465 ms

@commy2
Copy link
Contributor Author

commy2 commented Jun 11, 2016

Good point. It's performance vs. functionality. I have no idea with what we should go here.
+0.10 ms isn't that bad though honestly

@PabstMirror
Copy link
Contributor

ACE calls multiple EHs for each bullet fired, so 50% increase isn't good

I think we can do most of what we need by using a function wrapper. Check out my demo code here (not tested, more of an idea): https://github.com/CBATeam/CBA_A3/compare/addEventHandlerWrapper?expand=1

@commy2
Copy link
Contributor Author

commy2 commented Jun 11, 2016

Is it a 50% increase even when measuring with the actual event function you are calling?

@PabstMirror
Copy link
Contributor

Prep:
["test", {x1=_this;}] call CBA_fnc_addEventHandler;
["test", {x2=_this;}] call CBA_fnc_addEventHandler;
["test", {x3=_this;}] call CBA_fnc_addEventHandler;

Test:
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;
["test", []] call CBA_fnc_localEvent;

@commy2
Copy link
Contributor Author

commy2 commented Jun 11, 2016

My point was that {x1=_this;} is an extremly simple function. The bigger the event function is, the smaller the share of the overhead of CBA_fnc_localEvent becomes.

@commy2
Copy link
Contributor Author

commy2 commented Jun 11, 2016

@PabstMirror I read the link you posted.
I like that idea. It would mean having no additional overhead by default, but an alternative addEventHandler function that would apply all these variables.
Any naming suggestions for FUNC(addBigEH)?

CBA_fnc_addEventHandlerArguments ??
CBA_fnc_addArgumentsEventHandler ??

I'd still have to copy the array though, but that is only once per event call and not once per event per call.

@Killswitch00
Copy link
Contributor

addEventHandlerArgs, addArgsEventHandler

An alternative/wrapper function sounds like the best way forward, so as to not compromise performance of the original function. Now, to think of a suitable name...

@commy2
Copy link
Contributor Author

commy2 commented Jun 12, 2016

Done.

I'm going with addEventHandlerArgs instead of the usual addXEventHandler, because it isn't actually a new thing on it's own. It also uses the same removeEventHandler function.

@Killswitch00 Killswitch00 merged commit 6e148c9 into master Jun 13, 2016
@thojkooi thojkooi deleted the add_event_meta_data_to_CBA_events branch June 16, 2016 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants