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

Add option to send sound alerts data to allies #1169

Merged
merged 9 commits into from
Aug 17, 2023

Conversation

RedDoby
Copy link
Contributor

@RedDoby RedDoby commented Apr 27, 2023

Closes #620
Allow option to override original function in OnABilityActivated to pass sound alerts from allies to other allies.

Allow option to override original function in OnABilityActivated to pass sound alerts from allies to other allies.
@Iridar Iridar added the ready-to-review A pull request is ready to be reviewed label May 11, 2023
@Iridar Iridar added this to the 1.25.0 milestone May 11, 2023

/// HL-Docs: ref:ConsiderAlliesforSoundAlerts
// start issue #620
static function bool ConsiderAlliesforSoundAlerts()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there are a particular reason you created a function to check the value of a config variable?

You could just do class'CHHelpers'.default.bConsiderAlliesforSoundAlerts wherever you need to check this.

But even if using a function, it can be simplified to:

static function bool ConsiderAlliesforSoundAlerts()
{
    return default.bConsiderAlliesforSoundAlerts;
}

Also, your function has no return value for when default.bConsiderAlliesforSoundAlerts is false. It will still return false as that is the default value for bool values, but it's not a proper way to do things.

@Iridar Iridar added waiting-on-author A pull request is waiting on changes from the author and removed ready-to-review A pull request is ready to be reviewed labels May 11, 2023
@Iridar Iridar modified the milestones: 1.25.0, 1.26.0 May 21, 2023
@Iridar Iridar modified the milestones: 1.26.0, 1.27.0 Aug 13, 2023
Using a bool variable now rather than a static function
Sorry I someone didn't copy the latest XComGame file and removed these comments in my latest commit.
@@ -32,13 +32,13 @@ bEnableVersionDisplay=true
;;; HL-Docs: ref:ArmorEquipRollDLCPartChance
;+CosmeticDLCNamesUnaffectedByRoll=MyDLCName

;;; HL-Docs: ref:DisableExtraLOSCheckForSmoke
Copy link
Contributor

@Iridar Iridar Aug 16, 2023

Choose a reason for hiding this comment

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

What are these three lines concerning issue #669 doing here?


if( VSizeSq(vLoc - vCenter) < RadiiSumSquared )
{
OutEnemies.AddItem(kUnit.GetReference());
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're collecting units, not enemies, would make sense for the array to be called OutUnits rather than OutEnemies.

@Iridar
Copy link
Contributor

Iridar commented Aug 16, 2023

Please remove the unrelated changes to issue #669 HL-Docs from this PR. It's actually breaking the docs, btw. Other than that, should be good to go.

RedDoby and others added 4 commits August 16, 2023 20:56
remove lines not pertaining to this issue
For sake of making sense change original variable name since this was copied from another function
@Iridar Iridar modified the milestones: 1.27.0, 1.26.0 Aug 17, 2023
@Iridar Iridar added enhancement ready-for-merge the pull request was reviewed and is ready to be merged. and removed waiting-on-author A pull request is waiting on changes from the author labels Aug 17, 2023
@Iridar Iridar merged commit dc39a19 into X2CommunityCore:master Aug 17, 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.

Sound is heard by allied units too, not just enemies
3 participants