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

Minelaying improvements #20959

Merged
merged 2 commits into from
Jul 17, 2023
Merged

Conversation

michaeldgg2
Copy link
Contributor

Second PR with Minelayer improvements:

  1. option to specify time it takes to lay a mine
  2. delay after mine has been laid is now customizable (previously hardcoded to 20 ticks)
  3. Minelayer exposes static method for creating BeginMinefield order

No. 3 is useful for mods that would want to change the way this feature is activated.

OpenRA.Mods.Common/Activities/LayMines.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/LayMines.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/LayMines.cs Outdated Show resolved Hide resolved
@michaeldgg2 michaeldgg2 force-pushed the Minelaying-Improvements branch 2 times, most recently from 39505b9 to 577d821 Compare July 10, 2023 15:32
@PunkPun
Copy link
Member

PunkPun commented Jul 10, 2023

No changes were pushed

@michaeldgg2
Copy link
Contributor Author

michaeldgg2 commented Jul 10, 2023

I did mess up the first push (with the fixes), but the second push went through (though I forgot about the Wait in FinishLayingMine). Now it should be complete (including the fix for empty ammo pool).

It seems that the GitHub's Compare feature between two commits (where one or both could be already "force-pushed" out of the repo) isn't working properly, because Commits and Files tabs show the changes correctly.

@PunkPun
Copy link
Member

PunkPun commented Jul 10, 2023

you just removed MineLaidDelay, and didn't really push any of the changes

OpenRA.Mods.Common/Traits/Minelayer.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/LayMines.cs Outdated Show resolved Hide resolved
@michaeldgg2
Copy link
Contributor Author

michaeldgg2 commented Jul 14, 2023

In order to make cancellation of mine laying work correctly, I had to add another method to INotifyMineLaying to notify other traits that the laying process was canceled.

Also LayMine still has to return bool and false, if the ammo pool doesn't have enough ammo. Because theoretically other trait could take ammo from that ammo pool during PreLayDelay (and thus LayMines activity would lay a mine without actually adhering to ammo pool restriction).

This is how the minelayer unit looks in OpenE2140 with current changes in the PR. I made a custom and very simple WithDeployMineAnimation trait that takes advantage of INotifyMineLaying. Since I'm planning on making another PR with improvements to minelaying, so I could throw that in, if you'd like.

opene2140_miner_minelaying_test2

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

Final nits, otherwise LGTM

OpenRA.Mods.Common/Activities/LayMines.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Minelayer.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/TraitsInterfaces.cs Outdated Show resolved Hide resolved
@michaeldgg2 michaeldgg2 force-pushed the Minelaying-Improvements branch 3 times, most recently from 24083d2 to a14e056 Compare July 14, 2023 17:11
@Mailaender Mailaender merged commit 8aa548f into OpenRA:bleed Jul 17, 2023
3 checks passed
@Mailaender
Copy link
Member

Changelog

@michaeldgg2 michaeldgg2 deleted the Minelaying-Improvements branch September 20, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants