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 ModifyMaxSquadSize DLC hook #954

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

robojumper
Copy link
Member

@robojumper robojumper commented Jan 13, 2021

I personally don't plan to publish any mod depending on this, but I think this could come in helpful for a mod that gives more control over squad size overall (base size, squad size upgrades, special missions).

396b40b...e33a82f

@robojumper robojumper added the ready-to-review A pull request is ready to be reviewed label Jan 13, 2021
@Xymanek
Copy link
Member

Xymanek commented Feb 11, 2021

Why a DLCInfo hook and not an event?

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.

I don't have strong objections to anything in this PR, and a hook like that is no doubt gonna come in handy on many occasions, although same as Xymanek, I wonder if perhaps making this a delegate method (like the one Xym and I made for #889) would have been better.

Although on second thought, delegates have more cumbersome setup, while mission squad size is not particularly hot code, so it's probably okay to have it as a DLC hook.

///
/// The final max squad size will be calculated with the expression `Min(Max(1, MaxBaseSize + SquadSizeUpgradeMod + SituationMod), SitRepMax)`.
///
/// This hook is best combined with `GetValidFloorSpawnLocations` or `SPAWN_EXTRA_TILE` (TODO: Docs for these) in order to support
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 not sure we'd want to ship a "TODO", but I assume this is here just because it's a Draft PR? It is marked as "ready for review", so I'm getting some mixed signals here.

///
/// if (MissionSite != none && MissionSite.GeneratedMission.Mission.MissionName == 'AssaultFortressLeadup')
/// {
/// // Leviathan gets four more soldiers because reasons
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I had no trouble following these docs and I find the provided explanations perfectly sufficient, but perhaps they should be a bit lighter on the joke-ish wordings like "cargo cult" and "because reasons".

@Iridar Iridar added this to the 1.27.0 milestone Aug 10, 2023
@Iridar Iridar modified the milestones: 1.27.0, 1.28.0 Oct 29, 2023
@Iridar Iridar modified the milestones: 1.28.0, 1.29.0 May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-review A pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants