New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Bib & footprint cells - part 1 #13561

Merged
merged 4 commits into from Jul 5, 2017

Conversation

Projects
None yet
3 participants
@reaperrr
Contributor

reaperrr commented Jun 28, 2017

This PR tries to get stuff out of the way that is easy, but would blow the number of commits and changed lines out of proportion on #13553.

Adds ITargetableCells interface for use in #13553, renames Bib, and removes the bib-related hacks from Building and FootprintUtils, while adding a LocalCenterOffset to Building, to allow shifting the actor center to account for bibs (or other visuals, if modders chose to use it that way).
Finally, moves the renamed WithBuildingBib to Mods.Cnc.

Dependency for update of #13553.

@reaperrr reaperrr added this to the Playtest featuring updated HitShapes milestone Jun 28, 2017

@reaperrr reaperrr requested a review from pchote Jun 28, 2017

@reaperrr reaperrr changed the title from Targetable footprint cells - part 1 to Refactor Bib & Targetable footprint cells - part 1 Jun 28, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 28, 2017

Contributor

Note: The reason I didn't move the bib trait to Mods.Cnc is the preview init stuff, which I didn't feel like messing with.

Contributor

reaperrr commented Jun 28, 2017

Note: The reason I didn't move the bib trait to Mods.Cnc is the preview init stuff, which I didn't feel like messing with.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 28, 2017

Member

Note: The reason I didn't move the bib trait to Mods.Cnc is the preview init stuff, which I didn't feel like messing with.

I fixed that for you in #13562.

Member

pchote commented Jun 28, 2017

Note: The reason I didn't move the bib trait to Mods.Cnc is the preview init stuff, which I didn't feel like messing with.

I fixed that for you in #13562.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 28, 2017

Contributor

Updated and rebased on #13562.

Contributor

reaperrr commented Jun 28, 2017

Updated and rebased on #13562.

@reaperrr reaperrr removed the PR: Rebase me! label Jul 1, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 1, 2017

Contributor

Rebased.

Contributor

reaperrr commented Jul 1, 2017

Rebased.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 1, 2017

Contributor

Before this is approved/merged, I'm not 100% sure how the 'bib' cells of the footprints were treated before.
They definitely need to be treated like '=' for placement purposes (except for mini-bibs), but I'm not 100% sure about pathfinding, because if the pathfinder doesn't consider them 'pathable', densely packed bases might spell trouble for the pathfinder.

Should we maybe introduce a - cell type that behaves like = for placement/occupation, but like _ for pathfinding, and use that for bib cells?
I can probably move the FootprintCellType enum to this PR to do this, to keep temporary hacks to a minimum.

I just need approval of that idea first, before I go through the hassle of updating the upgrade rule and yaml changes.

EDIT: It doesn't look like the above is necessary, since FootprintUtils.PathableTiles is only used by bridges and tunnels, which don't use = cells anyway.

Contributor

reaperrr commented Jul 1, 2017

Before this is approved/merged, I'm not 100% sure how the 'bib' cells of the footprints were treated before.
They definitely need to be treated like '=' for placement purposes (except for mini-bibs), but I'm not 100% sure about pathfinding, because if the pathfinder doesn't consider them 'pathable', densely packed bases might spell trouble for the pathfinder.

Should we maybe introduce a - cell type that behaves like = for placement/occupation, but like _ for pathfinding, and use that for bib cells?
I can probably move the FootprintCellType enum to this PR to do this, to keep temporary hacks to a minimum.

I just need approval of that idea first, before I go through the hassle of updating the upgrade rule and yaml changes.

EDIT: It doesn't look like the above is necessary, since FootprintUtils.PathableTiles is only used by bridges and tunnels, which don't use = cells anyway.

@reaperrr reaperrr changed the title from Refactor Bib & Targetable footprint cells - part 1 to Refactor Bib & footprint cells - part 1 Jul 1, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 4, 2017

Contributor

Updated.
As discussed on IRC, I've deferred the HasMinibib property removal to a follow-up, to make this PR a bit easier to review and get it merged more quickly.

Contributor

reaperrr commented Jul 4, 2017

Updated.
As discussed on IRC, I've deferred the HasMinibib property removal to a follow-up, to make this PR a bit easier to review and get it merged more quickly.

@@ -180,12 +181,17 @@ public Building(ActorInitializer init, BuildingInfo info)
occupiedCells = FootprintUtils.UnpathableTiles(self.Info.Name, Info, TopLeft)
.Select(c => Pair.New(c, SubCell.FullCell)).ToArray();
targetableCells = FootprintUtils.UnpathableTiles(self.Info.Name, Info, TopLeft)

This comment has been minimized.

@reaperrr

reaperrr Jul 4, 2017

Contributor

Note to reviewers: This duplication is intended for now, #13553 will update this part as soon as this PR is merged.
This is primarily a measure to keep the diff of the next revision of #13553 in check, as code-wise that will be harder to review than this PR.

@reaperrr

reaperrr Jul 4, 2017

Contributor

Note to reviewers: This duplication is intended for now, #13553 will update this part as soon as this PR is merged.
This is primarily a measure to keep the diff of the next revision of #13553 in check, as code-wise that will be harder to review than this PR.

@abcdefg30

Looks good to me otherwise. 👍 once those comments have been resolved.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 5, 2017

Contributor

Updated.

Contributor

reaperrr commented Jul 5, 2017

Updated.

@pchote

pchote approved these changes Jul 5, 2017

Looks good as far as I can tell aside from that one nit. Feel free to merge once you update that.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 5, 2017

Contributor

Updated.

Contributor

reaperrr commented Jul 5, 2017

Updated.

@reaperrr reaperrr merged commit 8f4a92a into OpenRA:bleed Jul 5, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@reaperrr reaperrr deleted the reaperrr:targetable-cells-pt1 branch Jul 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment