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 TargetPosition targeting and give D2k buildings better hit-shapes #13196

Merged
merged 4 commits into from Jun 5, 2017

Conversation

Projects
None yet
5 participants
@reaperrr
Contributor

reaperrr commented Apr 25, 2017

The discussion in #13165 leans towards footprint-based hit-shapes, so I've built on that plus the initial review feedback.
Rather than updating the old PR, I've decided to file this separately (mostly due to the branch name, but also to keep the discussion and actual reviewing a bit separate).

This is basically the spiritual successor to #10383 and actually scavenged some code from there.

The commit count makes this look more intimidating than it really is, only the first 3 commits contain code and the individual yaml commits are small.

@reaperrr reaperrr changed the title from Targetable positions to Fix TargetPosition targeting and give buildings footprint-based hit-shapes Apr 25, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 25, 2017

Member

For invisible insta-hit projectiles to shoot at the closest target
position instead, modders should either use Bullet or set an Inaccuracy of
= 1.

Since this requires changing the projectile args IMO it would be clearer and more correct to introduce a specific flag for this than to bolt it on to the inaccuracy.

Member

pchote commented Apr 25, 2017

For invisible insta-hit projectiles to shoot at the closest target
position instead, modders should either use Bullet or set an Inaccuracy of
= 1.

Since this requires changing the projectile args IMO it would be clearer and more correct to introduce a specific flag for this than to bolt it on to the inaccuracy.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 25, 2017

Member

I played with this briefly in TS and TD, and I'm really liking it. Having the impacts land on the "outside" of the buildings make them feel more solid, and overall improves the polish in a small but hopefully significant way.

One issue that I did notice is that units still face towards the center position, which looks quite wrong when you have a gun turret or rocket trooper standing next to the corner of a construction yard.

Member

pchote commented Apr 25, 2017

I played with this briefly in TS and TD, and I'm really liking it. Having the impacts land on the "outside" of the buildings make them feel more solid, and overall improves the polish in a small but hopefully significant way.

One issue that I did notice is that units still face towards the center position, which looks quite wrong when you have a gun turret or rocket trooper standing next to the corner of a construction yard.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 25, 2017

Member

Moving the explosions away from the center of the structure causes a tricky problem in the classic mods: explosions from behind now render behind the structure instead of on top. Forcing a +1 cell z offset on all explosions would work around this, but feels a bit hacky.

Member

pchote commented Apr 25, 2017

Moving the explosions away from the center of the structure causes a tricky problem in the classic mods: explosions from behind now render behind the structure instead of on top. Forcing a +1 cell z offset on all explosions would work around this, but feels a bit hacky.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 25, 2017

Member

The awkward diagonal perspective on the RA conyard means that impacts land visually outside the structure on the bottom corner cells. A potential solution is to have another new trait that lets you list the targetable positions directly, and then fudge those corner points further inwards.

Member

pchote commented Apr 25, 2017

The awkward diagonal perspective on the RA conyard means that impacts land visually outside the structure on the bottom corner cells. A potential solution is to have another new trait that lets you list the targetable positions directly, and then fudge those corner points further inwards.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 26, 2017

Member

It might be better to create a hit-shape that is a collection of hit-shapes then copy one for each occupied cell. The footprint is used for marking occupied positions rather than the shape. Then use new trait as @pchote mentions, or possibly merge ITargetablePositions into ITargetable and extend TargetableInfo to contain something like WVec[] Offsets where each offset is from the center and the default is the center (new WVec[] { new WVec(0, 0, 0) }). This would allow for one Rectangle or Circle hit-shape for some buildings.

Multi-cell hit-shapes could have calculations for each cell for damage.

Member

atlimit8 commented Apr 26, 2017

It might be better to create a hit-shape that is a collection of hit-shapes then copy one for each occupied cell. The footprint is used for marking occupied positions rather than the shape. Then use new trait as @pchote mentions, or possibly merge ITargetablePositions into ITargetable and extend TargetableInfo to contain something like WVec[] Offsets where each offset is from the center and the default is the center (new WVec[] { new WVec(0, 0, 0) }). This would allow for one Rectangle or Circle hit-shape for some buildings.

Multi-cell hit-shapes could have calculations for each cell for damage.

@pchote pchote added this to the Next Release milestone Apr 30, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 1, 2017

Contributor

Having played around with this a bit locally, I think we should either
a) keep ITargetablePositions in a trait separate from Targetable, or
b) do as @atlimit8 suggests.

Reasoning: It is both rather annoying and risky wrt regressions on the yaml side to have to disable TargetableX and enable TargetableY in the rules when you just want to change an inherited target position type and have not intention to change TargetTypes, for example.
In my opinion, for modders it would be best if we either stuffed everything into Targetable or had a separate TargetablePositionsOverride trait that only implements ITargetablePositions and nothing else.

Contributor

reaperrr commented May 1, 2017

Having played around with this a bit locally, I think we should either
a) keep ITargetablePositions in a trait separate from Targetable, or
b) do as @atlimit8 suggests.

Reasoning: It is both rather annoying and risky wrt regressions on the yaml side to have to disable TargetableX and enable TargetableY in the rules when you just want to change an inherited target position type and have not intention to change TargetTypes, for example.
In my opinion, for modders it would be best if we either stuffed everything into Targetable or had a separate TargetablePositionsOverride trait that only implements ITargetablePositions and nothing else.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 1, 2017

Member

I think it would make the most sense to mergeITargetablePositions into IHitShape, and then as suggested have a new type of hit shape for multi-celled actors. Explicitly aggregating other hit shapes sounds like overkill though, I would much prefer that we explicitly defined a polygon which could then be any piecewise-linear shape.

Member

pchote commented May 1, 2017

I think it would make the most sense to mergeITargetablePositions into IHitShape, and then as suggested have a new type of hit shape for multi-celled actors. Explicitly aggregating other hit shapes sounds like overkill though, I would much prefer that we explicitly defined a polygon which could then be any piecewise-linear shape.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 1, 2017

Contributor

Polygon hitshape and related warhead changes are out of my league, though.

I'm also not entirely sure whether we won't run into edge cases where we want a non-CenterPosition targetable position, but not a Health trait, or if 3rd-party mods want a more flexible health system with multiple health / shield traits and such.

Contributor

reaperrr commented May 1, 2017

Polygon hitshape and related warhead changes are out of my league, though.

I'm also not entirely sure whether we won't run into edge cases where we want a non-CenterPosition targetable position, but not a Health trait, or if 3rd-party mods want a more flexible health system with multiple health / shield traits and such.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 1, 2017

Member

I think it would make the most sense to merge ITargetablePositions into IHitShape

I actually had the same idea afterwards and I did say "Then use new trait as @pchote mentions, or possibly merge ITargetablePositions into ITargetable...". Yea, with my idea of using Exits for targetable positions (the genesis of #13208 really), I had abandoned merging into Targetable and now would rather targetable position providers specify valid types via conditions and have Targetable just act as a manager where at least one position must be valid for a targetable type to be valid:

WeapFactWithRepair:
    Targetable:
    Exit@1:
        ...
        ProductionTypes: 
            Vehicle: !powered-down # can't open door without power
        TargetTypes:
            Vehicle.Repair: !powered-down # can't open door without power
    Health:
        ...
        TargetTypes:
            Vehicle:
Member

atlimit8 commented May 1, 2017

I think it would make the most sense to merge ITargetablePositions into IHitShape

I actually had the same idea afterwards and I did say "Then use new trait as @pchote mentions, or possibly merge ITargetablePositions into ITargetable...". Yea, with my idea of using Exits for targetable positions (the genesis of #13208 really), I had abandoned merging into Targetable and now would rather targetable position providers specify valid types via conditions and have Targetable just act as a manager where at least one position must be valid for a targetable type to be valid:

WeapFactWithRepair:
    Targetable:
    Exit@1:
        ...
        ProductionTypes: 
            Vehicle: !powered-down # can't open door without power
        TargetTypes:
            Vehicle.Repair: !powered-down # can't open door without power
    Health:
        ...
        TargetTypes:
            Vehicle:
@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 1, 2017

Member

Such a refactor could easily handle multiple Health traits if they just disable their target types with themselves.

Member

atlimit8 commented May 1, 2017

Such a refactor could easily handle multiple Health traits if they just disable their target types with themselves.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 1, 2017

Member

A (convex) polygon can be handled by decomposing it into right triangles and rectangles. Perhaps it would be better if I worked on the hit shapes then.

Member

atlimit8 commented May 1, 2017

A (convex) polygon can be handled by decomposing it into right triangles and rectangles. Perhaps it would be better if I worked on the hit shapes then.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 1, 2017

Contributor

By the way, one of the reasons why I implemented and think we should continue to offer a way to derive targetable positions from building footprints is that larger buildings simply have a lot of cells, so manually setting targetable positions via WVec lists is awfully inefficient in terms of time. Modders need some automatic, simple way to do that, in my opinion, and occupied cells are the easiest approach in my book.

Contributor

reaperrr commented May 1, 2017

By the way, one of the reasons why I implemented and think we should continue to offer a way to derive targetable positions from building footprints is that larger buildings simply have a lot of cells, so manually setting targetable positions via WVec lists is awfully inefficient in terms of time. Modders need some automatic, simple way to do that, in my opinion, and occupied cells are the easiest approach in my book.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 1, 2017

Member

I already thought about footprints and have my own idea:

SomeBuilding:
    Health:
        Shape: ByFootprint
            Footprint: _x_ xxx axb # If missing, pull from Building
            Shapes: # each shape template operates from the center of the instancing cell
                x: Rectangle
                    TopLeft: -512, -512
                    BottomRight: 512, 512
                a: RightTriangle
                    RightCorner: 512, -512
                    Legs: -1024, 1024
                    TargetableOffset: 256, -256
                    DamageMultiplier: 50
                b: RightTriangle
                    RightCorner: -512, -512
                    Legs: -1024, 1024
                    TargetableOffset: -256, -256
                    DamageMultiplier: 50
    Building:
        ...
        Footprint: _x_ xxx =x=
        ...
Member

atlimit8 commented May 1, 2017

I already thought about footprints and have my own idea:

SomeBuilding:
    Health:
        Shape: ByFootprint
            Footprint: _x_ xxx axb # If missing, pull from Building
            Shapes: # each shape template operates from the center of the instancing cell
                x: Rectangle
                    TopLeft: -512, -512
                    BottomRight: 512, 512
                a: RightTriangle
                    RightCorner: 512, -512
                    Legs: -1024, 1024
                    TargetableOffset: 256, -256
                    DamageMultiplier: 50
                b: RightTriangle
                    RightCorner: -512, -512
                    Legs: -1024, 1024
                    TargetableOffset: -256, -256
                    DamageMultiplier: 50
    Building:
        ...
        Footprint: _x_ xxx =x=
        ...
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 1, 2017

Member

We need to be really careful about having a whole bunch of shapes per object, because we have to enumerate them every time we calculate damage etc. If we have a normal square footprint (3x3 etc) then we really shouldn't be using more than a single square shape.

Member

pchote commented May 1, 2017

We need to be really careful about having a whole bunch of shapes per object, because we have to enumerate them every time we calculate damage etc. If we have a normal square footprint (3x3 etc) then we really shouldn't be using more than a single square shape.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 1, 2017

Member

@pchote, that can be fudged:

SomeBuilding:
    Health:
        Shape: ByFootprint
            Footprint: _x_ _c_ axb # If missing, pull from Building
            Shapes: # each shape template operates from the center of the instancing cell
                x: Rectangle
                    TopLeft: -512, -512
                    BottomRight: 512, 512
                c: Rectangle
                    DamagePerCell: true
                    TargetablePerCell: true
                    TopLeft: -1c512, -512
                    BottomRight: 1c512, 512
                a: RightTriangle
                    RightCorner: 512, -512
                    Legs: -1c, 1c
                    TargetableOffsets: 256, -256
                    DamageMultiplier: 50
                b: RightTriangle
                    RightCorner: -512, -512
                    Legs: -1c, 1c
                    TargetableOffsets: -256, -256
                    DamageMultiplier: 50
    Building:
        ...
        Footprint: _x_ xxx =x=
        ...
OtherBuilding:
    Health:
        Shape: ByFootprint
            Footprint: ___ _a_ _b_ # If missing, pull from Building
                a: Rectangle
                    DamagePerCell: true
                    TargetablePerCell: true
                    TopLeft: -1c512, -1c512
                    BottomRight: 1536, 512
                b: Rectangle
                    DamagePerCell: true
                    TargetablePerCell: true
                    TopLeft: -1c512, -512
                    BottomRight: 1c512, 0
                    DamageMultiplier: 50
    Building:
        ...
        Footprint: xxx xxx ===
        ...
Square:
    Health:
        Shape: Rectangle
           DamagePerCell: true
           TargetablePerCell: true
           TopLeft: -1c512, -1c512
           BottomRight: 1c512, 1c512
    Building:
        ...
        Footprint: xxx xxx xxx
        ...
Member

atlimit8 commented May 1, 2017

@pchote, that can be fudged:

SomeBuilding:
    Health:
        Shape: ByFootprint
            Footprint: _x_ _c_ axb # If missing, pull from Building
            Shapes: # each shape template operates from the center of the instancing cell
                x: Rectangle
                    TopLeft: -512, -512
                    BottomRight: 512, 512
                c: Rectangle
                    DamagePerCell: true
                    TargetablePerCell: true
                    TopLeft: -1c512, -512
                    BottomRight: 1c512, 512
                a: RightTriangle
                    RightCorner: 512, -512
                    Legs: -1c, 1c
                    TargetableOffsets: 256, -256
                    DamageMultiplier: 50
                b: RightTriangle
                    RightCorner: -512, -512
                    Legs: -1c, 1c
                    TargetableOffsets: -256, -256
                    DamageMultiplier: 50
    Building:
        ...
        Footprint: _x_ xxx =x=
        ...
OtherBuilding:
    Health:
        Shape: ByFootprint
            Footprint: ___ _a_ _b_ # If missing, pull from Building
                a: Rectangle
                    DamagePerCell: true
                    TargetablePerCell: true
                    TopLeft: -1c512, -1c512
                    BottomRight: 1536, 512
                b: Rectangle
                    DamagePerCell: true
                    TargetablePerCell: true
                    TopLeft: -1c512, -512
                    BottomRight: 1c512, 0
                    DamageMultiplier: 50
    Building:
        ...
        Footprint: xxx xxx ===
        ...
Square:
    Health:
        Shape: Rectangle
           DamagePerCell: true
           TargetablePerCell: true
           TopLeft: -1c512, -1c512
           BottomRight: 1c512, 1c512
    Building:
        ...
        Footprint: xxx xxx xxx
        ...
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 1, 2017

Contributor

Updated.

Adressed the comment on InstantHit, added a debug overlay and for now (temporarily or not) an additional trait for custom positions and a test-case (D2k Repair Pad).

We need to be really careful about having a whole bunch of shapes per object, because we have to enumerate them every time we calculate damage etc. If we have a normal square footprint then we really shouldn't be using more than a single square shape.

Yeah, I guess that's ultimately the better approach for most buildings.

Now that I think about it, with custom TargetablePositions we could technically set up shapes that are smaller than the footprint and just match the visuals of the building.
In other words, as long as we set the TargetablePositions accordingly, we possibly don't even need to use footprint-based/polygon shapes for the shipping mods or TS (which doesn't even have any asymmetric footprints).

Contributor

reaperrr commented May 1, 2017

Updated.

Adressed the comment on InstantHit, added a debug overlay and for now (temporarily or not) an additional trait for custom positions and a test-case (D2k Repair Pad).

We need to be really careful about having a whole bunch of shapes per object, because we have to enumerate them every time we calculate damage etc. If we have a normal square footprint then we really shouldn't be using more than a single square shape.

Yeah, I guess that's ultimately the better approach for most buildings.

Now that I think about it, with custom TargetablePositions we could technically set up shapes that are smaller than the footprint and just match the visuals of the building.
In other words, as long as we set the TargetablePositions accordingly, we possibly don't even need to use footprint-based/polygon shapes for the shipping mods or TS (which doesn't even have any asymmetric footprints).

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 1, 2017

Member

I think a mix of multi-cell support for hit shapes plus ByFootprint provides the best compromise between performance and modder accessibility.

Member

atlimit8 commented May 1, 2017

I think a mix of multi-cell support for hit shapes plus ByFootprint provides the best compromise between performance and modder accessibility.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 2, 2017

Contributor

Updated as discussed on IRC for now.
The last commit shows that we need #12609 sooner rather than later, although we could temporarily work around that by increasing the default values until we remove the Target(Extra)SearchRadius fields completely.

Contributor

reaperrr commented May 2, 2017

Updated as discussed on IRC for now.
The last commit shows that we need #12609 sooner rather than later, although we could temporarily work around that by increasing the default values until we remove the Target(Extra)SearchRadius fields completely.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR May 2, 2017

Member

Looks like units look at center of the target regardless of where they are attacking instead of the closest target position.
turret targeting

Light factory is targeted from its exit site in original. But looks like it is missing a target position here.
lfac

Top parts of IX Res. Center, Hi-Tech Fac and Heavy Fac. are also targeted in original. I think it is missing in this PR as it is not possible to have those wierdly shaped hit shape is not possible yet.

These doesn't look matching.
hitshapes

Ornithopter is still powerful too powerful.

Member

MustaphaTR commented May 2, 2017

Looks like units look at center of the target regardless of where they are attacking instead of the closest target position.
turret targeting

Light factory is targeted from its exit site in original. But looks like it is missing a target position here.
lfac

Top parts of IX Res. Center, Hi-Tech Fac and Heavy Fac. are also targeted in original. I think it is missing in this PR as it is not possible to have those wierdly shaped hit shape is not possible yet.

These doesn't look matching.
hitshapes

Ornithopter is still powerful too powerful.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 2, 2017

Member

These doesn't look matching.

An easy fix for these and the RA conyard would be to replace the RotateToIsometry flag with a general WAngle rotation angle, and then rotate the shapes to more closely match the structures.

Member

pchote commented May 2, 2017

These doesn't look matching.

An easy fix for these and the RA conyard would be to replace the RotateToIsometry flag with a general WAngle rotation angle, and then rotate the shapes to more closely match the structures.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 2, 2017

Contributor

Looks like units look at center of the target regardless of where they are attacking instead of the closest target position.

Ah, forgot to fix that.

Light factory is targeted from its exit site in original. But looks like it is missing a target position here.

The original behavior looks bad, though. I'll add a target position on the edge of the exit (at the border of the visible sprite), that should be a good compromise.

Top parts of IX Res. Center, Hi-Tech Fac and Heavy Fac. are also targeted in original. I think it is missing in this PR as it is not possible to have those wierdly shaped hit shape is not possible yet.

Yes, that was intentional. In my opinion this is good enough until someone like @atlimit8 implements RectangleUnion or Polygon hitshapes, then we can adapt those edge cases. I wanted to avoid using my hacky per-cell hit-shapes, as that hack will eventually go away anyway.

These doesn't look matching.

I know, I took visual appearance of the buildings into account a bit.

An easy fix for these and the RA conyard would be to replace the RotateToIsometry flag with a general WAngle rotation angle, and then rotate the shapes to more closely match the structures.

I don't think that will help on the Ix Research Center, but for the heavy factory and maybe high tech factory it might work, yeah.
Edit: Although rotating hit shapes would a) match the original even less, and b) could lead to overlapping hitshapes which would make it more likely that two buildings receive full damage at once when they're place right beside each other.

Contributor

reaperrr commented May 2, 2017

Looks like units look at center of the target regardless of where they are attacking instead of the closest target position.

Ah, forgot to fix that.

Light factory is targeted from its exit site in original. But looks like it is missing a target position here.

The original behavior looks bad, though. I'll add a target position on the edge of the exit (at the border of the visible sprite), that should be a good compromise.

Top parts of IX Res. Center, Hi-Tech Fac and Heavy Fac. are also targeted in original. I think it is missing in this PR as it is not possible to have those wierdly shaped hit shape is not possible yet.

Yes, that was intentional. In my opinion this is good enough until someone like @atlimit8 implements RectangleUnion or Polygon hitshapes, then we can adapt those edge cases. I wanted to avoid using my hacky per-cell hit-shapes, as that hack will eventually go away anyway.

These doesn't look matching.

I know, I took visual appearance of the buildings into account a bit.

An easy fix for these and the RA conyard would be to replace the RotateToIsometry flag with a general WAngle rotation angle, and then rotate the shapes to more closely match the structures.

