Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3530 closed defect (fixed)

[PATCH] Pathfinder - can't hunt chicken anymore

Reported by: elexis Owned by: wraitii
Priority: Must Have Milestone: Alpha 19
Component: Core engine Keywords: patch pathfinding
Cc: Patch:

Description (last modified by elexis)

Since r17132 you can't hunt chicken or deer anymore. It fails about 95% of the time, other animals work mostly.

Reproduce:

  1. Select cavalry, infantry or woman
  2. Attack a chicken

The chicken is killed, but then the unit stops while staying in the walk-animation.

http://trac.wildfiregames.com/raw-attachment/ticket/3530/chickendance.gif

If you apply the changeset of that commit (r17132) to previous revisions the same bug applies too. So that commit only revealed the bug.

Hopefully a duplicate of #3505 (which is hopefully a duplicate of #3471). Originally reported in comment:7:ticket:3505.

Attachments (6)

chickendance.gif (421.2 KB ) - added by elexis 8 years ago.
commands.txt (88.0 KB ) - added by elexis 8 years ago.
Only one command. Initially stuck without walk-animation. Entering the walk animation after three minutes. Brief movement after 10minutes, but getting stuck immediately again. Still stuck after 16mins.
testLine.patch (748 bytes ) - added by mimo 8 years ago.
t3530_unobstructive_corpses_v1.patch (819 bytes ) - added by elexis 8 years ago.
Duplicates the obstruction-removal code that corpses have when creating the resource-template after killing animals. Thus no unit shape will be added, we'll have the performance improvement, the bug will not occur and siege is not blocked anymore by dead sheep. We should still check the clearance = 0 thing and make sure it doesn't fail.
t3530_throw_warnings.patch (845 bytes ) - added by elexis 8 years ago.
There might be other cases where an illegal unit shape obstruction (one with clearance 0) is added. We should warn in that case.
SaveTheChickenNotTheWhales.patch (2.0 KB ) - added by wraitii 8 years ago.
Fixes the issue (this is a debug patch)

Download all attachments as: .zip

Change History (20)

by elexis, 8 years ago

Attachment: chickendance.gif added

comment:1 by elexis, 8 years ago

Description: modified (diff)

by elexis, 8 years ago

Attachment: commands.txt added

Only one command. Initially stuck without walk-animation. Entering the walk animation after three minutes. Brief movement after 10minutes, but getting stuck immediately again. Still stuck after 16mins.

comment:2 by mimo, 8 years ago

Corpses have clearance 0 and this seems to be not correctly dealt with in the obstructionManager. A possible fix is attached, but it may be worth to understand what's wrongly done in the Geometry::TestRayAASquare function called from CCmpObstructionManager::TestLine.

by mimo, 8 years ago

Attachment: testLine.patch added

comment:3 by mimo, 8 years ago

Reading the logs on IRC, It seems that this bug raises some questions. Here are some comments:

  • why is it only apparent after r17132 ? that's because the gatherDistance is 2.0, and the unit try to go to gatherDistance-GoalDelta. before r17132, Goaldelta was 0 so the unit tried to go at a distance of 2 from the corpse, and that was fine because the test of obstruction on the corpse was on 0+1 (clearance of the corpse + clearance of the entity). But after r17132, GoalDelta is 1 and thus the unit tries to go to a distance of 1 which is considered as obstructed. A simple test is to check that changing the gatherDistance to 2.5 make the bug disappear.
  • what should we do to fix it ? there is of course to understand what an obstruction of 0 means, and if we should forbid such obstruction in the Engine (even if we fix the corpse case, modders could always create such cases). If obstructions 0 are prevented, then no additionnal fix is needed, but otherwise the fix attached here is needed.

