Skip to content
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

Fix RA/TD selection/decoration/interaction bounds #15198

Merged
merged 7 commits into from Jul 1, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Jun 1, 2018

The dynamic/per-frame 'canvas' for SHP(TD) introduced by #15148 messed up the old auto-calculation assumptions.

We already use fixed bounds in most other places (D2k, TS, all moving actors in RA and TD, several structures in RA/TD), so finishing the job for RA/TD buildings is the easiest way to fix #15184, avoiding both reversion of #15148 as well as code-level work-arounds.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 1, 2018

Do we need to do civilian buildings and tech structures too?

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jun 2, 2018

Tested the changes in ra, cnc and ts. In ra and cnc the bounds for civilian buildings/tech buildings are fine, in ts only the scrin ship needs adjustments.

See https://imgur.com/a/VbWDHB8 for screenshots of selected civilian buildings per mod. Note that those buildings in the screenshots that are not selected (couldn't select them for some reason) have correct bounds too.

Edit: Selectable bounds of buildings like ra's windmill or oil derrick and ts' civilian array that have animated components look good too.

@pchote pchote added this to the Next release milestone Jun 2, 2018

@reaperrr reaperrr force-pushed the reaperrr:fix-ra-td-bounds branch from 5a81b3a to 222662d Jun 2, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 2, 2018

Added RA/TD tech buildings to be on the safe side.

@lawando It could be (but I haven't checked yet) that the damaged frame of the civ buildings alters the bounds a bit. That's pretty much the only possible issue to look out for, though.

TS Scrin ship should look like that before #15148 already.
TS buildings should get isometric selection-bounds/-boxes eventually, with the former derived from just the footprint and the latter calculated from footprint + a manually set height, just like in the original, so anything that passes as "acceptable" is good enough for TS buildings for now, because that will be adressed properly later on anyway.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 3, 2018

I have added a section to https://github.com/OpenRA/OpenRAModSDK/wiki/Update-notes:-release-20180307-to-next-release to make modders aware of this change.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 22, 2018

damaged frame of the civ buildings alters the bounds a bit. That's pretty much the only possible issue to look out for, though.

I have filed #15290 to address this, changing this PR to a "simple" polish improvement rather than a bugfix/bandaid.

@pchote
Copy link
Member

pchote left a comment

Just a couple of minor comments:

@@ -70,6 +70,8 @@ LST:
RevealsShroud:
Range: 7c0
WithFacingSpriteBody:
Selectable:
Bounds: 48,40

This comment has been minimized.

@pchote

pchote Jun 22, 2018

Member

IMO this should be 48,48: this matches the original sprite, and looks better across the range of casings.

@@ -200,7 +206,7 @@ PROC:
PipCount: 10
Capacity: 700
Selectable:
Bounds: 72,56,0,12
Bounds: 72,56

This comment has been minimized.

@pchote

pchote Jun 22, 2018

Member

Why remove the offset here?

This comment has been minimized.

@reaperrr

reaperrr Jul 1, 2018

Author Contributor

See description of 77603d2 for my reasoning. If you disagree I can drop this change, but to me the new value feels better.

@@ -33,6 +35,8 @@ FPWR:
SYRF:
Inherits: ^FakeBuilding
Inherits@infiltrate: ^InfiltratableFake
Selectable:
Bounds: 72,72

This comment has been minimized.

@pchote

pchote Jun 22, 2018

Member

This should be 72,48.

@@ -242,6 +246,8 @@ SPEN:

SYRD:
Inherits: ^Building
Selectable:
Bounds: 72,72

This comment has been minimized.

@pchote

pchote Jun 22, 2018

Member

72,48

reaperrr added some commits Jun 1, 2018

Ensure interactable/deco bounds of TD walls are consistent
Regardless of current frame's bounds.
Ensure select/deco bounds of TD buildings are consistent
Regardless of current frame's bounds.
Tweak TD refinery selection bounds
The old vertical offset made the bounds include the entire concrete 'bib', but not the processor tower in the back.

@reaperrr reaperrr force-pushed the reaperrr:fix-ra-td-bounds branch from 222662d to b2cc245 Jul 1, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jul 1, 2018

Rebased and updated.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 1, 2018

Checking against the last release, the (fake + normal) sub pen bounds should also be smaller.

reaperrr added some commits Jun 1, 2018

Fix RA refinery selection bounds offset
Selection bounds were too low on the vertical axis.

@reaperrr reaperrr force-pushed the reaperrr:fix-ra-td-bounds branch from b2cc245 to 642c172 Jul 1, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jul 1, 2018

Checking against the last release, the (fake + normal) sub pen bounds should also be smaller.

Fixed.

@pchote

pchote approved these changes Jul 1, 2018

@pchote pchote merged commit f31e5d1 into OpenRA:bleed Jul 1, 2018

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:fix-ra-td-bounds branch Aug 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.