Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#1044 closed enhancement (fixed)

[PATCH] Autogarrison newly trained units in the production building

Reported by: Jonathan Waller Owned by: mimo
Priority: Should Have Milestone: Alpha 15
Component: UI & Simulation Keywords: rally point patch
Cc: Patch:

Description

The player should be able to set the rally point of a building on itself so that trained units are automatically garrisoned. Units should not appear outside of the building at all and ungarrisoning should result in the units being ejected. Will also require a fix for the rally point line drawing code so that the rally point line is removed when the rally point position is set.

Attachments (3)

autogarrison.diff (9.6 KB ) - added by mimo 11 years ago.
autogarrison-2.diff (8.6 KB ) - added by mimo 11 years ago.
adapted to latest svn
autogarrison-3.diff (8.6 KB ) - added by mimo 11 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 by Jonathan Waller, 12 years ago

Type: defectenhancement

comment:2 by mimo, 11 years ago

Keywords: patch added
Summary: Autogarrison newly trained units in the production building[PATCH] Autogarrison newly trained units in the production building

Here is a first version of a patch to allow autogarrison. But if setting the rally-point on a building generates autogarrison, this prevents us from unsetting the rally-point (which is now done by setting the rally-point on the building). So with this patch, we can only modify a rally-point, not unset it.

From discussions on IRC, there are several ways to deal with that :

1 - adding a building COMMAND to unset the rally-point

2 - adding a hokey to unset the rally-point

3 - adding a hotkey to set the rally-point on the building itself, keeping as now the unset of the rally-point by defaut when setting the rally-point on the building itself

any preferences ? my choice would be 3 with a Ctrl key.

comment:3 by leper, 11 years ago

Option 3 with Ctrl as the hotkey sounds nice IMO. It is also consistent with our use of Ctrl for garrisoning elsewhere.

by mimo, 11 years ago

Attachment: autogarrison.diff added

comment:4 by mimo, 11 years ago

Keywords: review added

This now now implemented in the new version.

comment:5 by historic_bruno, 11 years ago

Milestone: BacklogAlpha 15

comment:6 by zweigousefen, 11 years ago

I think the buildings should have a "unset the rally point" button or hotkey. This could allow the player to unset at once the rally points of all the buildings he has in his selection.

comment:7 by mimo, 11 years ago

Yes, having an unset-rally-point command in the buildings would be useful. In fact, thinking again about it (and playing a bit with the patch), I believe that the best solution would be to use the option 1 in my first post (i.e. unsetting can only be done via the building button, and no hotkey is necessary for autogarrison): as the building must be selected to set and unset rally points, the button is directly available and the fact that Ctrl is needed to auto-garrison but not for garrison inside an other building nor for any other rally point may be disturbing. Any opinion on that ?

by mimo, 11 years ago

Attachment: autogarrison-2.diff added

adapted to latest svn

comment:8 by sanderd17, 11 years ago

The patch works as advertised. But after play-testing it, I have seen some problems.

  • There's no visual way of knowing if your building will autogarrison or not for most buildings
  • For some buildings, the rallypoint flag is visible in the centre of the building, but the flag is only used for created units, not for units you'll ungarrison. That's quite logical, but the visualisation should differ.
  • You can set the autogarrison even on buildings where it's impossible (like the defense tower), and again, depending on the building, that flag is visible, but pointless.

Of course, these are all connected.

IMO, we need a new way to visualise the rallypoint if it's a garrison command. I'm thinking of making the texture of the building a bit blue-ish (like it's red-ish for places where you can't build foundations). As the flag inside a building is always partially obscured, texturing that building blue-ish will make it more visible in all cases, not only the autogarrison case.

I didn't have an issue with the commands so far though. Maybe we could require the ctrl key in all cases to have a garrison command, and when you don't push the ctrl, it becomes a repair command (if repairable), or else a simple move command.

comment:9 by mimo, 11 years ago

Thanks for the review. I fully agree with your comments about the visual aspect. Your blue-ish buildings could work, I was rather thinking to have bigger flags to make them more visible. But anyway, it could be an independant ticket as design discussions are needed, and this is not linked to the implementation of the feature.

For your third point, autogarrison possible while useless, I've fixed it in the new attached patch.

by mimo, 11 years ago

Attachment: autogarrison-3.diff added

comment:10 by mimo, 11 years ago

Owner: set to mimo
Resolution: fixed
Status: newclosed

In 14144:

Allows autogarrison of newly trained units, fixes #1044

comment:11 by leper, 11 years ago

Keywords: review removed
Note: See TracTickets for help on using tickets.