#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 )
Since r17132 you can't hunt chicken or deer anymore. It fails about 95% of the time, other animals work mostly.
Reproduce:
- Select cavalry, infantry or woman
- Attack a chicken
The chicken is killed, but then the unit stops while staying in the walk-animation.
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)
Change History (20)
by , 8 years ago
Attachment: | chickendance.gif added |
---|
comment:1 by , 8 years ago
Description: | modified (diff) |
---|
by , 8 years ago
Attachment: | commands.txt added |
---|
comment:2 by , 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 , 8 years ago
Attachment: | testLine.patch added |
---|
follow-up: 4 comment:3 by , 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.
comment:4 by , 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
- add debug output to
comment:5 by , 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 , 8 years ago
Attachment: | t3530_unobstructive_corpses_v1.patch added |
---|
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 , 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 , 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 , 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 , 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 , 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 , 8 years ago
Attachment: | SaveTheChickenNotTheWhales.patch added |
---|
Fixes the issue (this is a debug patch)
comment:10 by , 8 years ago
Just realized I messed up previews in that patch, but that doesn't matter.
comment:14 by , 8 years ago
Keywords: | review removed |
---|
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.