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

Moving WormManager from d2k to Common as ActorSpawnManager #14349

Merged
merged 2 commits into from Mar 8, 2018

Conversation

Projects
None yet
@CDVoidwalker
Copy link
Contributor

CDVoidwalker commented Nov 12, 2017

Works just like old WormManager with addition of being conditional and having Types.

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Nov 13, 2017

Checking for explicit interface violations...
ActorSpawnManager must explicitly implement the interface member ITick.Tick
Explicit interface violations: 1
make: *** [check] Error 1

Also commits should be squashed.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Nov 13, 2017

OpenRA.Mods.Common/Traits/ActorSpawner.cs:L25: [SA1513] Statements or elements wrapped in curly brackets must be followed by a blank line.
OpenRA.Mods.Common/Traits/ActorSpawner.cs:L14: [SA1208] System using directives must be placed before all other using directives.
OpenRA.Mods.Common/Traits/World/ActorSpawnManager.cs:L21: [SA1001] Invalid spacing around the comma.
OpenRA.Mods.Common/Traits/World/ActorSpawnManager.cs:L34: [SA1012] Invalid spacing around the opening curly bracket.
OpenRA.Mods.Common/Traits/World/ActorSpawnManager.cs:L34: [SA1013] Invalid spacing around the closing curly bracket.

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:actorspawner branch from 3247e83 to c45050d Nov 13, 2017

@Unit158

This comment has been minimized.

Copy link
Contributor

Unit158 commented Nov 14, 2017

There's a binary file that shouldn't have been included.

public readonly string[] ActorSignature = { };

public readonly string ActorSignNotification = "";
public readonly string ActorOwnerPlayer = "Creeps";

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

I'd argue that this would be just as clear as "Owner:"

This comment has been minimized.

@CDVoidwalker

CDVoidwalker Nov 14, 2017

Author Contributor

That was directly copied from WormManager, as with all the comments

public readonly string ActorSignNotification = "";
public readonly string ActorOwnerPlayer = "Creeps";

[Desc("Type of ActorSpawner which it connects with")]

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

"which it connects with" -> "with which it connects"


bool enabled;
int spawnCountdown;
int actorPresent;

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

actorsPresent


do
{
// Always spawn at least one actor, plus however many

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

"Always spawn at least one actor, plus however many needed to reach the minimum"

if (!spawnPointActors.Value.Any())
return;

// Apparently someone doesn't want actors or the maximum number of actors has been reached

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

I'd argue the next line doesn't warrant a comment.

[Desc("An actor with this trait indicates a valid spawn point for actors.")]
public class ActorSpawnerInfo : ConditionalTraitInfo
{
[Desc("Type of ActorSpawner which it connects with")]

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

"ActorSpawner which it connects with" -> "ActorSpawnManager with which it connects"


[FieldLoader.Require]
[Desc("Name of the actor that will be spawned.")]
public readonly string[] ActorSignature = { };

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

ActorSignature -> Actors

[Desc("Name of the actor that will be spawned.")]
public readonly string[] ActorSignature = { };

public readonly string ActorSignNotification = "";

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

SpawnNotification

This comment has been minimized.

@penev92

penev92 Nov 14, 2017

Member

No, that was actually public readonly string WormSignNotification = "WormSign";.
So this one probably needs some more work.

@Unit158

This comment has been minimized.

Copy link
Contributor

Unit158 commented Nov 14, 2017

I haven't tested yet, but the code looks OK, minus some style/naming/language improvements.

return spawnPoint.CenterPosition;
}

Actor GetRandomSpawnPoint(Actor self)

This comment has been minimized.

@Unit158

Unit158 Nov 14, 2017

Contributor

I'd argue this should only pass the shared RNG.

if (info.Maximum < 1 || actorsPresent >= info.Maximum)
return;

if (--spawnCountdown > 0 && actorsPresent >= info.Minimum)

This comment has been minimized.

@MunWolf

MunWolf Nov 14, 2017

Contributor

This might not ever happen but there could be an "underflow" here if the second condition never fires.
EDIT: Since it never happened in the old code I guess it is fine?

This comment has been minimized.

@CDVoidwalker

CDVoidwalker Nov 14, 2017

Author Contributor

There's no real case when it would happen, as adding actors is after this check

This comment has been minimized.

@MunWolf

MunWolf Mar 5, 2018

Contributor

I meant that --spawnCountdown might eventually underflow to a really big value after 2147483648 ticks... which will probably never happen since it will take way too long.

This comment has been minimized.

@reaperrr

reaperrr Mar 5, 2018

Contributor

@MunWolf Unless I'm miscalculating, we're talking about nearly 12000 hours at maximum game speed (50 ticks/sec).

I dare say whoever manages to set things up weirdly enough to trigger this, deserves the crash ;)

