Page MenuHomeWildfire Games

Handle entityrenamed on formation aura's
Changes PlannedPublic

Authored by temple on Oct 31 2017, 8:22 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

As pointed out by The Undying Nephalim on the forum, when units are promoted they lose their position in their formation. They also lose any aura bonuses. This patch fixes that.

Ideally we should check if the new and old units have formation auras, and apply or remove them. But currently only Athens' Iphicrates has a formation aura (+3 armor), and he can't be promoted, so it's not an issue.

Test Plan

Verify that promoted units (use the developer overlay) now stay in formation and keep their formation aura bonus.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4508
Build 7875: Vulcan Lint
Build 7874: Vulcan Build
Build 7873: arc lint + arc unit

Event Timeline

temple created this revision.Oct 31 2017, 8:22 PM
bb retitled this revision from Remember promoted unit's position in formation to Handle entityrenamed on formation aura's.Dec 28 2017, 12:50 PM
bb added a subscriber: bb.Dec 28 2017, 12:57 PM
bb added inline comments.
binaries/data/mods/public/simulation/components/Formation.js
954–955

is it such a hassle to do this? maybe make a new prototype function in handling the aura changes to avoid duplication (the code handling it is in AddMembers)

temple added inline comments.Jan 12 2018, 7:17 PM
binaries/data/mods/public/simulation/components/Formation.js
954–955

I'd rather save it for another patch.

temple updated this revision to Diff 5345.Jan 16 2018, 8:38 PM
temple added a reviewer: Restricted Owners Package.
temple set the repository for this revision to rP 0 A.D. Public Repository.

inline

I'd still prefer to save the todo for another patch, since it's not affecting anything (in vanilla 0AD anyway).

mimo added a subscriber: mimo.Jan 18 2018, 8:25 PM
mimo added inline comments.
binaries/data/mods/public/simulation/components/Formation.js
946

Not clear to me why you'd need that? it looks to me as if you should only check if the renamed entity was in this.formationMembersWithAura, and if yes, remove it, adds the renamed one in this.formationMembersWithAura and adds it the aura.

temple added inline comments.Jan 23 2018, 9:16 PM
binaries/data/mods/public/simulation/components/Formation.js
946

I think you're talking about the other half, the part I'm skipping in this diff.
Here we need to test if e.g. Iphicrates is in our formation (in vanilla 0AD he's the only possible candidate to be in this.formationMembersWithAura), and if so apply that bonus to the renamed entity.

mimo added a comment.Jan 24 2018, 7:04 PM

As fixing the TODO would need to change back the lines you are modifying in that patch, i think we should include it.

At first sight, it seems that to avoid useless operations, the auras component would need to be completed (to allow checking if two entities have some common and different auras), but we can already easily treat it in a non-optimal way: is the msg.entity has a formation aura (i.e. is in formationmemberswithaura) remove its aura from the formation, then apply the remaining formation auras to the new entity and finally if the msg.newentity has a formation aura, add it to the formation, with a TODO for improving it later.

binaries/data/mods/public/simulation/components/Formation.js
934

would be better with a return when index == -1

temple planned changes to this revision.Feb 17 2018, 7:00 PM

Their position within the formation is fixed after rP21243.

I think might be able to do RemoveMembers then AddMembers instead, but I need to look more carefully.