Implement Carryall edge spawn, harvester delivery by carryall and harvester insurance for D2k #7664

penev92 · 2015-03-14T14:40:42Z

First off, this got a bit bigger than expected. So sorry, reviewers.

This is more or less a direct successor to #7300.

I spent quite a while organizing the changes into propper self-contained commits, so I strongly suggest reviewing, and even testing, by commits.
Despite the nice commit messages, I'm going to sum this up:
1. Make built carryalls spawn at the closest map edge cell instead of spawning at the High Tech facility
2. Do some refactoring to enable 3. and 4.
3. Enable FreeActors to be delivered from outside the map via transport (use case are the D2k harvesters, transported by carryalls; this might also be useful in some missions (TS?), but that's not important at the moment). This uses the closest map edge cell by default, but can be set to spawn at a specific location in the YAML file.
4. Add harvester insurance by attaching a new HarvesterInsurance trait to the player actor and making worms call that if they eat a harvester.

Side bonus: the shellmap got a bit more interesting due to the harvester's new harvesting location (the first time you watch it, at least :D ).

phrohdoh · 2015-03-14T15:25:11Z

What does the 'insurance' do? An empty description helps no one.

penev92 · 2015-03-14T15:29:22Z

Added a trait description to HarvesterInsurance.

pchote · 2015-03-15T00:04:17Z

Did the insurance only apply to worms in the original, or to general harvester death?

penev92 · 2015-03-15T00:43:17Z

The insurance in the original was only a per-mission feature (a "you may need a new harvester in this mission, just so we don't make the mission too hard" thing) and it didn't care who killed the harvester.

Our worms, however, are much more pesky than the ones in the original and our AI is really unequipped to deal with them, so I thought I'd make it permanent.
I also figured:
1. Insurance regardless of who killed your harvester might have more serious gameplay implications.
2. This way is much easier and also much nicer (code- and performance-wise) to implement.

Micr0Bit · 2015-03-15T03:48:53Z

would be nice to have "harvester-insurance" as option ... "default = on" of course , for the bots ...
turning it off would spice up some multiplayer/tourny games (since it doesnt exist in the original multiplayer)

Mailaender · 2015-03-15T06:41:52Z

mods/d2k/maps/shellmap/shellmap.lua

-	atreides = Player.GetPlayer("Atreides")
-
-	InsertHarvester()
-	Media.PlayMusic("score")


It also plays the shellmap music.

Mailaender · 2015-03-15T06:51:34Z

This seems to work extremely well in-game.

penev92 · 2015-03-15T13:35:48Z

Fixed.

Mailaender · 2015-03-15T14:26:50Z

mods/d2k/maps/shellmap/shellmap.lua

 	Media.PlayMusic("score")
-end
+end 


Trailing space.

Mailaender · 2015-03-15T14:40:32Z

👍

reaperrr · 2015-03-15T18:56:42Z

Needs a rebase.

penev92 · 2015-03-15T20:32:48Z

Rebased.

AppVeyor seems to be failing because of some warnings (not errors!) in the OpenRA.Test project.

Mailaender · 2015-03-16T20:38:24Z

OpenRA.Mods.D2k/OpenRA.Mods.D2k.csproj

    <Compile Include="Traits\Buildings\DamagedWithoutFoundation.cs" />
    <Compile Include="Traits\Buildings\LaysTerrain.cs" />
    <Compile Include="Traits\Carryable.cs" />
+    <Compile Include="Traits\Player\HarvesterInsurance.cs" />
+    <Compile Include="Traits\Render\WithBuildingPlacedOverlay.cs" />


No, this is the real error:

CSC : error CS2001: Source file 'Traits\Render\WithBuildingPlacedOverlay.cs' could not be found

not the warnings in the test project.

Probably a rebase problem after #7570 so sorry about that.

Oops, didn't see that.

penev92 · 2015-03-18T01:56:27Z

Crash fixed.

@MicroJOo the insurance can be turned off per-map, but I have no plans for UI integration for the moment.

obrakmann · 2015-03-19T20:32:08Z

mods/d2k/rules/aircraft.yaml

@@ -1,4 +1,4 @@
-carryall.scripted:
+carryall.reinforce:


This should probably not have TargetableAircraft. carryall.infantry neither, come to think of it.

Actually they should all be targetable. The initial idea of making carryalls untargetable was questionable, because they were targetable in the original. (or was it their cargo that was targetable, I'm not sure now).
Anyway, I'll remove TargetableAircraft from all carryalls for now, but it probably should be brought back when the proper balancing is being done.

This is intentionally creating more work for the future.

I meant that only the carryall used for the harvester insurance should have TargetableAircraft removed from it, not all of the carryalls. (Changed my mind about carryall.infantry, it's better if people can defend against them). Regular Carryalls should of course be targetable.

Edit: my reasoning: if the carryall that performs the harvester insurance function is shot down, it will not trigger another harvester being sent. And the player receiving the harvester cannot even control the carryall or its flight path, so that would be a pretty shoddy gameplay experience. It would also lure rockets troopers away as the spy plane in RA did.

See I hadn't considered stances. Having "chase all the way across the map" as the default stance really sucks. We should change the default to Hold position or similar.

Anyway, it was decided not long ago (no idea by who) that we want carryalls untargetable, because they are uncontrollable. I'm fine with it being like this for the moment, until the time comes to do the proper balance. We'll be changing everything there anyway.

obrakmann · 2015-03-19T20:39:53Z

This looks fine for the most part, but those two nits (+ typos) need to be fixed.

penev92 · 2015-03-20T02:54:56Z

Fixed the problems, but I still don't know what's wrong with ChooseClosestEdgeCell() and isometric maps.

Mailaender · 2015-03-21T07:24:14Z

I still don't know what's wrong with ChooseClosestEdgeCell() and isometric maps

I suggest you leave a TODO there as we don't need to solve it now for the next release.

penev92 · 2015-03-21T07:55:15Z

Agreed. It will be quite a while before anyone may want (if anyone at all) to use this on an isometric map.
@obrakmann your suggested method produced the exact same results as mine in both D2k and TS, so I'll leave it as it was, unless someone can help with the isometric map coordinates.

pchote · 2015-03-21T10:00:55Z

I suggest you leave a TODO there as we don't need to solve it now for the next release.

This isn't really acceptable. "I don't know how to fix this" cannot be a free pass to merge incomplete code if we want to keep a maintainable project. This is how we end up with "// HACK" and "this is shit" comments scattered throughout the code.

There are two examples on how to use MPos with bounds and borders right before ChooseClosestEdgeCell in Map.cs. Please try to get a feeling for how these work, as this really isn't a hard problem. You need to:

Convert the given CPos to a MPos.
Check against the bounds to find the nearest edge (all done in map coordinates)
Convert the resulting MPos back to a CPos and return.

penev92 · 2015-03-22T14:43:50Z

Looks like this is going to wait for a decision on #7701.

pchote · 2015-03-22T14:50:21Z

OpenRA.Mods.Common/Traits/Buildings/FreeActor.cs

 		[Desc("What the unit should start doing. Warning: If this is not a harvester", "it will break if you use FindResources.")]
 		public readonly string InitialActivity = null;
-		[Desc("Offset relative to structure-center in 2D (e.g. 1, 2)")]
+
+		[Desc("Offset relative to structure-center in 2D (e.g. 1, 2).")]


This is actually relative to the top-left cell, not the center.

pchote · 2015-03-22T14:54:24Z

The two testcases break compilation under non-windows, and prevent testing the actual pr contents.

Fixup Carryable

…reorganize Carryall actor definitions

Fix FreeActorInfo fields descriptions

obrakmann · 2015-03-25T19:02:56Z

Since #7701 does not seem to be the fault of this PR, I'm going to merge it now. It looks ok code-wise, and works very nicely in-game, too. AI probably benefits the most. 👍

Implement Carryall edge spawn, harvester delivery by carryall and harvester insurance for D2k

obrakmann · 2015-03-25T19:13:27Z

Changelog

penev92 added this to the Next release milestone Mar 14, 2015

penev92 force-pushed the bleed_harvesterInsurance branch 2 times, most recently from 6fda2aa to bfb0041 Compare March 14, 2015 15:14

penev92 force-pushed the bleed_harvesterInsurance branch from bfb0041 to a9e651b Compare March 14, 2015 15:29

Mailaender reviewed Mar 15, 2015
View reviewed changes

Mailaender added the PR: Changes requested label Mar 15, 2015

penev92 force-pushed the bleed_harvesterInsurance branch from a9e651b to 23476c1 Compare March 15, 2015 13:29

penev92 removed the PR: Changes requested label Mar 15, 2015

Mailaender reviewed Mar 15, 2015
View reviewed changes

penev92 force-pushed the bleed_harvesterInsurance branch from 23476c1 to 9d909e2 Compare March 15, 2015 14:33

Mailaender added the PR: Needs +2 label Mar 15, 2015

reaperrr added the PR: Rebase me! label Mar 15, 2015

penev92 force-pushed the bleed_harvesterInsurance branch from 9d909e2 to 09c0e0c Compare March 15, 2015 20:03

penev92 removed the PR: Rebase me! label Mar 15, 2015

Mailaender reviewed Mar 16, 2015
View reviewed changes

penev92 force-pushed the bleed_harvesterInsurance branch from 09c0e0c to 8f34775 Compare March 18, 2015 01:34

obrakmann reviewed Mar 19, 2015
View reviewed changes

obrakmann added the PR: Changes requested label Mar 19, 2015

penev92 force-pushed the bleed_harvesterInsurance branch from 8f34775 to 5153302 Compare March 20, 2015 02:41

Mailaender removed the PR: Changes requested label Mar 20, 2015

obrakmann added the PR: Changes requested label Mar 20, 2015

penev92 force-pushed the bleed_harvesterInsurance branch from 5153302 to a947f03 Compare March 21, 2015 07:59

penev92 mentioned this pull request Mar 21, 2015

Possible CPos -> MPos conversion miscalculation #7701

Closed

penev92 added PR: Has dependencies and removed PR: Changes requested labels Mar 22, 2015

pchote reviewed Mar 22, 2015
View reviewed changes

penev92 added 7 commits March 22, 2015 16:58

Add ChooseClosestEdgeCell() method to the Map class

5927e10

Produce D2k carryalls at the edge of the map

f9a2378

Refactor AutoCarryall trait and rename to Carryall

4eb67b3

Fixup Carryable

Split CarryUnit into PickupUnit and DeliverUnit

7608cb9

Remove the Lua scripted harvester delivery from the D2k shellmap and …

fa6e594

…reorganize Carryall actor definitions

Implement FreeActorWithDelivery to enable off-map harvester delivery

0d05fb5

Fix FreeActorInfo fields descriptions

Add D2k harvester insurance on worm frenzy

efee359

penev92 force-pushed the bleed_harvesterInsurance branch from cdbfb38 to efee359 Compare March 22, 2015 15:01

penev92 removed the PR: Has dependencies label Mar 22, 2015

obrakmann added a commit that referenced this pull request Mar 25, 2015

Merge pull request #7664 from penev92/bleed_harvesterInsurance

6fca67e

Implement Carryall edge spawn, harvester delivery by carryall and harvester insurance for D2k

obrakmann merged commit 6fca67e into OpenRA:bleed Mar 25, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Carryall edge spawn, harvester delivery by carryall and harvester insurance for D2k #7664

Implement Carryall edge spawn, harvester delivery by carryall and harvester insurance for D2k #7664

penev92 commented Mar 14, 2015

phrohdoh commented Mar 14, 2015

penev92 commented Mar 14, 2015

pchote commented Mar 15, 2015

penev92 commented Mar 15, 2015

Micr0Bit commented Mar 15, 2015

Mailaender Mar 15, 2015

Mailaender commented Mar 15, 2015

penev92 commented Mar 15, 2015

Mailaender Mar 15, 2015

penev92 Mar 15, 2015

Mailaender commented Mar 15, 2015

reaperrr commented Mar 15, 2015

penev92 commented Mar 15, 2015

Mailaender Mar 16, 2015

Mailaender Mar 16, 2015

penev92 Mar 16, 2015

penev92 commented Mar 18, 2015

obrakmann Mar 19, 2015

penev92 Mar 20, 2015

phrohdoh Mar 20, 2015

obrakmann Mar 20, 2015

penev92 Mar 21, 2015

obrakmann commented Mar 19, 2015

penev92 commented Mar 20, 2015

Mailaender commented Mar 21, 2015

penev92 commented Mar 21, 2015

pchote commented Mar 21, 2015

penev92 commented Mar 22, 2015

pchote Mar 22, 2015

pchote commented Mar 22, 2015

obrakmann commented Mar 25, 2015

obrakmann commented Mar 25, 2015

		@@ -1,4 +1,4 @@
		carryall.scripted:
		carryall.reinforce:

Implement Carryall edge spawn, harvester delivery by carryall and harvester insurance for D2k #7664

Implement Carryall edge spawn, harvester delivery by carryall and harvester insurance for D2k #7664

Conversation

penev92 commented Mar 14, 2015

phrohdoh commented Mar 14, 2015

penev92 commented Mar 14, 2015

pchote commented Mar 15, 2015

penev92 commented Mar 15, 2015

Micr0Bit commented Mar 15, 2015

Choose a reason for hiding this comment

Mailaender commented Mar 15, 2015

penev92 commented Mar 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mailaender commented Mar 15, 2015

reaperrr commented Mar 15, 2015

penev92 commented Mar 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penev92 commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obrakmann commented Mar 19, 2015

penev92 commented Mar 20, 2015

Mailaender commented Mar 21, 2015

penev92 commented Mar 21, 2015

pchote commented Mar 21, 2015

penev92 commented Mar 22, 2015

Choose a reason for hiding this comment

pchote commented Mar 22, 2015

obrakmann commented Mar 25, 2015

obrakmann commented Mar 25, 2015