But in addition, i do not expect (hoping I'm wrong) that somebody will have a look at these obstructions soon, so if we want to have a playable game such that people can continue testing a19, the patch attached is still worth as a temporary fix.

  • most of the required distances (gather, trade, attack, ...) should ideally depends on the unit clearance instance of some fixed values.

in reply to:  3 comment:4 by elexis, 8 years ago

Keywords: patch review added
Summary: Pathfinder - can't hunt chicken anymore[PATCH] Pathfinder - can't hunt chicken anymore

Replying to mimo:

  • why is it only apparent after r17132 ?...

Ah, so

  • the gatherdistance is defined as 2 in both cases
  • the obstruction size of the corpse is 1 in both cases (sheep clearance 0 + unit clearance 1)
  • the geometry doesn't allow gathering at the eddge of the corpse (i.e. unit distance must be greater than 1)
  • when gathering, the unit is told to walk to gatherDistance-GoalDelta distance to the center of the corpse
    • which has been 2-0 before that commit (thus gathering with actual distance 2, no blocking)
    • and has become 2-1 after that commit (thus attempting to gather with actual distance 1, blocking)

worth as a temporary fix

Everything's better than that bug. A comment in the code why we introduce the check might be nice for the future.

2. Proposal: On the other hand, the real problem seems to be to gather at gatherDistance-goalDelta, i.e. extending the goal inwards instead of outwards, i.e. having to gather inside of the corpse (I know right?). If you look at the comment in Pathfinding.h for that variable, it says:

	/**
	 * For extending the goal outwards/inwards a little bit
	 * NOTE: keep next to the definition of NAVCELL_SIZE to avoid init order problems
	 *	between translation units.
	 */
	const entity_pos_t GOAL_DELTA = NAVCELL_SIZE;

So shouldn't we rather make it a + instead of - gather distance, so that units will gather from 1 distance around the corpse, no matter how large (fat?) it is? Imagine for some reason corpses would later have clearance 1. Then they couldn't gather at distance 2 as the obstruction had the distance 2 too.

We should look at why other resources don't have the same problem.

3. Proposal: Ideally the corpse shouldn't produce an obstructions in the first place.

  • Dead sheep would also not block siege anymore, which has been reported on march 1st in irc:

    (15:04:26) cc_: hum dead sheep are an obstruction for siege engines

  • A bit of performance improvement
  • Regular corpses already don't produce obstructions, so it would be somewhat consistent
    • add debug output to AddUnitShape / AddStaticShape to see that these functions are not called when killing humans

comment:5 by mimo, 8 years ago

No, the UnitMotion does not work that way and you've misunderstood the basics. When you give an order MoveToTargetRange(Rmin,Rmax), you expect to go at a distance Rmin<=R<=Rmax, so it is expected that GoalDelta is subtracted to Rmax (and added to Rmin). Then, obstructions of the target and entities are taken into account and the nearest possible point will be reached. The problem here is that the unit motion knows that the obstruction of the corpse was zero, so it thinks it can bring the unit at a distance 1, but the function which checks if a move is obstructed uses the points with obstruction=0.

by elexis, 8 years ago

Duplicates the obstruction-removal code that corpses have when creating the resource-template after killing animals. Thus no unit shape will be added, we'll have the performance improvement, the bug will not occur and siege is not blocked anymore by dead sheep. We should still check the clearance = 0 thing and make sure it doesn't fail.

comment:6 by elexis, 8 years ago

Unfortunately we can't fix this bug by ignoring entities with clearance 0, as the same bug occurs with the worker elephant, which has clearance 1 (#3539).

by elexis, 8 years ago

Attachment: t3530_throw_warnings.patch added

There might be other cases where an illegal unit shape obstruction (one with clearance 0) is added. We should warn in that case.

comment:7 by mimo, 8 years ago

I suppose the original problem is due to the fact that the unit obstructions (clearance) is now defined by the passability class in the UnitMotion component, and units without UnitMotion like resources have then a clearance of 0. That may be what is wanted (that's nice for chicken, but no obstruction for elephant resource looks weird), but that's a change from A18 which is only driven by some change of the code and we should decide if we want to keep it or not.

The other option would be to define the passability in the Obstruction component rather than the UnitMotion one, and then resources would keep the clearance of their parent (i don't have a18 here, but iirc resources had some obstruction). That would also be quite useful for mods as now only units with UnitMotion can have some obstruction.

I've a slight preference for the second approach, especially if we go later with #3497.

comment:8 by wraitii, 8 years ago

If I understand this correctly, obstruction size 0 means that the unit should not be obstructing? Ie should be equivalent to entirely removing the obstruction component? If so, I think we should forbid it.

I agree with mimo on the unitmotion/obstruction stuff, clearance path is not really related to unit motion. It we want fancy movement someday, they will have to use another parameter anyway.

comment:9 by wraitii, 8 years ago

I've thought about this a bit, here is my definitive take:

-Clearance is a unitMotion thing, a pathfinding component. It is not an obstruction and should not be moved there. In fact, Obstruction should be considered as deprecated for unit shapes, since we basically use none of the functionality. It's only useful for the short-range pathfinder, which could use something else. We ought to remove all unit shapes from the game (Itms agrees).

-Resources should become static obstructions if we want them to obstruct. This fixes all pathfinding issues.

-Clearance 0 should probable be made impossible, or silently tolerated, but it's not really that important for now.

Attached patch turns all resources into small static obstructions when needed. Fixes the chicken issue. A proper commit would probably take a parameter somewhere.

by wraitii, 8 years ago

Fixes the issue (this is a debug patch)

comment:10 by wraitii, 8 years ago

Just realized I messed up previews in that patch, but that doesn't matter.

comment:11 by Stan, 8 years ago

There is a typo in this patch too 'the' instead of 'them':)

comment:12 by wraitii, 8 years ago

Owner: set to wraitii
Resolution: fixed
Status: newclosed

In 17156:

Make resources have a static, passable obstruction. Ought to be a temporary fix. Fixes #3530
See http://trac.wildfiregames.com/ticket/3530#comment:9 for a proper fix to this issue.

comment:13 by Itms, 8 years ago

Ref #3562 for tracking future developments.

comment:14 by mimo, 8 years ago

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