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

Interaction - Add "pass grenade" interaction #10463

Merged
merged 13 commits into from
Nov 16, 2024
Merged

Conversation

Timi007
Copy link
Contributor

@Timi007 Timi007 commented Oct 30, 2024

When merged this pull request will:

  • Add "pass grenade" interaction that passes the currently selected grenade

screenshot2

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

@Timi007 Timi007 marked this pull request as ready for review November 1, 2024 16:25
Copy link
Contributor

@johnb432 johnb432 left a comment

Choose a reason for hiding this comment

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

When selecting the category, it will either use the last grenade that was passed or the next available one in the inventory.

In my personal opinion, that particular aspect of the PR is not intuitive (magazine passing doesn't have that, so why should grenade passing have it? also no documentation), not helpful (it only saves you the final mouse movement, so it save you pretty little; it's unlikely you'd be passing numerous grenades this way, you'd probably put it in a backpack or the ground) and therefore not useful.

I'd like to hear more from other members of the ACE team though, before you undertake any changes related to that.

addons/interaction/functions/fnc_getThrowableInfo.sqf Outdated Show resolved Hide resolved
@Timi007
Copy link
Contributor Author

Timi007 commented Nov 3, 2024

In my personal opinion, that particular aspect of the PR is not intuitive (magazine passing doesn't have that, so why should grenade passing have it? also no documentation), not helpful (it only saves you the final mouse movement, so it save you pretty little; it's unlikely you'd be passing numerous grenades this way, you'd probably put it in a backpack or the ground) and therefore not useful.

Do I understand you correctly that you don't want to have this split into lethal and non-lethal grenades? Should all grenades be listed under "Pass grenade" directly?

Edit:
When passing a magazine, you don't select a specific type. My thought process was that I wanted something similar for grenades but also let the user decide specific types if needed. Sometimes you just want any available lethal grenade passed to you.

As for saving mouse movement and no documentation, the same can be said for the ACE tagging system. You can spray the last used tag by just selecting the top tagging action. This is, as far as I know, also not documented in the wiki.

If needed, I can document this feature in the wiki.

@Drofseh
Copy link
Contributor

Drofseh commented Nov 3, 2024

When selecting the category, it will either use the last grenade that was passed or the next available one in the inventory.

In my personal opinion, that particular aspect of the PR is not intuitive (magazine passing doesn't have that, so why should grenade passing have it? also no documentation), not helpful (it only saves you the final mouse movement, so it save you pretty little; it's unlikely you'd be passing numerous grenades this way, you'd probably put it in a backpack or the ground) and therefore not useful.

I'd like to hear more from other members of the ACE team though, before you undertake any changes related to that.

I kinda feel like it's too busy with a node for every grenade type carried and that a single pass grenade node that just passes your currently selected grenade would be fine.

@johnb432
Copy link
Contributor

johnb432 commented Nov 3, 2024

When passing a magazine, you don't select a specific type

Yea, you're right, forgot about that despite having the code under my eyes.

If needed, I can document this feature in the wiki.

Regardless of what will be done in the end, some documentation should be provided.

I kinda feel like it's too busy with a node for every grenade type carried and that a single pass grenade node that just passes your currently selected grenade would be fine.

I agree with it being too busy, especially if you use the radial menu, but I do see the appeal of being able to pass any grenade.

@Timi007
Copy link
Contributor Author

Timi007 commented Nov 6, 2024

So do all agree to have only one "pass current grenade" action node and to pass the currently selected grenade?

@Timi007
Copy link
Contributor Author

Timi007 commented Nov 6, 2024

Ready for review

addons/interaction/functions/fnc_passThrowable.sqf Outdated Show resolved Hide resolved
addons/interaction/functions/fnc_passThrowable.sqf Outdated Show resolved Hide resolved
addons/interaction/functions/fnc_passThrowable.sqf Outdated Show resolved Hide resolved
addons/interaction/functions/fnc_canPassThrowable.sqf Outdated Show resolved Hide resolved
addons/interaction/functions/fnc_canPassThrowable.sqf Outdated Show resolved Hide resolved
addons/interaction/functions/fnc_passThrowable.sqf Outdated Show resolved Hide resolved
addons/interaction/functions/fnc_passThrowable.sqf Outdated Show resolved Hide resolved
addons/interaction/functions/fnc_canPassThrowable.sqf Outdated Show resolved Hide resolved
@johnb432 johnb432 added the kind/feature Release Notes: **ADDED:** label Nov 11, 2024
@johnb432 johnb432 added this to the 3.18.2 milestone Nov 11, 2024
@johnb432 johnb432 merged commit 0d1089d into acemod:master Nov 16, 2024
3 checks passed
@Timi007 Timi007 deleted the pass_grenade branch November 16, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Release Notes: **ADDED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants