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.

Sign up for GitHub

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

Jump to bottom

Make gap generators work when shroud is disabled. #7665

Closed
wants to merge 1 commit into from
Closed

Make gap generators work when shroud is disabled. #7665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions OpenRA.Mods.Common/Graphics/ActorPreview.cs → OpenRA.Game/Graphics/ActorPreview.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,10 @@

using System;
using System.Collections.Generic;
using OpenRA.Graphics;
using OpenRA.Primitives;
using OpenRA.Traits;

namespace OpenRA.Mods.Common.Graphics
namespace OpenRA.Graphics
{
public interface IActorPreview
{
Expand Down
2 changes: 2 additions & 0 deletions OpenRA.Game/OpenRA.Game.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
<Compile Include="Actor.cs" />
<Compile Include="CacheStorage.cs" />
<Compile Include="FileSystem\IdxEntry.cs" />
<Compile Include="Graphics\ActorPreview.cs" />
<Compile Include="LogProxy.cs" />
<Compile Include="FileFormats\IdxReader.cs" />
<Compile Include="FileSystem\BagFile.cs" />
Expand Down Expand Up @@ -158,6 +159,7 @@
<Compile Include="Sync.cs" />
<Compile Include="TraitDictionary.cs" />
<Compile Include="Traits\Armor.cs" />
<Compile Include="Traits\CreatesDisruptionField.cs" />
<Compile Include="Traits\CreatesShroud.cs" />
<Compile Include="Traits\DrawLineToTarget.cs" />
<Compile Include="Traits\EditorAppearance.cs" />
Expand Down
6 changes: 4 additions & 2 deletions OpenRA.Game/Orders/UnitOrderGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public IEnumerable<Order> Order(World world, CPos xy, MouseInput mi)
else
{
var frozen = world.ScreenMap.FrozenActorsAt(world.RenderPlayer, mi)
.Where(a => a.Info.Traits.Contains<ITargetableInfo>() && !a.Footprint.All(world.ShroudObscures))
.Where(a => a.Info.Traits.Contains<ITargetableInfo>() && !a.Footprint.All(world.ShroudObscures)
&& !world.RenderPlayer.Shroud.IsUnderDisruptionField(a.OccCells))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring an explicit check for IsUnderDisruptionField here is a sign that the existing code is bogus. This code (from before your changes) should have been filtering out FrozenActors that aren't visible, and your changes should have just worked (because it should be making FrozenActors.Visible -> false). Please fix this properly by adding a filter on Visible instead of hacking in a disruption check in these places.

.WithHighestSelectionPriority();
target = frozen != null ? Target.FromFrozenActor(frozen) : Target.FromCell(world, xy);
}
Expand Down Expand Up @@ -74,7 +75,8 @@ public string GetCursor(World world, CPos xy, MouseInput mi)
else
{
var frozen = world.ScreenMap.FrozenActorsAt(world.RenderPlayer, mi)
.Where(a => a.Info.Traits.Contains<ITargetableInfo>() && !a.Footprint.All(world.ShroudObscures))
.Where(a => a.Info.Traits.Contains<ITargetableInfo>() && !a.Footprint.All(world.ShroudObscures)
&& !world.RenderPlayer.Shroud.IsUnderDisruptionField(a.OccCells))
.WithHighestSelectionPriority();
target = frozen != null ? Target.FromFrozenActor(frozen) : Target.FromCell(world, xy);
}
Expand Down
51 changes: 51 additions & 0 deletions OpenRA.Game/Traits/CreatesDisruptionField.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#region Copyright & License Information
/*
* Copyright 2007-2015 The OpenRA Developers (see AUTHORS)
* This file is part of OpenRA, which is free software. It is made
* available to you under the terms of the GNU General Public License
* as published by the Free Software Foundation. For more information,
* see COPYING.
*/
#endregion

using System.Linq;

namespace OpenRA.Traits
{
[Desc("Removes frozen actors and satellite icons from fog, prevents auto-target. Requires fog enabled.")]
public class CreatesDisruptionFieldInfo : ITraitInfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a Desc attribute to this, please.

{
public readonly WRange Range = WRange.Zero;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a Desc attribute to this, please.


public object Create(ActorInitializer init) { return new CreatesDisruptionField(init.Self, this); }
}

public class CreatesDisruptionField : ITick, ISync
{
public WRange Range { get { return cachedDisabled ? WRange.Zero : info.Range; } }
readonly CreatesDisruptionFieldInfo info;
readonly bool lobbyFogDisabled;
[Sync] CPos cachedLocation;
[Sync] bool cachedDisabled;

public CreatesDisruptionField(Actor self, CreatesDisruptionFieldInfo info)
{
this.info = info;
lobbyFogDisabled = !self.World.LobbyInfo.GlobalSettings.Fog;
}

public void Tick(Actor self)
{
if (lobbyFogDisabled)
return;

var disabled = self.IsDisabled();
if (cachedLocation != self.Location || cachedDisabled != disabled)
{
cachedLocation = self.Location;
cachedDisabled = disabled;
Shroud.UpdateDisruptionGenerator(self.World.Players.Select(p => p.Shroud), self);
}
}
}
}
12 changes: 6 additions & 6 deletions OpenRA.Game/Traits/CreatesShroud.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

namespace OpenRA.Traits
{
[Desc("Generates shroud, prevents auto-target. Overridden by CreatesDisruptionField if fog is enabled.")]
public class CreatesShroudInfo : ITraitInfo
{
public readonly WRange Range = WRange.Zero;
Expand All @@ -21,31 +22,30 @@ public class CreatesShroudInfo : ITraitInfo

public class CreatesShroud : ITick, ISync
{
public WRange Range { get { return cachedDisabled ? WRange.Zero : info.Range; } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because Phrohdoh told me to. "This should go above all methods and private members."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He does have a point. Properties usually go before methods.

readonly CreatesShroudInfo info;
readonly bool lobbyShroudFogDisabled;
readonly bool lobbyShroudDisabled;
[Sync] CPos cachedLocation;
[Sync] bool cachedDisabled;

public CreatesShroud(Actor self, CreatesShroudInfo info)
{
this.info = info;
lobbyShroudFogDisabled = !self.World.LobbyInfo.GlobalSettings.Shroud && !self.World.LobbyInfo.GlobalSettings.Fog;
lobbyShroudDisabled = !self.World.LobbyInfo.GlobalSettings.Shroud;
}

public void Tick(Actor self)
{
if (lobbyShroudFogDisabled)
if (lobbyShroudDisabled)
return;

var disabled = self.IsDisabled();
if (cachedLocation != self.Location || cachedDisabled != disabled)
{
cachedLocation = self.Location;
cachedDisabled = disabled;
Shroud.UpdateShroudGeneration(self.World.Players.Select(p => p.Shroud), self);
Shroud.UpdateShroudGenerator(self.World.Players.Select(p => p.Shroud), self);
}
}

public WRange Range { get { return cachedDisabled ? WRange.Zero : info.Range; } }
}
}
32 changes: 17 additions & 15 deletions OpenRA.Game/Traits/Player/FrozenActorLayer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.Drawing;
using System.Linq;
using OpenRA.Graphics;
using OpenRA.Primitives;

namespace OpenRA.Traits
{
Expand All @@ -25,6 +26,7 @@ public class FrozenActorLayerInfo : ITraitInfo
public class FrozenActor
{
public readonly MPos[] Footprint;
public readonly CPos[] OccCells;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is Occ, Occupied?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's for Occupied Cells. But there already is a method called OccupiedCells() so I wanted to give this a different name to avoid confusion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like duplication then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that method cannot be used if the actor is null. A frozen actor can exist even after its actor is destroyed. The actor's occupied cells have to be stored in the frozen actor so that they can be used even if the actor is null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what the Footprint above is for. Use that instead.

public readonly WPos CenterPosition;
public readonly Rectangle Bounds;
readonly Actor actor;
Expand All @@ -37,9 +39,7 @@ public class FrozenActor

public int HP;
public DamageState DamageState;

public bool Visible;

public bool IsRendering { get; private set; }

public FrozenActor(Actor self, MPos[] footprint, CellRegion footprintRegion, Shroud shroud)
Expand All @@ -48,6 +48,7 @@ public FrozenActor(Actor self, MPos[] footprint, CellRegion footprintRegion, Shr
isVisibleTest = shroud.IsVisibleTest(footprintRegion);

Footprint = footprint;
OccCells = Shroud.GetVisOrigins(actor).ToArray();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I use "OccCells = self.OccupiesSpace.OccupiedCells().Select(c => c.First).ToArray();" instead of this line? Do I need the checks in GetVisOrigins()?

CenterPosition = self.CenterPosition;
Bounds = self.Bounds;

Expand All @@ -64,6 +65,8 @@ public FrozenActor(Actor self, MPos[] footprint, CellRegion footprintRegion, Shr
int flashTicks;
IRenderable[] renderables = NoRenderables;
bool needRenderables;
DamageState DamageStateCached;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are private, so should the naming needs to be camelCase.

Player OwnerCached;

public void Tick()
{
Expand Down Expand Up @@ -98,23 +101,22 @@ public void Flash()

public IEnumerable<IRenderable> Render(WorldRenderer wr)
{
if (needRenderables)
if (needRenderables && (renderables == NoRenderables || DamageStateCached != DamageState || OwnerCached != Owner))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These extra checks don't look right to me. if needRenderables is true, then I would expect it to generate renderables. If there is some condition where you don't want it to generate renderables, then those conditions should be applied at the point that variable is set.

{
needRenderables = false;
if (!actor.Destroyed)
{
IsRendering = true;
renderables = actor.Render(wr).ToArray();
IsRendering = false;
}
IsRendering = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please only use the ActorPreview if actor.Destroyed is true. Using it for a living actor breaks the feature where the actor is frozen with its last visible frame (including muzzle flashes, turret position, etc).

var td = new TypeDictionary() { new HealthInit((float)(HP / 1000.0)) };
var previewInit = new ActorPreviewInitializer(Info, Owner, wr, td);
var preview = Info.Traits.WithInterface<IRenderActorPreviewInfo>().SelectMany(rpi => rpi.RenderPreview(previewInit));
renderables = preview.SelectMany(p => p.Render(wr, CenterPosition)).ToArray();
IsRendering = false;

DamageStateCached = DamageState;
OwnerCached = Owner;
}

if (flashTicks > 0 && flashTicks % 2 == 0)
{
var highlight = wr.Palette("highlight");
return renderables.Concat(renderables.Where(r => !r.IsDecoration)
.Select(r => r.WithPalette(highlight)));
}
.Select(r => r.WithPalette(wr.Palette("highlight"))));

return renderables;
}
Expand Down Expand Up @@ -179,7 +181,7 @@ public void Tick(Actor self)
public virtual IEnumerable<IRenderable> Render(Actor self, WorldRenderer wr)
{
return world.ScreenMap.FrozenActorsInBox(owner, wr.Viewport.TopLeft, wr.Viewport.BottomRight)
.Where(f => f.Visible)
.Where(f => f.Visible && !owner.Shroud.IsUnderDisruptionField(f.OccCells))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I mentioned above, your disruption checks should be built into f.Visible instead of adding IsUnderDisruptionField everywhere.

.SelectMany(ff => ff.Render(wr));
}

Expand Down
1 change: 1 addition & 0 deletions OpenRA.Game/Traits/TraitsInterfaces.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public class AttackInfo
public interface ITick { void Tick(Actor self); }
public interface ITickRender { void TickRender(WorldRenderer wr, Actor self); }
public interface IRender { IEnumerable<IRenderable> Render(Actor self, WorldRenderer wr); }
public interface IRenderActorPreviewInfo { IEnumerable<IActorPreview> RenderPreview(ActorPreviewInitializer init); }
public interface IAutoSelectionSize { int2 SelectionSize(Actor self); }

public interface IIssueOrder
Expand Down