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

CSW - Improved weapon attachments handling #9904

Merged
merged 10 commits into from
Jun 22, 2024
Merged

Conversation

johnb432
Copy link
Contributor

@johnb432 johnb432 commented Apr 2, 2024

When merged this pull request will:

  • Title.
  • Weapon accessories are returned to the player immediately.
  • Magazines are checked if the are compatible with the CSW. If they are, they are added to the CSW, if they are not, the magazines are placed on the ground.

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

@TheCandianVendingMachine
Copy link
Contributor

I am not a fan of this. The progress bar continuing when you dont have the weapon is kinda bad UX, it should just fail when the item is removed. Likewise, the initial decision was to “simulate” the weapon being setup. This seems like change just for changes sake

@johnb432
Copy link
Contributor Author

johnb432 commented Apr 2, 2024

I am not a fan of this. The progress bar continuing when you dont have the weapon is kinda bad UX, it should just fail when the item is removed.

Yea, I can add that.

This seems like change just for changes sake

It's not intended to be. I find it unnecessary to remove a weapon, only to have to add it back again if you cancel. It means we also have to handle all attachments, which we currently don't do.

@TheCandianVendingMachine
Copy link
Contributor

Wouldn’t the attachments would be killed when the weapon is removed and placed as a vehicle anyway? Logic needs to exist anyway for that edge case.

@johnb432
Copy link
Contributor Author

johnb432 commented Apr 2, 2024

Wouldn’t the attachments would be killed when the weapon is removed and placed as a vehicle anyway? Logic needs to exist anyway for that edge case.

Currently yes, I still need to add handling of attachments.


Imo it's just easier to remove upon success, but I can revert the behaviour.

@TheCandianVendingMachine
Copy link
Contributor

I’d like another person to comment on it, I am not so against this I will just block it but I just see it as unnecessary

@johnb432 johnb432 changed the title CSW - Remove CSW parts only if successful CSW - Improved weapon attachments handling Apr 2, 2024
@johnb432 johnb432 added the kind/enhancement Release Notes: **IMPROVED:** label Apr 2, 2024
@johnb432
Copy link
Contributor Author

johnb432 commented Apr 2, 2024

@TheCandianVendingMachine I've removed the part where it only removes the weapon upon successful completion.
Essentially, this has become a different PR focussing on the improved handling of weapon attachments.

@johnb432 johnb432 added this to the 3.18.0 milestone May 9, 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.

@TheCandianVendingMachine any further thoughts? LGTM

@LinkIsGrim LinkIsGrim merged commit 1f5044a into master Jun 22, 2024
5 checks passed
@LinkIsGrim LinkIsGrim deleted the csw-failure-code-tweak branch June 22, 2024 17:03
blake8090 pushed a commit to blake8090/ACE3 that referenced this pull request Aug 18, 2024
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