Opened 11 years ago

Closed 6 years ago

#1919 closed enhancement (fixed)

[PATCH] Packing and unpacking units into structure and vice versa

Reported by: Doménique Owned by:
Priority: Nice to Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by fatherbushido)

The attached patch allows units not just to unpack into another unit entity but also into a structure entity. This enables the use of mobile civ centres and other movable structures for future nomadic tribes.

With the current civilisations of 0 A.D. this patch may not be directly useful, so while I assigned it to Alpha 14 it may be better suited for a later point. I implemented it for a mini civ I made some time ago which you can find here for testing purposes: https://github.com/dvangennip/amazones-0AD

The core changes of this patch involve enabling pack/unpack commands for structure entities, plus the transfer of any garrisoned units and production queues from the old entity to the new. So if the old entity can garrison cavalry and infantry units, but packing up into a mobile cart entity which cannot garrison cavalry, the cavalry will ungarrison while infantry may remain garrisoned. A similar eligibility check applies for any production batches. If the new entity also allows a certain batch to be produced, it will transfer, otherwise it will reset (and funds restored, as per the default behaviour).

This patch would thus also allow transforming one building into another, which might be useful if someone wants to implement building upgrades on phase change (or any other cue). For this reason I feel that the Pack component could be renamed to Transform to better capture its usefulness and perhaps unified with unit Promotion (there is similar code in both components), although that is beyond this patch.

Issues:

  • Pack/Unpack commands may not be shown correctly (or hide other buttons). See comment:10 for more info.
  • Transferring the production queue has a few kinks left to be ironed out.

Attachments (2)

pack.patch (13.6 KB ) - added by Doménique 11 years ago.
Patch for enabling unit and structure packing/unpacking
pack_v2.patch (14.2 KB ) - added by Doménique 11 years ago.
Improved patch for enabling unit and structure packing/unpacking

Download all attachments as: .zip

Change History (24)

by Doménique, 11 years ago

Attachment: pack.patch added

Patch for enabling unit and structure packing/unpacking

comment:1 by wraitii, 11 years ago

I find the idea interesting. It's certainly good for modders. I guess we could switch some siege engines to become structures when stationary, this might have uses for pathfinding optimizations (not sure how that currently is handled). It's quite consistent with how buildings and units are basically the same with a different AI component right now.

Not too sure about the name or usefullness of CanAcceptTemplate, is there really no other way to do that currently?

comment:2 by Doménique, 11 years ago

Thanks for your response! I am not sure if moving unpacked siege units towards structure entities is a good idea. I would not know about the benefits of your idea (e.g., how much would this help the pathfinder), but there are two arguments that go against it.

When I started with this patch, I simply used unit entities with structure actors to mimic the unpacking-into-a-building idea. Problem was a civil centre would rotate to fire its arrows, just like an archer would :S So the opposite, using a structure entity for a siege unit would mean the siege unit no longer turns towards enemies. It may still aim correctly, but it looks odd/incorrect. A second issue is move commands.

Secondly, a siege unit tasked to move will do so (by first packing up and then starting to move). A structure entity does not accept a move command, instead giving a right-click command simply sets the rally point (as you would except for a building). Thus, if a siege unit would be a structure entity as you propose, moving it means an extra manual step for a player. Depending on how the team views the role of siege units in gameplay, this may be a strong or weak downside.

About the CanAcceptTemplate part: I'm not aware of an existing way to check which templates can be constructed by an entity. I believe this is currently not checked because only possible/allowed entities are shown in the GUI for a produced entity. No such commands are given outside the GUI, so checking for invalid production templates is not necessary. The name of the function could be changed, I tried to follow the standard which uses CanDoSomething or HasSomething or IsSomething for boolean responses. It could be AcceptsTemplate or CanBuild(templateName).

comment:3 by michael, 11 years ago

Would this patch allow the player to upgrade individual structures, rather than having to upgrade the entire class of structure? E.g., upgrade a single tower to a "ballista tower" instead of all towers to ballista tower. I could also envision upgrading individual units, or multiple units in a selection, to greater status (a higher or better template), like promoting a Hypaspist to an "Officer" or upgrading a single warship.

