Skip to content

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