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

Rclick melee delegate 1138 #1152

Merged

Conversation

FearTheBunnies
Copy link
Contributor

@FearTheBunnies FearTheBunnies commented Jul 29, 2022

Uses a delegate based approach to expanding the functionality versus actually forcing a revised sorting algorithm. Will allow other mods to hook into the right-click melee determination logic and provide prioritization dynamically for which melee ability should be used. Closes #1138

@FearTheBunnies FearTheBunnies force-pushed the rclick-melee-delegate-1138 branch 2 times, most recently from e0cf4fe to 35cc73c Compare July 29, 2022 18:24
@Iridar Iridar added this to the 1.25.0 milestone May 11, 2023
Copy link
Contributor

@Iridar Iridar left a comment

Choose a reason for hiding this comment

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

Either way this would be a useful functionality to have, so I hope you can address the requested changes so this can be added to CHL.

@Iridar Iridar added the waiting-on-author A pull request is waiting on changes from the author label May 11, 2023
@FearTheBunnies
Copy link
Contributor Author

Simplified the process significantly by reducing a lot of the clunkiness and now passes UnitState to the delegate allowing for the end consumer to interact with it and the ruleset cache as much as the desire. Should be much cleaner now.

@Iridar Iridar self-requested a review May 21, 2023 07:32
Copy link
Contributor

@Iridar Iridar left a comment

Choose a reason for hiding this comment

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

Simplified the process significantly by reducing a lot of the clunkiness and now passes UnitState to the delegate allowing for the end consumer to interact with it and the ruleset cache as much as the desire. Should be much cleaner now.

It seems alright superficially, I haven't gone through it letter by letter or ran any tests yet. I still think the system would benefit from being able to override which ability to use depending on the target.

This would just mean copying the code from the X2AbilityTrigger_EndOfMove to:

1) XComTacticalInput - here you would have to retrieve the target State Object from History using PathingPawn.LastTargetObject.GetReference().ObjectID.

2) XComPathingPawn - you get XComGameState_BaseObject TargetObject right in the function arguments.

The X2AbilityTrigger_EndOfMove should still call the override though, just without the target state. You can make the target state object an optional argument for the delegate, or just explicitly pass none there.

This is just so that the override can remain at least somewhat functional in case mod-added code also runs X2AbilityTrigger_EndOfMove::GetAvailableEndOfMoveAbilityForUnit().

@Iridar Iridar modified the milestones: 1.25.0, 1.26.0 May 21, 2023
@FearTheBunnies
Copy link
Contributor Author

Updated to include the target object now. Still tried to maintain a minimal footprint in the Highlander itself.

@Iridar Iridar added ready-to-review A pull request is ready to be reviewed and removed waiting-on-author A pull request is waiting on changes from the author labels May 29, 2023
{
// Begin Issue #1138
// Forward the work to the new endpoint to maintain BC
return GetAvailableEndOfMoveAbilityForUnitWithTarget(UnitState, none);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm uncomfortable with how this has been handled. GetAvailableEndOfMoveAbilityForUnitWithTarget() doesn't exist in base game code, but it's not immediately obvious.

I also have BC concerns.

Here's what I request:

  1. Get rid of GetAvailableEndOfMoveAbilityForUnitWithTarget(), keep the original code inside GetAvailableEndOfMoveAbilityForUnit().
  2. Add the CHHelpersObj.TriggerPrioritizeRightClickMelee() above the original code, just give it a none target object.
  3. Also call CHHelpersObj.TriggerPrioritizeRightClickMelee() from the two places where you called GetAvailableEndOfMoveAbilityForUnitWithTarget() from, but this time with the target object.

Let's do it this way, because then we do not have to maintain an additional function and worry about having to protect internal API or maintain BC.

It's not as pretty and results in more lines of code, but I believe it is the better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tweaked the implementation a little by finally realizing there are actually two things at play here. First thing is allowing a mod to fix the "bug" of the right-click priority choosing abilities that are non-intuitive. The second thing was enhancing the selection ability to allow for the target to be provided increasing the flexibility of the choice.

@FearTheBunnies FearTheBunnies force-pushed the rclick-melee-delegate-1138 branch 2 times, most recently from 5bf9389 to 4399dc1 Compare August 14, 2023 13:57
@Iridar
Copy link
Contributor

Iridar commented Aug 15, 2023

Refactored the PR. Footnotes:

  • Moved the code from the original GetAvailableEndOfMoveAbilityForUnit() into a new function GetAvailableEndOfMoveAbilityForUnit_Original(). Replaced for() cycle with foreach() for the sake of optimization.
  • GetAvailableEndOfMoveAbilityForUnit_CH() triggers the override, then defers to GetAvailableEndOfMoveAbilityForUnit_Original() if no override is found.
  • GetAvailableEndOfMoveAbilityForUnit() does the same, but does not pass the target object to the override.
  • Marked TargetObject as an optional to make it clear it's not guaranteed.
  • Slapped private and final where appropriate.
  • Updated docs and comments.

/// You can optionally specify callback Priority. The default Priority is 50.
///```unrealscript
///CHHelpersObj.AddPrioritizeRightClickMeleeCallback(PrioritizeRightClickMelee, 45);
///```
Copy link
Contributor

Choose a reason for hiding this comment

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

Discord formatting?

Copy link
Contributor

Choose a reason for hiding this comment

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

HL-Docs can render UC code in a similar fashion, example: this -> renders like this

@Iridar
Copy link
Contributor

Iridar commented Aug 19, 2023

Moved the override trigger after the default logic so that the override could allow for a more informed decision without directly replicating base game logic.

@Iridar
Copy link
Contributor

Iridar commented Aug 19, 2023

This is tested and ready to go, just waiting for @FearTheBunnies to squash the commit with my adjustments and it'll be ready to merge.

@Iridar Iridar added ready-for-merge the pull request was reviewed and is ready to be merged. and removed ready-to-review A pull request is ready to be reviewed labels Aug 19, 2023
@Iridar Iridar merged commit 43c368b into X2CommunityCore:master Aug 19, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ready-for-merge the pull request was reviewed and is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Right Click Melee should smartly prioritize which melee skill to choose
3 participants