Last edited 11 years ago by michael (previous) (diff)

comment:4 by Doménique, 11 years ago

Yes, currently the patch does not change the overall behaviour of the Pack component. This means everything works on a per unit basis, no mass upgrading can be done (although you could use this functionality on an array of entities, of course). So if an entity has a Pack component (as given by its xml code) it will show a pack/unpack icon in the GUI for each entity. For your idea to work (upgrade a soldier based on some event/criteria without explicit user intervention) there should be an extra bit of info the XML code; whether the pack/unpack (or upgrade, in such a case) can be user initiated, so users cannot promote units at will :)

comment:5 by leper, 11 years ago

Keywords: review removed

See the comment I added in r13504 for the TODO. You need to create a preview entity, that doesn't influence the simulation, so using a local entity would be best (for checking placement that is).

The patch works as advertised (and your mod is, at least for testing, quite fun). I haven't looked at the code a whole lot, but I still got some comments:

  • The unit_commands.js additions aren't really working (as the pack/unpack button just hides the first trainable entity)
  • CanTransformHere() should just return false if previewEntity == INVALID_ENTITY; you should create a local preview| entity, also that makes the MoveOutOfWorld() unneeded.
  • In PackProgress() why don't you use a for each (var a in b) loop for the garrisoning loop? Also that unloading and then garrisoning again is a bit strange IMO, why not try to move it and if not ungarrison? (though that could need some changes to GarrisonHolder)
    • Would using Array.filter be nicer for the ProductionQueue transfer?
  • The GetQueue() change should be a new method, as adding parameters for something completely different isn't really descriptive.

comment:6 by Doménique, 11 years ago

I will be away for a week, but when I get back I will make sure this patch gets updated. I think I can incorporate nearly all suggestions (I have to study the code a bit). There has been very little time for 0 A.D. on my side lately, which explains why I hadn't responded yet, but next week I can take a few hours to work this out :)

comment:7 by sanderd17, 11 years ago

dvangennip, any progress on this?

comment:8 by Doménique, 11 years ago

