Skip to content

Conversation

@Fryone
Copy link
Contributor

@Fryone Fryone commented Jun 17, 2023

Requires units to have:
'Ammo' > 0;
'IsSimpleDeployer=yes';
'Convert.Deploy='.

Parameters
'OnAmmoDepletion.AutoDeploy=true' - uses Deploy command on unit when Ammo gets 0.
'OnAmmoDepletion.DeployBlock=true' - all Deploy commands on unit will turn into AreaGuard/Guard if Ammo equals 0.

Fryone added 4 commits June 17, 2023 10:53
TechnoType:
OnAmmoDepletion.AutoDeploy
OnAmmoDepletion.DeployBlock
removed unsuccesseful building support
This reverts commit 33c8a68.
@Fryone
Copy link
Contributor Author

Fryone commented Jun 17, 2023

For now 'OnAmmoDepletion.DeployBlock=true' turns Deploy command to AreaGuard/Guard, because I couldn't figure out to save previous command.

Copy link
Member

@MortonPL MortonPL left a comment

Choose a reason for hiding this comment

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

Hello, thank you for your contriburion. Here's a few notes on how to improve it (more in review comments):

It's a good idea to avoid nesting conditions if possible. Also, there's no need to use return at the end of a function body with no return type.

Please add an entry to CREDITS.md, docs/Whats-New.md and documentation.

Copy link
Contributor

@Starkku Starkku left a comment

Choose a reason for hiding this comment

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

Given that this is clearly aimed towards using Ares' type conversion, I do feel like messing with the deploying functionality instead of directly using type conversion using the recently implemented functionality is bit of a questionable choice, however lifting the restriction on IsSimpleDeployer would possibly allow some niche usage with vehicle to building deployers, and doing it this way allows using existing deploying animations etc. without requiring a new bespoke implementation so perhaps it is not all bad. Besides the IsSimpleDeployer thing there are a couple of smaller things and one bigger design issue I'd like to see addressed.

Fryone and others added 10 commits June 17, 2023 15:57
Co-authored-by: MortonPL <Bartm12@wp.pl>
Co-authored-by: MortonPL <Bartm12@wp.pl>
Co-authored-by: MortonPL <Bartm12@wp.pl>
Co-authored-by: MortonPL <Bartm12@wp.pl>
Co-authored-by: MortonPL <Bartm12@wp.pl>
Co-authored-by: MortonPL <Bartm12@wp.pl>
Co-authored-by: MortonPL <Bartm12@wp.pl>
Co-authored-by: MortonPL <Bartm12@wp.pl>
Co-authored-by: MortonPL <Bartm12@wp.pl>
Suggested refactoring applied
@Metadorius
Copy link
Member

Heads up, if you go to Files changed tab, you can apply review suggestions in batch to not create excessive commits.

Fryone and others added 9 commits June 17, 2023 18:27
Updated feature description
Credits and Changelog update
Co-authored-by: MortonPL <Bartm12@wp.pl>
OnAmmoDepletion_DeployBlock replaced with OnAmmoDepletion_DeployUnlockAmount
issue fixed, wrong condition
removed unnecessary functions
Implemented Hooks.Unload extension
@Fryone Fryone closed this Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants