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

GDI-turret and GDI-/NOD-gates replace walls #14862

Merged
merged 3 commits into from Apr 7, 2018

Conversation

Projects
None yet
6 participants
@fruestueck
Copy link
Contributor

fruestueck commented Feb 27, 2018

Implements traits Replaceable and Replacement.
Allows GDI-Turrets, GDI- and Nod-Gates to replace walls.

ToDo: Polish code according to you : )

  • Done: After replacing a building with a replacement, it is not possible to attach a plug or replace the location again.
  • Done: Allow Replacement to replace more than one Replaceable type (by accepting more items)
  • Done: Make 2x2 (and bigger) buildings replaceable
  • Done: Make Replaceable Conditional

unbenannt
Gates and Turrets built on walls.

Thankful for any feedback : )

Closes #12088
Continues #14850

@fruestueck fruestueck force-pushed the fruestueck:replace-ts-wall-gate branch from bc0e8ab to 716559a Mar 1, 2018

@fruestueck

This comment has been minimized.

Copy link
Contributor Author

fruestueck commented Mar 2, 2018

Concerning that one bug described above

After replacing a building with a replacement, it is not possible to attach a plug or replace the location again.

Solved

unbenannt
For testing, i've made a powerplant replaceable by itself. If i

  1. Build one Powerplant (short pp) ontop of two other,
  2. and then try to replace the pp again,
  3. it isn't replaceable, becouse of something unknown blocking where the previous pp were.
  4. This is also, why plugs cant be placed on turrets build on walls.

I think it has something to do with a blocking layer added by buildings (that i do not clear when disposing replaceable actors) but have no idea yet. Will continue to search for it.

Any advice, how to solve that bug is welcome : )

IsBuildingAtCellReplaceable(world, cell, replacementInfo)))
return false;
}
else

This comment has been minimized.

@Unit158

Unit158 Mar 2, 2018

Contributor

The else if should be on one line, please

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

Will change it.
I was not 100 percent sure, if i've handled the if-statement right.