Yes, there has been some progress but I haven't wrapped up the work in a new patch as some steps remain to be done (and I wildly overestimated my time during a move from one country to another, including getting a PhD off the ground). From the previous list in comment 5:

  • unit_commands.js: I still have to look at that.
  • Territory checks now function as intended with a preview|entity :) But it seems the MoveOutOfWorld() is still required? Without it I run into trouble.
  • Using a FOR EACH (var A in B) looks nicer, but only really works if no temporary reference to the to-be-transferred entity is required. In such a loop var A goes empty once a unit is removed from the old GarrisonHolder's entity list, which gives trouble on the next step; garrisoning the now released entity (which is no longer defined within the garrisoned entities list). As for removing the ungarrisoning step: for that to work the old entity must be made aware that its garrisoned entities are gone (thus, have been properly removed from its list). Otherwise the new entity may think the entities are garrisoned inside, while actually those are out on the streets because oldEntity.destroy() ungarrisoned them (since the old entity still had 'm in its list). This means the !Garrison(entity) function needs changes so it can update the old garrisonholder (that is, remove the entity from that garrisonholder's entities list). That boils down to the same steps, but would be a lot harder to accomplish in the GarrisonHolder component because that code has no way of knowing from which other GarrisonHolder the to-be-garrisoned entity came from. I hope my explanation makes sense, it gets a bit complex :)
  • Using Array.filter might not improve or shorten the code, but I plan to try it as I haven't used Array.filter before.
  • I split my two functionalities for GetQueue() into two functions.

The take home message: it's not done yet, although it shouldn't take me more than an evening. I've just put it in my agenda for this weekend, so I'll upload a new patch by then. If the upcoming alpha is in any way held back by this patch, feel free to assign it to the next one as no units currently use this logic.

comment:9 by sanderd17, 11 years ago

Milestone: Alpha 14Alpha 15

No problem, it's just nice to know the current situation.

I'll push this to A15 as the feature freeze is for in a few days.

Thanks for working on it.

by Doménique, 11 years ago

Attachment: pack_v2.patch added

Improved patch for enabling unit and structure packing/unpacking

comment:10 by Doménique, 11 years ago

I just uploaded an updated patch with the current state. It's still not ready but at least it compiles against the current svn revision. There are two issue at the moment preventing this patch from being implemented:

  • (GENERAL CODE) There is a general commands panel bug. As leper correctly wrote, any pack command button is laid over the first trainable entity. Thus that unit cannot be trained, which is a show stopper. Worse, when the (un)packable entity is not the first one selected, the Pack buttons are not shown at all (or maybe its panel is hidden below a trainable/buildable buttons panel). This is not unique to my patch but rather an existing bug. Normally it doesn't rear its ugly head, because currently there are no packable units that can also train or build entities. I'm not sure whether this bug warrants a separate ticket. Perhaps I can find a fix, but that will take some extra time as I'm not really familiar with the GUI code.
  • (THIS PATCH) The transfer of the production queue needs some work. Testing shows it refunds money upon transforming, while the queue does get transferred (a.k.a. free units). Sometimes the training time resets as well. This worked fine before :S

So I'll work on it a little more. Any tips for the first issue are appreciated.

Version 0, edited 11 years ago by Doménique (next)

comment:11 by Doménique, 11 years ago

Description: modified (diff)
Owner: set to Doménique
Status: newassigned

comment:12 by sanderd17, 11 years ago

That's good progress.

For the first issue, I think it would be good to open a new ticket. As it's a bug that's not related to your code.

Some hints on where to find the right pieces of code: in public/gui/session/session.xml, the placement of the panels are defined. It's a big file, but a search for "Pack" brings you to the right part. The problem arises because different panels are placed on top of each other, and filled separately.

The filling of a panel with icons (notice that the pack panel may take up to 4 icons) happens in public/gui/session/unit_commands.js.

IMHO, the right panel is for construction, training and researching technologies. Not for unit conversion like packed-unpacked, or gate open-closed. But I believe you'll find a decent way on how to do it.

The wiki also contains some info on the GUI: http://trac.wildfiregames.com/wiki/GUI_-_Scripting_Reference.

comment:13 by Doménique, 11 years ago

I've created another ticket for the general commands panel bug, see #2084.

comment:14 by wraitii, 10 years ago

Milestone: Alpha 15Alpha 16

Any update, dvangennip?

comment:15 by Doménique, 10 years ago

No, unfortunately not. I've been busy with other stuff, which left very little time for 0 A.D. Since my upgrade to OS X Mavericks I have not been able to compile the game, so that poses a problem for development.

comment:16 by leper, 10 years ago

Milestone: Alpha 16Backlog

comment:17 by historic_bruno, 10 years ago

Owner: Doménique removed
Status: assignednew

comment:18 by sanderd17, 10 years ago

I anyone wants to continue working on this. The remaining problem of overlapping buttons should be easy enough to solve.

In the rather new selection_panels file, you can add a setPosition method to the Training object (similarly to Research and Command having a custom setPosition).

This one should simply shift the position up the number of positions already placed in the pane. Yse numberOfRightPanelButtons(), which gets you the number of icons added to the right panel so far. As "Gate" and "Pack" are handled before "Training", it should count their icons.

Only thing you have to watch out for is for the overflow. Now there's a fixed maximum number of buttons per panel. But if buttons start to shift based on other panels, it should probably be turned into a calculating function rather than a constant number.

comment:19 by sanderd17, 10 years ago

In 15387:

Allow mixing the buttons for the right pane, so make it possible to have a structure that can pack and produce units. Refs #1919

comment:20 by fatherbushido, 7 years ago

refs r18467

comment:21 by fatherbushido, 6 years ago

Description: modified (diff)

It seems that we can close it as fixed by r18467?

comment:22 by leper, 6 years ago

Milestone: BacklogAlpha 21
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.