Opened 7 years ago

Closed 7 years ago

#4490 closed defect (fixed)

Boudica shows up as idle worker and breaks the petra bot

Reported by: elexis Owned by:
Priority: Should Have Milestone: Alpha 22
Component: UI & Simulation Keywords:
Cc: Patch:

Description

If you press the idle worker hotkey, the briton heroine Boudica will show up as an idle worker, due to var g_WorkerTypes = ["Female", "Trader", "FishingBoat", "CitizenSoldier"]; in session.js.

We likely shouldn't make that Support, but rather use the MatchClassesList thing to exclude military units.

Attachments (1)

support_female.patch (15.0 KB ) - added by elexis 7 years ago.
solely removing the Female class from Boudica and session.js works too, dont really care

Download all attachments as: .zip

Change History (24)

comment:1 by fatherbushido, 7 years ago

Yes (see also your utility cleanup diff). (Else I am not sure that Boudica should be Female.)

comment:2 by mimo, 7 years ago

Going to support would nonetheless have the advantage that mauryan worker elephant would be included, which doesn't seem to be the case now.

comment:3 by elexis, 7 years ago

I considered that a disadvantage as you can't force it to do anything useful in a lot of time, thus would often remain idle, showing up again and again when pressing the hotkey. No strong feelings though

comment:4 by elexis, 7 years ago

Expanding on the elephant:

The use case of the idle worker hotkey is to assign all units a task, so that none show up as idle anymore.

Citizen soldiers and females can gather resources, repair entities, walk and attack, so they always have a task they can accomplish. If they can't be used for anything, the units are typically deleted and replaced with military units or traders.

A worker elephant can only walk and construct, while it also can be used as a mobile dropsite. So it would be a false positive if the elephant is solely used as a dropsite outside of the territory or if it has nothing to build currently, which is true for the majority or a significant part of its lifespan.

comment:5 by elexis, 7 years ago

Keywords: simple removed
Milestone: BacklogAlpha 22

Fixed by r19301

Thanks for the patch Boudica, perfect ticket to start with! ;)

comment:6 by fatherbushido, 7 years ago

(Perhaps also Boudica is not a Female)

comment:7 by elexis, 7 years ago

Still don't share that opinion

comment:8 by fatherbushido, 7 years ago

That's not an opinion, that's a question. Define. Some people started adding Classes for things following what they thought it shoud/could be.

comment:9 by fatherbushido, 7 years ago

Else I have examples, but there is not enough place in that margin.

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:10 by elexis, 7 years ago

Definitions derive from opinions. IMO Female should be given for female units, so that we can query all females, not only the support ones.

comment:11 by fatherbushido, 7 years ago

I don't care about opinion. I care about code (it defines things and I respect it). Female could have been called String or Car or even 011110110, it would mean the same thing. Perhaps names are bad but then they can be replaced. But that's another story.

Refs wolves, sheeps.

comment:12 by mimo, 7 years ago

I'd agree with fatherbushido here. What is important is what is the use of the class Female in the game? and i don't see any where it would be useful to have boudica included, so we should remove this class for that hero. There is at least one use case i know, which is the AI, and it uses Female as a way to select workers which are not CitizenSoldiers. I do not think we'll have any other use of this class.

comment:13 by elexis, 7 years ago

Yep, we don't have a use case for querying female units other than support females. Yet.

Using precise language prevents doing mistakes based on wrong assumptions. If an entity has the Female calss, one would expect it to query female entities. If an entity has the Domestic class, one would expect it to query domestic animals Calling it SupportFemale would be more clear and be more safe if we add a Female class for some fertility tech that applies to all female units for example. Looks like there are < 10 lines to change to accomplish that.

comment:14 by mimo, 7 years ago

come on, if you want to differentiate according to gender (but for which use in our game) do it as is done for the maiden archer which have a <Gender>female</Gender>. But once again, what would be the use case? our graphical units are good enough that people should see the distinction :-) Then what about dogs and elephant having the class Human? that is perfectly fine for me, and would rather lead to additional complexity if they were not. And i'm pretty sure we have a lot of other classes which the classes are not rigorously exact, but an easy simplification. That said, if you want to add additionnal classes to clean all that, please do it, i've nothing against except that i think it is useless and bug prone. But currently the boudica Female class (which i was not aware of) breaks the AI (hopefully this must not noticeable as nobody reported it).

comment:15 by elexis, 7 years ago

come on, if you want to differentiate according to gender (but for which use in our game) do it as is done for the maiden archer which have a <Gender>female</Gender>.

That wouldn't work with the example I gave afaik (a tech that buffs all gender female units)

Having elephants Human and then using Human in some templates/techs/auras and Organic in others is the result of error prone imprecise language, no? (also #4224 which seems outdated by later design decisions)

Don't want to add another class, just rename the existing one to be more precise, then we can remove it from Boudica et al.

Doesn't look like that AI query mismatch with boudica was noticed before as one rarely sets that hero to passive

by elexis, 7 years ago

Attachment: support_female.patch added

solely removing the Female class from Boudica and session.js works too, dont really care

comment:16 by fatherbushido, 7 years ago

Something like that is ok.

comment:17 by fatherbushido, 7 years ago

(just for refs, it was introduced in r19095 and partially removed in r19103, see also the comment http://trac.wildfiregames.com/ticket/4147#comment:10)

comment:18 by fatherbushido, 7 years ago

(not related but as it's modified in the diff above, EjectClassesOnDestroy should be cleaned a bit. Perhaps it's my fault in a previous commit).

edit: 4 lines in one hour (5 with that one and 6 with the blank one), will that count for my activity account? (Uhm I am at 7 now).

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:19 by elexis, 7 years ago

The template entries of EjectClassesOnDestroy appeared like the intention was to eject non-mounted units, as mounted-units might be too slow to accomplish that. But it can be changed if you feel that makes more sense.

comment:20 by fatherbushido, 7 years ago

Oh no, idc, just noticing. (++lines)

comment:21 by elexis, 7 years ago

Boudica class removal here https://code.wildfiregames.com/D242 Female -> SupportFemale rename there https://code.wildfiregames.com/D244

comment:22 by elexis, 7 years ago

In 19329:

Rename the Female identity class to FemaleCitizen, because it is intended for female support units only.

Thus prevent template editors from adding that tag to female military units and
allow modders to add a Female class for wider purposes.
Remove the FemaleCitizen training restriction category that was added for debug purposes but forgotton to be removed in rP12832.

Differential Revision: https://code.wildfiregames.com/D244
Reviewed By: fatherbushido
Refs: #1432 #4490

comment:23 by elexis, 7 years ago

Priority: Nice to HaveShould Have
Resolution: fixed
Status: newclosed
Summary: Boudica shows up as idle workerBoudica shows up as idle worker and breaks the petra bot

MatchesClassesList support for idle workers added in rP19301 / https://code.wildfiregames.com/D223 Female tag that broke the bot removed in r19328 https://code.wildfiregames.com/D242 Rename from Female to FemaleCitizen to prevent repetition of this mistake in r19329 / https://code.wildfiregames.com/D244

Thanks for the patch, discussion and reviews!

Note: See TracTickets for help on using tickets.