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

Added destructible concrete for Dune 2000 #15610

Merged
merged 1 commit into from Sep 15, 2018

Conversation

Projects
None yet
5 participants
@Mailaender
Copy link
Member

Mailaender commented Sep 13, 2018

This adds a simple model to remove player built concrete. Closes #9410

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 13, 2018

I'm pretty sure that the original game had a damage threshold before concrete was destroyed. One-shot removing it seems really overkill IMO.

public void RemoveTile(CPos cell, TerrainTile tile)
{
map.CustomTerrain[cell] = byte.MaxValue;
ChangeTile(cell, tile);

This comment has been minimized.

@pchote

pchote Sep 13, 2018

Member

Removing the tile should fall back to the default of not rendering anything!

This comment has been minimized.

@Mailaender

Mailaender Sep 13, 2018

Member

I am not sure how else to remove it.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 13, 2018

You are right. I added a damage threshold.

public class DamagesConcreteWarhead : Warhead
{
[Desc("The rock tile ID to fall back to.")]
public readonly ushort Template = 266;

This comment has been minimized.

@pchote

pchote Sep 13, 2018

Member

The last revision hid the previous comment, so repeating here: removing the tile needs to reset the TerrainLayer to its default state of rendering nothing. Drawing a duplicate tile is bogus.

This comment has been minimized.

@Mailaender

Mailaender Sep 13, 2018

Member

Fixed that as well. Now it preserves the smudges.

@Mailaender Mailaender force-pushed the Mailaender:destroy-concrete branch from d6eba3b to 8a0aa18 Sep 13, 2018

@@ -21,6 +21,8 @@ public class BuildableTerrainLayerInfo : ITraitInfo
[Desc("Palette to render the layer sprites in.")]
public readonly string Palette = TileSet.TerrainPaletteInternalName;

public int MaxStrength = 1024;

This comment has been minimized.

@reaperrr

reaperrr Sep 13, 2018

Contributor

Why not readonly?
Having adescription would also be nicer for modders.


// Terrain tiles define their origin at the topleft
var s = theater.TileSprite(tile);
dirty[cell] = new Sprite(s.Sheet, s.Bounds, s.ZRamp, float2.Zero, s.Channel, s.BlendMode);
}

public void HitTile(CPos cell, int damage)
{
strength[cell] = strength[cell] - damage;

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 13, 2018

Member

What happens when strength[cell] == 0 is true already (the cell doesn't have concrete on it)? I'm not sure how bad counting towards "negative infinity" is in practice, but it might be better to add a safe-guard just in case.

Edit: Actually, this is not really bad, as RemoveTile would be called and the strength would be reset to 0 again regardless.

@Mailaender Mailaender changed the title Added a DamagesConcreteWarhead Added destructible concrete for Dune 2000 Sep 14, 2018

@Mailaender Mailaender force-pushed the Mailaender:destroy-concrete branch from 8a0aa18 to 4c08d92 Sep 14, 2018

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 14, 2018

Updated.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Shouldn't Missile Tanks, Missile Quads and Troopers (rocket infantry) remove concrete as well? (They do leave quite big craters.) The Death Hand Missile should do some damages as well, imho. (Maybe even the air strike.)

@MustaphaTR
Copy link
Member

MustaphaTR left a comment

  • Concrete seems like it had a health of 500 in original. actually 900 health
  • It has armor of Concrete, but it seems like everything has either %100 or %0, so not sure if it is necessary. actually Armor None
  • Original game didn't have a special logic for concrete destruction, so all weapons should deal their normal damage to concrete too.
  • Sonic weapons dealt damage too, which is done via projectile on OpenRA. We need a logic for that. I think i know something wrong about how sonic tank works.
  • Didn't test but i don't see a logic to make layers with building on not take damage. Seems like they don't take damage. Guess i missed something.
  • Destructibility isn't giving us much. We still need it to give buildable area for one who placed it on it, which i think is required to call the logic complete. Doesn't have to be in this PR tho.
@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 14, 2018

Concrete seems like it had a health of 500 in original.

The cannons have a much larger damage:

	Warhead@1Dam: SpreadDamage
		Damage: 2700

so that would give us one-shot concrete destruction, which again @pchote didn't like.

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Sep 14, 2018

That would be 5000 here since we did 10x damage/health in last release, which would result in 2 shot.

@Mailaender Mailaender force-pushed the Mailaender:destroy-concrete branch from 4c08d92 to b7e9ad2 Sep 14, 2018

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 14, 2018

Updated so that all actors damage concrete.

@Mailaender Mailaender force-pushed the Mailaender:destroy-concrete branch 2 times, most recently from 639d717 to a3e0ba2 Sep 14, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Code wise fine, and works ingame. 👍 /

(Consider this 👍 still valid after balancing/changing the damage values.)

@MustaphaTR
Copy link
Member

MustaphaTR left a comment

My tests has shown Concrete has None armor. So damages should be adjusted as if it has this armor class. It can be done for another PR for future balancing as abcdefg30 says, but imo should be better done here.

@Mailaender Mailaender force-pushed the Mailaender:destroy-concrete branch from a3e0ba2 to 9d5a718 Sep 15, 2018

Recalculcated everything with your recent findings.

@Mailaender Mailaender force-pushed the Mailaender:destroy-concrete branch from 9d5a718 to dce8f0a Sep 15, 2018

@abcdefg30 abcdefg30 merged commit 399e451 into OpenRA:bleed Sep 15, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 15, 2018

@Mailaender Mailaender deleted the Mailaender:destroy-concrete branch Sep 15, 2018

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