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

Add Support of Types for GivesBuildableArea #13641

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
7 participants
@MustaphaTR
Member

MustaphaTR commented Jul 14, 2017

Fixes #10550.

Also moved GivesBuildableArea to its own file from Building.cs

For testcase i made D2K engineer allow placement of walls around him normally and turrets when owner has palace.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Sep 15, 2017

Member

NTS: Needs rebase.

Member

MustaphaTR commented Sep 15, 2017

NTS: Needs rebase.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Sep 15, 2017

Member

Updated.

Member

MustaphaTR commented Sep 15, 2017

Updated.

@pchote

Sorry for the long delay in reviewing this. Looks good overall, but a couple of requests to make it a bit more general:

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Buildings/Building.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Buildings/Building.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Buildings/GivesBuildableArea.cs Outdated
@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Oct 22, 2017

Member

Updated and Rebased.

Testcase now gives engineers ability to build turrets around them when owner has Palace to test that conditions on GivesBuildableArea: and multipile GivesBuildableArea: traits are working.

Member

MustaphaTR commented Oct 22, 2017

Updated and Rebased.

Testcase now gives engineers ability to build turrets around them when owner has Palace to test that conditions on GivesBuildableArea: and multipile GivesBuildableArea: traits are working.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Oct 23, 2017

Member

Updated.

Member

MustaphaTR commented Oct 23, 2017

Updated.

@penev92

A few minor nits to clear up.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 10, 2017

Member

About the default value thing, and also to remove hack/special case added with #13845, i think i can move Adjacent to own trait that something like RequiresBuildableArea:, which requires Building:so instead of -1 special case and on unbuildable buildings we can just not use that trait. And also give it [FileLoader.Require] with no problem.

Member

MustaphaTR commented Nov 10, 2017

About the default value thing, and also to remove hack/special case added with #13845, i think i can move Adjacent to own trait that something like RequiresBuildableArea:, which requires Building:so instead of -1 special case and on unbuildable buildings we can just not use that trait. And also give it [FileLoader.Require] with no problem.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 11, 2017

Member

Yes, that sounds great, thanks :)

Member

penev92 commented Nov 11, 2017

Yes, that sounds great, thanks :)

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 12, 2017

Member

Updated, but not done yet. I'm aware that i messed the rebase up, i'll fix that. This also needs a Upgrade Rule, and i only manually upgraded the RA mod, so others wouldn't work right now.

Also added a test case that removes RequiresBuildableArea: from Kennel.

Edit: Travis is unhappy that i haven't updated all the mods, you can ignore it for now.

Member

MustaphaTR commented Nov 12, 2017

Updated, but not done yet. I'm aware that i messed the rebase up, i'll fix that. This also needs a Upgrade Rule, and i only manually upgraded the RA mod, so others wouldn't work right now.

Also added a test case that removes RequiresBuildableArea: from Kennel.

Edit: Travis is unhappy that i haven't updated all the mods, you can ignore it for now.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 13, 2017

Member

I unfortunately couldn't manage to write a sane upgrade rule for seperating Ajacent from Building. If anyone can write it please do. I'll just write one that adds AreaTypes: to GivesBuildableArea and fix other stuff manually for now.

Member

MustaphaTR commented Nov 13, 2017

I unfortunately couldn't manage to write a sane upgrade rule for seperating Ajacent from Building. If anyone can write it please do. I'll just write one that adds AreaTypes: to GivesBuildableArea and fix other stuff manually for now.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 19, 2017

Member

Fixed travis, Lonestar was missing AreaTypes: for GivesBuildableArea: on its custom rules (i didn't run upgrade rule for RA, so it was missed).

Member

MustaphaTR commented Nov 19, 2017

Fixed travis, Lonestar was missing AreaTypes: for GivesBuildableArea: on its custom rules (i didn't run upgrade rule for RA, so it was missed).

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 20, 2017

Member

Sorry, I know this comes a bit too late, but here's an upgrade rule based on yours:

if (engineVersion < 20171113)
{
	// AreaTypes support is added to GivesBuildableArea and it is required.
	var givesBuildableArea = node.Value.Nodes.FirstOrDefault(n => n.Key == "GivesBuildableArea");
	if (givesBuildableArea != null)
		givesBuildableArea.Value.Nodes.Add(new MiniYamlNode("AreaTypes", "building"));

	// RequiresBuildableArea trait is added and Building.Adjacent is moved there.
	var building = node.Value.Nodes.FirstOrDefault(n => n.Key == "Building");
	if (building != null)
	{
		var adjacent = building.Value.Nodes.FirstOrDefault(n => n.Key == "Adjacent");
		var areaTypes = new MiniYamlNode("AreaTypes", "building");
		var requiresBuildableArea = new MiniYamlNode("RequiresBuildableArea", "");

		requiresBuildableArea.Value.Nodes.Add(areaTypes);
		if (adjacent != null)
			requiresBuildableArea.Value.Nodes.Add(adjacent);

		node.Value.Nodes.Add(requiresBuildableArea);
		building.Value.Nodes.Remove(adjacent);
	}
}

I think after that this PR should be ready.

Member

penev92 commented Nov 20, 2017

Sorry, I know this comes a bit too late, but here's an upgrade rule based on yours:

if (engineVersion < 20171113)
{
	// AreaTypes support is added to GivesBuildableArea and it is required.
	var givesBuildableArea = node.Value.Nodes.FirstOrDefault(n => n.Key == "GivesBuildableArea");
	if (givesBuildableArea != null)
		givesBuildableArea.Value.Nodes.Add(new MiniYamlNode("AreaTypes", "building"));

	// RequiresBuildableArea trait is added and Building.Adjacent is moved there.
	var building = node.Value.Nodes.FirstOrDefault(n => n.Key == "Building");
	if (building != null)
	{
		var adjacent = building.Value.Nodes.FirstOrDefault(n => n.Key == "Adjacent");
		var areaTypes = new MiniYamlNode("AreaTypes", "building");
		var requiresBuildableArea = new MiniYamlNode("RequiresBuildableArea", "");

		requiresBuildableArea.Value.Nodes.Add(areaTypes);
		if (adjacent != null)
			requiresBuildableArea.Value.Nodes.Add(adjacent);

		node.Value.Nodes.Add(requiresBuildableArea);
		building.Value.Nodes.Remove(adjacent);
	}
}

I think after that this PR should be ready.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 20, 2017

Member

Updated.

Member

MustaphaTR commented Nov 20, 2017

Updated.

@penev92

Code looks good to me now, and I'm keeping my fingers crossed for the YAML changes.

@penev92 penev92 merged commit ca1448c into OpenRA:bleed Nov 20, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@penev92
Member

penev92 commented Nov 20, 2017

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