Opened 8 years ago

Closed 6 years ago

#4268 closed defect (fixed)

[PATCH] Destroy sheep corpses when constructing buildings

Reported by: elexis Owned by: elexis
Priority: Must Have Milestone: Alpha 23
Component: UI & Simulation Keywords: patch
Cc: pathfinding Patch: Phab:D21 Phab:D1415

Description (last modified by Itms)

When a sheep or other animal is killed and a building is constructed in the same place, gathering units will try to reach the corpse but never reach it. When constructing buildings, these corpses should be deleted.

Attachments (1)

commands.txt (6.7 KB ) - added by elexis 8 years ago.
Sample replay for alpha 21 / r18804. The killed sheep is still visible and selectable in the middle of the fortress.

Download all attachments as: .zip

Change History (21)

comment:1 by fatherbushido, 8 years ago

I don't know if we have still the related bug: 'the ptolemaic camel archer kill a sheep wich was moving near the cc and then it tries to gather the corpse and doesn't suceed)".

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

comment:2 by elexis, 8 years ago

r17156 introduced the bug, before then the entity was destroyed.

Before that commit, the obstruction of the dead animal was:

    obstruction: {
        active: true,
        blockMovement: true,
        blockPathfinding: false,
        blockFoundation: false,
        blockConstruction: true,
        disableBlockMovement: false,
        disableBlockPathfinding: false,
        shape: {
            type: "cluster"
        }

After that commit it was

    obstruction: {
        active: true,
        blockMovement: false,
        blockPathfinding: false,
        blockFoundation: false,
        blockConstruction: false,
        disableBlockMovement: false,
        disableBlockPathfinding: false,
        shape: {
            type: "static",
            width: 2,
            depth: 2
        }
    },

I have set the blockConstruction flag in the XML part of the TemplateLoader for dead gatherable animals, but that alone didn't do the trick. This is also the flag that the Foundation component checks for in the Build function (defined in GetUnitCollisions()). The blockMovement tag also wasn't it.

by elexis, 8 years ago

Attachment: commands.txt added

Sample replay for alpha 21 / r18804. The killed sheep is still visible and selectable in the middle of the fortress.

comment:3 by wraitii, 8 years ago

See logs on October 8 2016 for more information on this issue.

Git branch for testing: https://github.com/wraitii/0ad/tree/sheepFix

Should focus on putting various stuff under foundations and seeing how it reacts, and also check if walls and gates act sanely.

comment:4 by elexis, 8 years ago

Keywords: patch rfc added
Milestone: BacklogAlpha 22
Summary: Destroy sheep corpses when constructing buildings[PATCH] Destroy sheep corpses when constructing buildings
  • The flags should be accessible through the component, defined in JS, or it should become a boolean argument

comment:5 by elexis, 8 years ago

Looks better with the most recent git commit, but I think that Obstruction.js comment wasn't intentional.

comment:6 by wraitii, 8 years ago

Keywords: roc review added; rfc removed

comment:7 by elexis, 8 years ago

Keywords: roc removed

comment:8 by scythetwirler, 7 years ago

Keywords: beta added

comment:9 by wraitii, 7 years ago

Rebased branch, squashed into a single commit. Ready for review and commit.

comment:10 by elexis, 7 years ago

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:11 by Itms, 7 years ago

Description: modified (diff)

Style fixes:

  • the PROFILE call has a typo in the name
  • the split line in GetStaticsOnObstruction has bad indentation (coming from copy-paste), see the other function and copy that indentation.
  • remove the comment in GetAllOnObstruction
  • you didn't replace all occurrences in the public mod of GetUnitCollisions that you removed from the API

For me the big problem of this patch is this boolean passed to GetCollisions which has an obfuscated name. It should be clear in the API what this boolean does (i.e. without having to understand the whole code in the Obstruction component).

I suggest calling the boolean onlyConstructionBlocking (and probably making it optional, with a default value of true). Then comments should be detailed more in the function (explain in the first comment that living animals have the flag while dead ones are static obstructions without the flag). Add a remark that if the boolean is false, the filter filters nothing and is equivalent to NullObstructionFilter.

Then I would add a comment in the Foundation.js code to explain that this code cleans everything under the foundation (corpses and other non foundation-blocking obstructions).

comment:12 by Itms, 7 years ago

Keywords: review removed

comment:13 by Itms, 7 years ago

Description: modified (diff)
Patch: Phab:D21

comment:14 by elexis, 6 years ago

Keywords: beta removed
Milestone: Work In ProgressAlpha 23

comment:15 by elexis, 6 years ago

Owner: set to elexis
Resolution: fixed
Status: newclosed

In 21597:

Delete sheep corpses when starting to build a foundation,

so that units don't try to gather forever while not being able to reach it and
so that the sheep corpse can't be selected and seen anymore.

Differential Revision: https://code.wildfiregames.com/D21
Fixes #4268
Based On Patch By: wraitii
Reviewed By: temple
Previously Reviewed By: Itms

comment:16 by elexis, 6 years ago

Priority: Must HaveRelease Blocker
Resolution: fixed
Status: closedreopened

Everyone who had written or reviewed the patch (me, temple wraitii, Itms) haven't considered the flag 0 well evidently.

As reported by temple in Phab:rP21597, placing a foundation in the fog of war yields a foundation that is never going to be constructed as it is blocked by it's own mirage.

comment:17 by elexis, 6 years ago

Patch: Phab:D21Phab:D21 Phab:D1415

comment:18 by elexis, 6 years ago

In 21624:

Fix mirages (and any other entity that blocks something but not BlockConstruction) blocking foundation construction following rP21597 / D21.
Clean the code by removing the animal hardcoding in the Foundation component and adding a flag DeleteUponConstruction to the Obstruction component.

Have locked gates and upgraded entities equally delete entities when transforming.
Add a workaround for trees inside walls on random maps.

Reviewed by: temple
Differential Revision: https://code.wildfiregames.com/D1415
Refs #4268

comment:19 by elexis, 6 years ago

Priority: Release BlockerMust Have

comment:20 by elexis, 6 years ago

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.