I don't think that will help on the Ix Research Center, but for the heavy factory and maybe high tech factory it might work, yeah.
Edit: Although rotating hit shapes would a) match the original even less, and b) could lead to overlapping hitshapes which would make it more likely that two buildings receive full damage at once when they're place right beside each other.

@reaperrr reaperrr changed the title from Fix TargetPosition targeting and give buildings footprint-based hit-shapes to Fix TargetPosition targeting and give D2k buildings better hit-shapes May 4, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 4, 2017

Contributor

Depends on #13236 (to revert the custom radius changes) and #13237 (to rotate shapes).

Contributor

reaperrr commented May 4, 2017

Depends on #13236 (to revert the custom radius changes) and #13237 (to rotate shapes).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 15, 2017

Member

I'm not sure if it is a good idea to give HFac 3x3 hitshape while Hi-Tech Fac and IX RC keep 3x2.

Somewhere earlier in the discussion it was pointed out that the original d2k uses full cells for its targeting and hitshapes, like this PR is doing. It should be easy to check if it indeed used 3x3 for HFac and 3x2 for the others.

Member

pchote commented May 15, 2017

I'm not sure if it is a good idea to give HFac 3x3 hitshape while Hi-Tech Fac and IX RC keep 3x2.

Somewhere earlier in the discussion it was pointed out that the original d2k uses full cells for its targeting and hitshapes, like this PR is doing. It should be easy to check if it indeed used 3x3 for HFac and 3x2 for the others.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 15, 2017

Contributor

@pchote: All 3 'hit-shapes' were occupancy-based in the original, so _x_ xxx xxx is what they'd need in theory.

I'm generally uncomfortable with incuding unoccupied cells in the hit-shape, in my opinion 'missing' the top cell is the lesser evil.

Contributor

reaperrr commented May 15, 2017

@pchote: All 3 'hit-shapes' were occupancy-based in the original, so _x_ xxx xxx is what they'd need in theory.

I'm generally uncomfortable with incuding unoccupied cells in the hit-shape, in my opinion 'missing' the top cell is the lesser evil.

@reaperrr reaperrr referenced this pull request May 17, 2017

Merged

Add HitShape trait #13314

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 17, 2017

Contributor

Split the easier part of the PR to #13314 (with some updates), to make this here a bit easier to review later.

Contributor

reaperrr commented May 17, 2017

Split the easier part of the PR to #13314 (with some updates), to make this here a bit easier to review later.

@reaperrr reaperrr modified the milestones: Playtest featuring updated HitShapes, Next release Jun 1, 2017

reaperrr added some commits Apr 21, 2017

Fix armaments/projectiles to aim at closest Target.Positions
Instead of CenterPosition.
TargetablePositions were already used for targeting/attack decisions, but not armaments/projectiles.
Move down D2k High-tech factory sprite
This better aligns its visuals with the footprint, hit-shape and targetable positions.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 3, 2017

Contributor

Rebased and updated.

All dependencies are merged and this contains the last two code commits, after that setting up hit-shapes and targetable offsets for the other 3 mods is all that remains.

Edit: Note to any reviewers, this will regress the other mods until their respective hit-shape PRs are merged (weapons will target offsets that might be outside their standard circle shapes), so don't bother testing anything else than D2k here.

Contributor

reaperrr commented Jun 3, 2017

Rebased and updated.

All dependencies are merged and this contains the last two code commits, after that setting up hit-shapes and targetable offsets for the other 3 mods is all that remains.

Edit: Note to any reviewers, this will regress the other mods until their respective hit-shape PRs are merged (weapons will target offsets that might be outside their standard circle shapes), so don't bother testing anything else than D2k here.

@pchote

pchote approved these changes Jun 4, 2017

Just one nit/question, otherwise looks great 👍

Show outdated Hide outdated mods/d2k/rules/structures.yaml
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 4, 2017

Contributor

Updated.

Contributor

reaperrr commented Jun 4, 2017

Updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 4, 2017

Contributor

Updated.

Contributor

reaperrr commented Jun 4, 2017

Updated.

@obrakmann

lgtm, 👍

@obrakmann obrakmann merged commit 3507ad2 into OpenRA:bleed Jun 5, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@obrakmann
Contributor

obrakmann commented Jun 5, 2017

@reaperrr reaperrr deleted the reaperrr:TargetablePositions branch Jul 23, 2017

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