{
if (!world.Map.Contains(cell))
return false;

// cell is occupied by a structure

This comment has been minimized.

@Unit158

Unit158 Mar 2, 2018

Contributor

This is clear given the next line.


if (!bi.AllowInvalidPlacement && world.ActorMap.GetActorsAt(cell).Any(a => a != toIgnore))
{
// check for 'Replacement'-trait & if it fits

This comment has been minimized.

@Unit158

Unit158 Mar 2, 2018

Contributor

This comment is not needed, same reason as above.

return false;

var replacementInfo = ai.TraitInfoOrDefault<ReplacementInfo>();
if (!(replacementInfo != null &&

This comment has been minimized.

@Unit158

Unit158 Mar 2, 2018

Contributor

This would be more clear as replacementInfo == null && !IsBuildingAtCellReplaceable(...

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

simple

if (replacementInfo == null)
	return false;
if (!IsBuildingAtCellReplaceable(world, cell, replacementInfo))
	return false;

or

if ((replacementInfo == null) ? true : !IsBuildingAtCellReplaceable(world, cell, replacementInfo))
	return false;

@fruestueck fruestueck force-pushed the fruestueck:replace-ts-wall-gate branch from 573456e to 30350e7 Mar 3, 2018

@fruestueck

This comment has been minimized.

Copy link
Contributor Author

fruestueck commented Mar 3, 2018

All known issues solved. Review required

@pchote
Copy link
Member

pchote left a comment

This looks and works pretty good. I have a couple more code design suggestions, nothing too major:

}

public static IEnumerable<Pair<CPos, Actor>> GetLineBuildCells(World world, CPos location, string name, BuildingInfo bi)
{
var lbi = world.Map.Rules.Actors[name].TraitInfo<LineBuildInfo>();
var topLeft = location; // 1x1 assumption!

if (world.IsCellBuildable(topLeft, bi))
if (world.IsCellBuildable(topLeft, null, bi))

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

This should pass in the ActorInfo the same was as in CanPlaceBuilding.

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

I've added that (commiting in a few) but was wondering what difference it makes, when IsCellBuildable gets the ActorInfo passed here

@@ -44,17 +52,18 @@ public static bool CanPlaceBuilding(this World world, string name, BuildingInfo
return true;

var res = world.WorldActor.TraitOrDefault<ResourceLayer>();
var actorInfo = world.Map.Rules.Actors[name];

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

How much work would it be to pass the ActorInfo into this method instead of name?

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

I ll look into this.
Still remember you asking for it but i just wanted to make it run first, before improveing.

@@ -71,7 +80,7 @@ public static bool CanPlaceBuilding(this World world, string name, BuildingInfo
continue;

var cell = topLeft + i * vecs[d];
if (world.IsCellBuildable(cell, bi))
if (world.IsCellBuildable(cell, null, bi))

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

Likewise here.

var replacementInfo = ai.TraitInfoOrDefault<ReplacementInfo>();
if (replacementInfo == null)
return false;
if (!IsBuildingAtCellReplaceable(world, cell, replacementInfo))

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

Can you please store the result of the world.WorldActor.Trait<BuildingInfluence>().GetBuildingAt(cell) above in a variable and then query the Replaceable trait inline here? IsBuildingAtCellReplaceable is only used from here, and most of its code is repeating queries that we already have the result for!

public object Create(ActorInitializer init) { return new Replaceable(init, this); }
}

public class Replaceable

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

It would probably be a good idea to make this a ConditionalTrait so that the replacement can be turned on or off by other things. (e.g. modders may want to disable replacement if an actor is EMPed or shielded).

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

What will i need for this? Methods like EnablePlug and DisablePlug. Anything else?
I'd like to add ConditiionalTrait but currently i have no idea how.

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

Making ReplaceableInfo inherit from ConditionalTraitInfo and this inherit ConditionalTrait<ReplaceableInfo> plus my other request that replaces AcceptsReplacement with a query that includes IsTraitDisabled will be enough.

this.Info = info;
}

public bool AcceptsReplacement(Actor self, string[] replaces)

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

This method isn't needed. OpenRA's convention for this kind of thing is to do host.TraitsImplementing<Replaceable>().Any(p => !p.IsTraitDisabled && replacement.Replacement.Overlaps(p.Info.Types)); at the point it is used (BuildingUtils in this case). This is accounting for my other comments about Info.Types and ConditionalTrait.

{
[FieldLoader.Require]
[Desc("Identifyer for this replaceable Actor (matched against Replaces in Replacement).")]
public readonly string Type = null;

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

OpenRA conventions have been moving to supporting multiple Types here and then checking for overlap.

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

So Replaceable (instead of Replacement) should now every possible Replacement for it? Or shoud it look more like Plug and Pluggable (wich is still unclear to me)?

@@ -128,6 +134,8 @@ GACTWR:
tower.sam: tower.sam
ProvidesPrerequisite@buildingname:
SelectionDecorations:
Replacement:
Replaces: gawall

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

gawall seamlessly connects to nawall, so I don't see any real reason to restrict tower or gate types to specific wall types. It would be better to have a single Wall replacement type shared by both.

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

I tried both ending up with this seperation only becouse it seemed "better" with the GDI-turret.
Will change it to wall as recommendet

This comment has been minimized.

@GraionDilach

GraionDilach Mar 3, 2018

Contributor

gawall seamlessly connects to nawall

Well, that's not how TS worked. This is the TS-accurate setup.

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

I'll have a look at vanilla-SUN. EDIT: Couldn't test it, vanilla crashed a couple of times :/
Think it's up for discussion. Changing the replaceable would be a simple yaml file fix, anyway.

Should we change LineBuild to distinct nawall and gawall?

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

I'm not fussed either way. It seems silly to not allow cross-faction replacement, but as it will be a rare edge case in practice I don't see any reason to cause drama by insisting on it.

@@ -16,6 +16,8 @@ NAWALL:
CrushClasses: heavywall
LineBuild:
NodeTypes: wall, turret
Replaceable:
Type: nawall

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

Didn't you also want to support placing walls on top of damaged walls to replace them?

This comment has been minimized.

@fruestueck

fruestueck Mar 3, 2018

Author Contributor

I just tried it and discovered a funny bug. When replacing a wall, it gets removed, then build and build a second time ontop (couse of linebuild, i assume).
Edit: still looking for whats causing the troubles with walls replacing walls - but nothing to major for now

[Desc("Identifyer for this replaceable Actor (matched against Replaces in Replacement).")]
public readonly string Type = null;
[Desc("Replacement types this Relpaceable actor accepts.")]
public readonly string[] Replacements = null;

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

Sorry, I meant to keep this as a collection of arbitrary types, rather than actor names, but just to change it to an HashSet<string> that accepts multiple!

[Desc("Replaceable Types this Actor replaces.")]
public readonly string[] Replaces = { };
[Desc("Replacement type (matched against Conditions in Replaceable.")]
public readonly string Type = null;

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

And then keeping this as an array (actually now that I think about it this should be a HashSet<string>) too. You can then use Overlaps to check whether the two sets overlap.

@fruestueck

This comment has been minimized.

Copy link
Contributor Author

fruestueck commented Mar 4, 2018

Issues solved thanks to the community and pchote. Review required

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 20, 2018

I split out the plumbing changes, with a few modifications, into #14955. a79bf39 has the rest of the changes in this PR rebased on top of that.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 20, 2018

IMO it would be nice to add this to D2K turrets too. The original game didn't support this, but it does visually connect the turrets to walls.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 22, 2018

#14955 has been merged, so needs a rebase. A simple option (if you trust my changes) is to hard reset the branch against my a79bf39 commit and then rebase that against bleed. Let me know if you have any problems, as I or another @OpenRA/engine-hackers can do this for you.

@pchote pchote force-pushed the fruestueck:replace-ts-wall-gate branch from 6064a22 to f42ef0e Apr 7, 2018

@pchote pchote removed the PR: Rebase me! label Apr 7, 2018

@pchote

pchote approved these changes Apr 7, 2018

@pchote pchote added the PR: Needs +2 label Apr 7, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 7, 2018

I have rebased this and added turret replacement to d2k. LGTM 👍

@Smittytron
Copy link
Contributor

Smittytron left a comment

LGTM

@ltem ltem merged commit cc04ec6 into OpenRA:bleed Apr 7, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

ltem commented Apr 7, 2018

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.