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

Fortify - Add onDeployStop event for cancelling before confirming #9854

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

DartRuffian
Copy link
Contributor

@DartRuffian DartRuffian commented Mar 13, 2024

When merged this pull request will:

  • Add a ace_fortify_onDeployStop local event for when deploying is cancelled before confirming
    • I.e. is called when right clicking after the menu appears
    • Is also called when ace_fortify_deployCanceled is

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}.

@DartRuffian
Copy link
Contributor Author

Title could probably be changed, feels a bit wordy but couldn't think of a good way to phrase it.

@johnb432
Copy link
Contributor

Why not use ace_fortify_deployCanceled, instead of creating a new event?

@DartRuffian
Copy link
Contributor Author

Why not use ace_fortify_deployCanceled, instead of creating a new event?

Different parameters, I made it match the acex_fortify_onDeployStart event, subbing out the object itself with the type of object.

@johnb432
Copy link
Contributor

Different parameters, I made it match the acex_fortify_onDeployStart event, subbing out the object itself with the type of object.

deployCanceled contains the same parameters (and more) as your current implementation of ace_fortify_onDeployStop, so imo not a good enough reason to add.

I'm guessing you want a listenable event when the player stops deploying?

@DartRuffian
Copy link
Contributor Author

Different parameters, I made it match the acex_fortify_onDeployStart event, subbing out the object itself with the type of object.

deployCanceled contains the same parameters (and more) as your current implementation of ace_fortify_onDeployStop, so imo not a good enough reason to add.

I'm guessing you want a listenable event when the player stops deploying?

I could give it the same params, just figured it would make more sense to have a matching event instead of the deployCanceled. Could just update it to use the other event as well.

@DartRuffian
Copy link
Contributor Author

Updated it to use deployCanceled instead of a new event

@DartRuffian DartRuffian changed the title Fortify - Add onDeployStop event for cancelling before confirming Fortify - Make deployCanceled emit when canceling before confirming Mar 21, 2024
@johnb432
Copy link
Contributor

Updated it to use deployCanceled instead of a new event

You misunderstood me and my intentions.

deployCanceled does not work in this context, because we refund the cost of placing something down if you cancel it (see XEH_postInit.sqf).

deployCanceled contains the same parameters (and more) as your current implementation of ace_fortify_onDeployStop, so imo not a good enough reason to add.

There might be a good reason to add a new event, however the one you gave isn't good enough.

So I ask again: I'm guessing you want a listenable event when the player stops deploying?
If yes, then, given how the component is structured, we should add a listenable event.

@DartRuffian
Copy link
Contributor Author

There might be a good reason to add a new event, however the one you gave isn't good enough.

So I ask again: I'm guessing you want a listenable event when the player stops deploying? If yes, then, given how the component is structured, we should add a listenable event.

Yes.
Currently, onDeployCanceled is only emitted after the player initially starts building an object. If you have code that runs on acex_fortify_onDeployStart and is supposed to stop running if a player stops building an object, there is no (easy) way to detect if a player stops building before starting to construct an object.

@johnb432
Copy link
Contributor

@DartRuffian I've added your initial suggestion back partially. However, this event won't be thrown when you cancel.
If you want to know when it's cancelled, use the cancel event. I see no reason why to mix stop and cancel in the stop event. It would make it worse imo: You couldn't tell the difference between when something has been cancelled or stopped, when there might be cases where you'd want to.

@DartRuffian
Copy link
Contributor Author

@DartRuffian I've added your initial suggestion back partially. However, this event won't be thrown when you cancel. If you want to know when it's cancelled, use the cancel event. I see no reason why to mix stop and cancel in the stop event. It would make it worse imo: You couldn't tell the difference between when something has been cancelled or stopped, when there might be cases where you'd want to.

I think I had both events originally there as a placeholder, can't remember an actual good reason for it being there

@DartRuffian DartRuffian changed the title Fortify - Make deployCanceled emit when canceling before confirming Fortify - Add onDeployStop event for cancelling before confirming Mar 24, 2024
Copy link
Contributor

@LinkIsGrim LinkIsGrim left a comment

Choose a reason for hiding this comment

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

LGTM

@johnb432 johnb432 merged commit 31217ae into acemod:master Mar 25, 2024
5 checks passed
@johnb432 johnb432 added this to the 3.17.0 milestone Mar 25, 2024
@johnb432 johnb432 added the kind/enhancement Release Notes: **IMPROVED:** label Mar 25, 2024
@DartRuffian DartRuffian deleted the fortify-cancelEvent branch September 23, 2024 00:55
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.

3 participants