@MunWolf

This comment has been minimized.

Copy link
Contributor

MunWolf commented Nov 14, 2017

This still needs an upgrade rule for the name change.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 17, 2017

Can you please also squash the fixup commits into the main one?

@pchote
Copy link
Member

pchote left a comment

I haven't tested functionality, but I have left a few style nits.
As @MunWolf mentioned this will also need an upgrade rule to manage the rename.

@@ -837,22 +839,14 @@
</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<Target Name="AfterBuild">
<MakeDir Directories="$(SolutionDir)mods/common/"/>

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

Please unstage these unrelated project file formatting changes.


namespace OpenRA.Mods.Common.Traits
{
[Desc("An actor with this trait indicates a valid spawn point for actors.")]

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

Can you please have this mention that it is specifically for ActorSpawnManager?

public class ActorSpawner : ConditionalTrait<ActorSpawnerInfo>
{
public ActorSpawner(ActorSpawnerInfo info) : base(info)
{ }

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

Our code style conventions for cases like this is

public ActorSpawner(ActorSpawnerInfo info)
	: base(info) { }

namespace OpenRA.Mods.Common.Traits
{
[Desc("Controls the spawning of actors. Attach this to the world actor.")]

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

Can you please change this to something like "Controls spawning of map creep actors"? There are other types of actor spawning in the game, so this could otherwise be confusing.

This comment has been minimized.

@CDVoidwalker

CDVoidwalker Nov 19, 2017

Author Contributor

Yeah, but also those actors aren't creep only, depends on what is set in Owner of the manager, so "Controls spawning of the map actors"?

This comment has been minimized.

@pchote

pchote Nov 21, 2017

Member

I would go with "Controls the spawning of specified actor types" to keep the distinction vs e.g. SpawnMpUnits clear.

[Desc("Name of the actor that will be spawned.")]
public readonly string[] Actors = { };

public readonly string ActorSignNotification = "";

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

This isn't used, so please remove it.


[FieldLoader.Require]
[Desc("Name of the actor that will be spawned.")]
public readonly string[] Actors = { };

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

These are actor references, so please attach the [ActorReference] annotation.

public readonly int SpawnInterval = 6000;

[FieldLoader.Require]
[Desc("Name of the actor that will be spawned.")]

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

Please mention that the spawned actor types are selected randomly from this list - otherwise it could be interpreted as spawning them all at once.


spawnCountdown = info.SpawnInterval;

var actorLocations = new List<WPos>();

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

This isn't used for anything?

@@ -121,22 +119,14 @@
</ItemGroup>
<Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
<Target Name="AfterBuild">
<MakeDir Directories="$(SolutionDir)mods/d2k/"/>

This comment has been minimized.

@pchote

pchote Nov 17, 2017

Member

Please unstage unrelated project file changes.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 21, 2017

It would be good to keep this moving and to get it merged before it loses momentum.

Do you need any help with this?

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented Nov 22, 2017

Will try to get it done for tomorrow

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:actorspawner branch from ecbc072 to 4325085 Nov 22, 2017

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Nov 23, 2017

OpenRA.Mods.Common/Traits/ActorSpawner.cs:L29: [SP2000] Invalid spacing at the end of the line.

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented Nov 23, 2017

Turns out I won't really be at home today to do so, I'll push only travis fix

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:actorspawner branch 3 times, most recently from 5f8a855 to c873630 Nov 24, 2017

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented Nov 25, 2017

needs review

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Dec 17, 2017

Needs rebase.


Actor GetRandomSpawnPoint(Support.MersenneTwister random)
{
return spawnPointActors.Random(random).Actor;

This comment has been minimized.

@GraionDilach

GraionDilach Dec 28, 2017

Contributor

ActorSpawner traits are also conditional, but this is ignored in this current revision. IMO it'd make the most sense to filter the the enabled traits here.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Dec 28, 2017

Now that I look at it again, I'm actually not certain how much sense it makes to cache the suitable actor spawners on creation. Not doing that would allow one to enable map creeps midgame based on some initial condition.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 29, 2017

Yes, that makes sense. Certain mods or minigames might want to use player-built actors as spawn points.

@Arular101

This comment has been minimized.

Copy link
Contributor

Arular101 commented Jan 13, 2018

Please don't forget to update the copyright notice year.

@reaperrr reaperrr force-pushed the CDVoidwalker:actorspawner branch 4 times, most recently from eb90571 to e3d09f9 Feb 25, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Feb 25, 2018

Adressed all remaining style nits, replaced caching-at-creation with evaluation right before spawn attempt, rebased.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Feb 25, 2018

OpenRA.Mods.Common/Traits/World/ActorSpawnManager.cs(48,29): error CS0169: Warning as Error: The private field `OpenRA.Mods.Common.Traits.ActorSpawnManager.spawnPointActors' is never used

@reaperrr reaperrr force-pushed the CDVoidwalker:actorspawner branch from e3d09f9 to ac01bf9 Feb 26, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Feb 26, 2018

Fixed.


var spawnPoint = GetRandomSpawnPoint(self.World, self.World.SharedRandom);

if (!enabled)

This comment has been minimized.

@GraionDilach

GraionDilach Feb 26, 2018

Contributor

Does caching the nullcheck to a boolean for this usage worth it?

Generalize WormManager into ActorSpawnManager.
Added support of multiple actors, conditions and types.

@reaperrr reaperrr force-pushed the CDVoidwalker:actorspawner branch from ac01bf9 to cf14e65 Mar 4, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Mar 4, 2018

Updated.

Does caching the nullcheck to a boolean for this usage worth it?

No, but I also forgot to keep the MapCreeps.Enabled check, so now I'm using enabled to cache that at creation but use spawnPoint == null to abort later if there's no enabled spawnpoint.

Don't cache ActorSpawners at creation
Re-evaluate before every spawn attempt instead.
Also accounts for ActorSpawner being conditional now.

While a bit more costly in terms of performance, this allows to create spawns mid-game.

@reaperrr reaperrr force-pushed the CDVoidwalker:actorspawner branch from cf14e65 to d8b76aa Mar 8, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Mar 8, 2018

I've proof-read and tested my own updates. If it turns out I overlooked anything, I'll take responsibility and file a follow-up.

@reaperrr reaperrr merged commit 55b11d1 into OpenRA:bleed Mar 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -1635,6 +1635,21 @@ internal static void UpgradeActorRules(ModData modData, int engineVersion, ref L
}
}

if (engineVersion < 20180225)

This comment has been minimized.

@GraionDilach

GraionDilach Mar 8, 2018

Contributor

Needs an update on this due to new release (?).

This comment has been minimized.

@reaperrr

reaperrr Mar 8, 2018

Contributor

Yeah, but it's not the only case, I think. I'll fix it either way.

@KOYK

This comment has been minimized.

Copy link
Contributor

KOYK commented Mar 21, 2018

Thank you @CDVoidwalker for this! And thank you again @reaperrr !!

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.