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

Added fully random spawn position option. #15137

Merged
merged 2 commits into from Jun 3, 2018

Conversation

Projects
None yet
5 participants
@CDVoidwalker
Copy link
Contributor

CDVoidwalker commented May 12, 2018

This option enables the starcraft-like behavior where spawn positions are fully random without accounting the length between players.

@ZxGanon

This comment has been minimized.

Copy link

ZxGanon commented May 12, 2018

Have been testing it on Shattered Paradise so far and it works great. It increases gameplay depth with the new feature because you gotta scout for your opponents spawning point and that actually influences strategies you choose to go for.

It also does improve other maps that are rarely used for 2v2 but now might be used for competitive 1v1. That basically happened in WC3 and Starcraft aswell.

Also random spawning points were added in the modern CnC´s aswell (C&C Generals and above).

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:randomspawnpos branch from 51a5404 to 52706a9 May 12, 2018

.OptionOrDefault("randomspawnpositions", info.CheckboxEnabled);
}
}
}

This comment has been minimized.

@GraionDilach

GraionDilach May 13, 2018

Contributor

Nit: Missing newline at the end.

This comment has been minimized.

@GraionDilach

GraionDilach May 13, 2018

Contributor

Nit fixed since.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 13, 2018

I would have gone with something like RandomizeSpawnPositionsOption instead, but that might be me alone who finds the naming bad.

@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@abcdefg30

abcdefg30 May 13, 2018

Member

Not sure if we want that change. It'd probably better if you could drop it.

This comment has been minimized.

@pchote

pchote May 13, 2018

Member

This will be the byte order mark mentioned in the coding standard.

This comment has been minimized.

@CDVoidwalker

CDVoidwalker May 13, 2018

Author Contributor

it was done automatically by visual studio, will revert it then

@@ -89,7 +89,8 @@ static CPos ChooseSpawnPoint(World world, List<CPos> available, List<CPos> taken
if (available.Count == 0)
throw new InvalidOperationException("No free spawnpoint.");

var n = taken.Count == 0
var fullyRandomSpawnPos = world.WorldActor.TraitOrDefault<SpawnPointOption>().Enabled;

This comment has been minimized.

@abcdefg30

abcdefg30 May 13, 2018

Member

This doesn't want to be .TraitOrDefault<T>.Enabled. Use either TraitOrDefault<T> combined with a null check (I suppose prefered in thise case) or just use Trait<T>.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 13, 2018

There are some conceptual points that i may want to raise here, so please dont merge before I have a chance to give a review. I am mostly out of contact and time until I return home next weekend.

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:randomspawnpos branch 2 times, most recently from 023abab to f9884ed May 13, 2018

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented May 13, 2018

Applied review notes

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:randomspawnpos branch from f9884ed to e656143 May 13, 2018

@@ -89,7 +89,10 @@ static CPos ChooseSpawnPoint(World world, List<CPos> available, List<CPos> taken
if (available.Count == 0)
throw new InvalidOperationException("No free spawnpoint.");

var n = taken.Count == 0
var spawnpointoptions = world.WorldActor.TraitOrDefault<SpawnPointOption>();
var fullyRandomSpawnPos = spawnpointoptions != null ? spawnpointoptions.Enabled : false;

This comment has been minimized.

@abcdefg30

abcdefg30 May 13, 2018

Member

var fullyRandomSpawnPos = spawnpointoptions != null && spawnpointoptions.Enabled;

You also want to have camelCase spawnPointOptions.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 20, 2018

The idea here seems reasonable, but I think the implementation and presentation need a bit of polishing.

"Random Spawn Positions" / "Toggles fully random starting positions." is not helpful for players who don't already know how the spawn randomizer works. Instead, I suggest calling this option "Separate Team Spawns" with a description "Players without assigned spawn points will start as far as possible from enemy players." (this inverts the meaning of the checkbox, so should default to enabled).

Please also move the checkbox definition to the MPStartLocations trait. It doesn't make much sense to have this as a separate trait. Finally, most of the custom/minigame maps in RA and TD will need to be updated to disable or hide this checkbox.

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented May 21, 2018

Will do today as soon as I'll get home ^^

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:randomspawnpos branch from e656143 to fb71b24 May 21, 2018

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented May 21, 2018

Applied review fixes and rebased

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:randomspawnpos branch from fb71b24 to 6c60eb5 May 21, 2018

{
public readonly WDist InitialExploreRange = WDist.FromCells(5);

[Translate]
[Desc("Descriptive label for the spawn positions checkbox in the lobby.")]
public readonly string CheckboxLabel = "Separate Team Spawns";

This comment has been minimized.

@GraionDilach

GraionDilach May 21, 2018

Contributor

Should have a prefix on all checkbox properties. SeparateSpawns mayhaps?

@@ -18,24 +18,58 @@

namespace OpenRA.Mods.Common.Traits
{
public class MPStartLocationsInfo : ITraitInfo
[Desc("Controls the 'Separate Team Spawns' checkbox in the lobby options.")]

This comment has been minimized.

@GraionDilach

GraionDilach May 21, 2018

Contributor

Allows the map to have working spawnpoints. Also controls...

void INotifyCreated.Created(Actor self)
{
SeparateTeamSpawns = self.World.LobbyInfo.GlobalSettings
.OptionOrDefault("randomspawnpositions", info.CheckboxEnabled);

This comment has been minimized.

@GraionDilach

GraionDilach May 21, 2018

Contributor

I'd rename the option string as well.

{
readonly MPStartLocationsInfo info;

public readonly Dictionary<Player, CPos> Start = new Dictionary<Player, CPos>();

public static bool SeparateTeamSpawns { get; private set; }

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Please define this as bool separateTeamSpawns;. It's not used by outside code, and shouldn't be static!

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 21, 2018

Finally, most of the custom/minigame maps in RA and TD will need to be updated to disable or hide this checkbox.

Also don't forget to adhere to this as well.

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:randomspawnpos branch 2 times, most recently from dc743dc to 60aa709 May 21, 2018

@@ -89,7 +130,7 @@ static CPos ChooseSpawnPoint(World world, List<CPos> available, List<CPos> taken
if (available.Count == 0)
throw new InvalidOperationException("No free spawnpoint.");

var n = taken.Count == 0
var n = taken.Count == 0 || !world.WorldActor.Trait<MPStartLocations>().separateTeamSpawns

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

This is already in the MPStartLocations, so || !separateTeamSpawns is sufficient.

This comment has been minimized.

@CDVoidwalker

CDVoidwalker May 21, 2018

Author Contributor

but it's in static method where bool separateTeamSpawns isn't static

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Pass it in as an option, or remove the static from the method.

public readonly bool SeparateTeamSpawnsCheckboxVisible = true;

[Desc("Display order for the spawn positions checkbox in the lobby.")]
public readonly int SeparateTeamSpawnsCheckboxDisplayOrder = 7;

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

This should default to 0, and then override it in the individual mod rules to a position that makes sense (incrementing the order on boxes that should come later).

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

As part of this, please ensure that this option always appears ahead of the Debug checkbox, and does not end up in the right hand column where the label is truncated. e.g. for RA we will want this at 6, LobbyPrerequisiteCheckbox@GLOBALFACTUNDEPLOY at 7 and LobbyPrerequisiteCheckbox@GLOBALBOUNTY at 8.

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented May 21, 2018

After I've briefly checked all the maps I don't think they really need that updating. They all handle it in some way already or do not really depend on it all too much.

  • Coop and normal missions hide it already by inheriting campaign-rules's world.

  • Dropzone maps do it by having custom rules as well,

  • Fort lonestar doesn't really have any difference in gameplay with or without that checkbox, can hide it there that if preffered.

  • Bomber John is the same case I'd say.

  • Training Camp disables it already

CnC mod has only "The Hot Box" which already disables the checkbox and d2k has no custom maps included.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 21, 2018

Yeah, please do disable and lock the checkbox in the maps where the option is not relevant.

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:randomspawnpos branch from 60aa709 to 498ce4a May 21, 2018

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:randomspawnpos branch from 498ce4a to 1ce6c48 May 21, 2018

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented May 21, 2018

Adjusted positioning of checkbox

@@ -53,6 +53,8 @@ World:
endless: Endless mode
Default: hard
DisplayOrder: 5
MPStartLocations:
SeparateTeamSpawnsCheckboxVisible: false

This comment has been minimized.

@GraionDilach

GraionDilach May 24, 2018

Contributor

I'd lock the values as well (using Locked: true and Enabled: true) on all hidden maps.

@pchote pchote force-pushed the CDVoidwalker:randomspawnpos branch from 1ce6c48 to b9211b8 Jun 3, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 3, 2018

The display ordering was still conflicting on a couple of the mods and this needed a rebase, so I have updated and force pushed a fix.

I also added an extra commit to remove (rather than just disable) the unused checkboxes on minigame maps. This improves consistency between options that are disabled vs completely removed, and fixes the issue where Fort Lonestar again couldn't fit all of the options in.

@pchote

pchote approved these changes Jun 3, 2018

@pchote pchote added the PR: Needs +2 label Jun 3, 2018

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍 then since the fixup commit was also my request as well.

@pchote pchote merged commit 5b47aa4 into OpenRA:bleed Jun 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.