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

Explosive events, fix #3104 #3106

Merged
merged 1 commit into from
Feb 4, 2016
Merged

Explosive events, fix #3104 #3106

merged 1 commit into from
Feb 4, 2016

Conversation

bux
Copy link
Member

@bux bux commented Jan 1, 2016

When merged this pull request will:

  • add 2 new events: defuse and explodeOnDefuse
  • add _unit to place event

Can be tested with:

["ace_explosives_place", {
    hint format ["Placed:\nexpl: %1\nunit: %2", _this select 0, _this select 3];
}] call ace_common_fnc_addEventHandler;

["ace_explosives_defuse", {
    hint format ["Defused:\nexpl: %1\nunit: %2", _this select 0, _this select 1];
}] call ace_common_fnc_addEventHandler;

["ace_explosives_explodeOnDefuse", {
    hint format ["ExplodedOnDefuse:\nexpl: %1\nunit: %2", _this select 0, _this select 1];
}] call ace_common_fnc_addEventHandler;

defuse and explodeOnDefuse
@bux bux added kind/enhancement Release Notes: **IMPROVED:** kind/feature Release Notes: **ADDED:** labels Jan 1, 2016
@bux bux added this to the 3.4.2 milestone Jan 1, 2016
@nicolasbadano
Copy link
Contributor

👍 Looks good

@jonpas
Copy link
Member

jonpas commented Jan 1, 2016

👍

@Cuel
Copy link
Contributor

Cuel commented Jan 4, 2016

shouldn't it be placed/defused/explodedOnDefuse ? 😄

@CorruptedHeart
Copy link
Member

It appears some listenable events are listed here this would probably need to be added there.

But there is both past tense and current tense words used in the events listed there so probably either naming scheme is fine but we might want to be consistent at some point.

@jonpas
Copy link
Member

jonpas commented Jan 5, 2016

@CorruptedHeart This list is generated with a script, events should be added to module-specific framework page, they get pulled into one big list on events framework page then.

@CorruptedHeart
Copy link
Member

Alright, I guess we still need to have a discussion about whether to keep them named like they are now or past tense.

@jonpas
Copy link
Member

jonpas commented Jan 6, 2016

I was always thinking that those that are there to signal that something happened (listenable) are named in past tense, and those that can be used to perform a certain action (callable) are like those in this PR.

@thojkooi thojkooi modified the milestones: 3.4.2, 3.5.0 Jan 6, 2016
@Cuel
Copy link
Contributor

Cuel commented Jan 9, 2016

Agree with @jonpas.
Furthermore why not throw a "ace_explosives_detonated" in? I was just looking for that

thojkooi added a commit that referenced this pull request Feb 4, 2016
@thojkooi thojkooi merged commit e0946c3 into master Feb 4, 2016
@thojkooi thojkooi deleted the explEvents branch February 4, 2016 18:52
@Bummeri
Copy link

Bummeri commented Feb 6, 2016

Is it possible to provide that event that @Cuel was speaking about?

@nicolasbadano
Copy link
Contributor

Is it possible to provide that event that @Cuel was speaking about?

It would be tricky, cause it would work with explosives detonated by remote control, but not for those that explode because of proximity, being damaged, etc. IMO it's not worth it with this limitations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Release Notes: **IMPROVED:** kind/feature Release Notes: **ADDED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants