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

Lay mines in nearby cells first. #16877

Merged
merged 3 commits into from Aug 20, 2019

Conversation

@pchote
Copy link
Member

commented Aug 5, 2019

This adds the minelayer improvements I proposed in #16549.

Fixes #15727.

@pchote pchote added this to the Next Release milestone Aug 5, 2019

@pchote pchote requested a review from tovl Aug 5, 2019

@tovl
Copy link
Contributor

left a comment

  • I noticed that the target line will often jump around awkwardly between different cells when the minelayer approaches the minefield. This looks a bit unpolished so it's probably better to choose a cell once and stick with it even if it is not exactly the closest anymore.

  • The original issue asks for the order of the mines to be explicitly controlled by the player using the drag direction. This would actually be pretty easy to do by just going through the minefield list in the original order.

  • When you queue multiple minefield orders to a group of minelayers there are a lot of redundant target lines as each one chooses a slightly different start tile for each field.

  • This may be a bit of scope creep, but currently when you order multiple minelayers to mine in a field, the entire overlay will stay visible until the entire field is finished. It would be nice if minelayers could signal to each-other that a tile has already been mined and remove it from their minefield list.

@pchote pchote force-pushed the pchote:minelayer-target-cells branch from 99b6173 to 40b2768 Aug 16, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

Fixed.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

This is already looking very good!

A couple things I noticed:

  • When multiple fields are queued using shift, here is still a problem with the overlay being visible in places that have already been mined. This is especially noticeable when queueing overlapping minefields. It also happens when you have ordered multiple minelayers and some of them are already on the next field while others are still reloading.
  • Minelayers will always go to rearm when empty even when there is nothing left to mine anymore and only end their current activity when done rearming. This also exacerbates the first point.
  • When multiple minelayers are on a field they tend to get in each others way a lot. They all rush to the same cell and block each other, then when one lays down a mine they all rush to the next cell and block each other there. This mean that in some cases some of the minelayers will not get a chance to lay down any mines until the others go back to reload. It would be nice if minelayers would claim a cell ahead of time so that the others would skip that cell.
  • This is more of an open question: I'm wondering what the behaviour should be when queueing overlapping minefields? Should they be treated as a single minefield? It might be a good idea to at least skip the cells that have already been ordered as part of a previously queued minefield.

Most of these issues would probably be difficult to address without some extra plumbing and what we have here is already a major improvement so I don't think we should block this if we can't solve all of them right now. Especially the issues relating to multiple minelayers on the same field might be more easily solved by building on the distributed movement logic and multiple minelayers are currently a rare thing anyway.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

The first point could probably be fixed by adding a Tick method to MineLayer that does something like

foreach (var field in self.CurrentActivity.ActivitiesImplementing<LayMines>())
	field.CleanPlacedMines();

This is a bit ugly, but a lot simpler than adding a minelayer messaging system.

The third point is a natural consequence of making the minelayers go from start to end. My fix for this was to make them mine the closest cell instead!

My opinion wrt the fourth point is already well known: IMO the best solution is to remove the custom targeting system completely (this also fixes points 1-3).

@pchote

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

Updated to fix the first two points.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

The third point is a natural consequence of making the minelayers go from start to end.

Not when you allow them to claim cells (which harvesters already do btw, But a solution based on the distributed movement logic may be better.)

My opinion wrt the fourth point is already well known: IMO the best solution is to remove the custom targeting system completely (this also fixes points 1-3).

I hope that was a joke. This fixes those issues in the same way that cutting off your leg fixes an itchy foot. I'd rather have this great bit of UI with some warts than nothing at all.

@tovl
Copy link
Contributor

left a comment

Can confirm the fixes for the first two points. This implicitly addresses part of the fourth point as well because the overlapping cells are effectively skipped.

This is looking great now!

@pchote pchote force-pushed the pchote:minelayer-target-cells branch from 46db630 to a5b81c1 Aug 19, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Updated and fixed a crash when queueing single-mine deploy orders.

@pchote pchote force-pushed the pchote:minelayer-target-cells branch from a5b81c1 to 4cff2bf Aug 19, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Is your 👍 still valid, @tovl?

@teinarss teinarss merged commit 7e4da8e into OpenRA:bleed Aug 20, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote deleted the pchote:minelayer-target-cells branch Aug 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.