Fix terrain positions for targets not being serialized for Orders

[RoosterDragon]

Takeover of #20216, see for more details.

Ensure we serialize terrain positions for a target correctly. Resolves my comment from original PR by using -1 as a magic number rather than 0 which conflicts with a 0-length array.

[anvilvapre]

untested.

this couldn't break i.e. with FrameEndTask being executed?

[anvilvapre]

what would the average length of the array be? soldiers one? larger units multiple? buildings more?

[abcdefg30]

what would the average length of the array be? soldiers one? larger units multiple? buildings more?

See #20216 (comment), the average length will be one as we usually only send terrainCenterPosition (which is now the -1 case). There is currently no case which is sending more over the network, but since it is technically possible we should handle it correctly instead of causing hard to track down desyncs.

[RoosterDragon]

this couldn't break i.e. with FrameEndTask being executed?

No I don't believe the use of (or lack of) FrameEndTask would affect this scenario.

[PunkPun]

we should probably create a testcase scenario. Or perhaps add a unit test?

[RoosterDragon]

we should probably create a testcase scenario. Or perhaps add a unit test?

Can't easily add a unit test as I'd need a world/actor or some other hard to create object. But if we add an extra method for testing then the following test will fail on bleed, but pass with the PR changes. Since Target.FromTestPositions is a hack I won't commit it though. This test did uncover a bug in the changes, so good shout! Have resolved the bug.

// OrderTest.cs
[TestCase(TestName = "Order target persists over serialization")]
public void SerializeTerrain()
{
	var o = new Order("Test", null, Target.FromTestPositions(new WPos(1, 2, 3), new[] { new WPos(4, 5, 6), new WPos(7, 8, 9) }), false);
	var o2 = Order.Deserialize(null, new BinaryReader(new MemoryStream(o.Serialize())));
	Assert.That(o2.Target.Positions, Is.EqualTo(o.Target.Positions));
}

//Order.cs
public static Target FromTestPositions(WPos centerPosition, WPos[] terrainPositions)
{
	return new Target(centerPosition, terrainPositions);
}

[pchote]

Since Target.FromTestPositions is a hack

Could this be done with reflection as a more acceptable (IMO) level of hackery?

[RoosterDragon]

Could this be done with reflection as a more acceptable (IMO) level of hackery?

This one scenario is an edge case we barely care about.

The scenarios we really do care about are the things used all the time like actors, frozen actors, cells - i.e. all the Target.From* methods that currently lack tests. They would be much higher value but would require somehow making the code testable and that's a whole different can of worms. But if somebody solved that then this edge case can be tested without hacks.

It'd be nice to merge this as a small fix as-is since @abcdefg30 went to the effort of authoring it - but I'd prefer to defer dealing with the worms until we get more value out of the effort for it.

[PunkPun]

Changeog