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

Allow queued structure rallypoints. #17021

Merged
merged 2 commits into from Dec 13, 2019
Merged

Conversation

tovl
Copy link
Contributor

@tovl tovl commented Aug 31, 2019

Fixes #15371.
Fixes #15995.

This adds a way to make units exiting a structure use a specific route to the rallypoint. This can be done by holding shift when ordering the rally point in the same manner as queueing unit orders.

chrisforbes
chrisforbes previously approved these changes Aug 31, 2019
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

This is a cool feature, but the implementation may need some more polishing/prereqs before we can merge.

See inline comments for some (hopefully) straightforward requests.

My bigger concern is about the near-enough handling - units will stall if a building or non-nudgeable unit blocks one of the rally point nodes!

OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Effects/RallyPointIndicator.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Effects/RallyPointIndicator.cs Outdated Show resolved Hide resolved
@@ -48,8 +48,7 @@ public class RallyPoint : IIssueOrder, IResolveOrder, ISync, INotifyOwnerChanged
{
const string OrderID = "SetRallyPoint";

[Sync]
public CPos Location;
public List<CPos> Locations;
Copy link
Member

Choose a reason for hiding this comment

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

Can we please rename this to Path? IMO Locations is ambiguous if not misleading.
This would also be a good opportunity to fix outside methods manipulating this directly by adding a void SetPath(IEnumerable<CPos> path) method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the name.

@@ -58,7 +57,7 @@ public class RallyPoint : IIssueOrder, IResolveOrder, ISync, INotifyOwnerChanged

public void ResetLocation(Actor self)
{
Location = self.Location + Info.Offset;
Locations = new List<CPos> { self.Location + Info.Offset };
Copy link
Member

Choose a reason for hiding this comment

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

This could then become ResetPath and use .Clear instead of allocating a new list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had this use .Clear in an earlier iteration but that breaks things because this method is also called to set the default rallypoint when the building is build. Since Locations is still null at that point, it crashes.

@tovl
Copy link
Contributor Author

tovl commented Sep 6, 2019

My bigger concern is about the near-enough handling - units will stall if a building or non-nudgeable unit blocks one of the rally point nodes!

This is now fixed as well.

@tovl
Copy link
Contributor Author

tovl commented Oct 11, 2019

Rebased.

@abcdefg30 abcdefg30 merged commit 203fff0 into OpenRA:bleed Dec 13, 2019
@abcdefg30
Copy link
Member

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants