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
Make gap generators work when shroud is disabled. #7665
Conversation
@@ -74,7 +74,7 @@ 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.IsUnderGapGen(a.Footprint)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is duplicated in the Order method, will that need updating too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, it seems to work with just this code. This is to prevent having an attack cursor for hidden buildings under fog (they dont have frozen images now if gap gen is active).
bf5b14a
to
86c690b
Compare
OK I think I fixed all code styling issues. I also squashed all commits. |
Also for gap gen to cover a building, all its footprint cells must be within range. So if part of the building is outside gap gen range it will appear as normal. Does footprint include bibs, the space below structures? |
Shroud is an engine-level feature, so please don't introduce RA-specific logic and naming there. You can do that instead in the mod-level traits. |
Do you mean UpdateShroudGeneration to UpdateGapGeneration etc changes? But it doesn't generate shroud anymore if fog is enabled. Isn't gap a general name for it? |
No, shroud is the name for shroud and fog is the name for fog. |
OK so how should it be called? It's not a shroud generator or fog generator but a combination of those. ShroudFogGeneration? |
Actually if fog is enabled, gap generator doesn't generate fog. The fog is generated automatically by the engine. What gap gen does then is remove frozen actors from fog. So it either generates shroud or removes frozen actors from fog. WHat should it be called? Gap generator seems to be the most appropriate name, it generates a gap in enemy radar/map. Are there any other name suggestions? ShroudFogGeneration doesn't look suitable since gap gen doesn't generate fog. |
Also the other point pchote made, I haven't introduced any ra specific code here? The shroud generation code was already there, I just changed its behaviour. And don't all mods have shroud and fog option? So it's not mod specific. |
The concept of having the "shroud generation" do one thing if shroud is enabled but another if it isn't is a RA specific idea. Other mods may want to have pure shroud generators or pure cloak generators, but you are instead enforcing them to do what the gap generator does. The shroud code needs to keep these ideas separate (with separate methods and state for shroud and cloak generation), and then the gameplay code (i.e. CreatesShroud) should expose trait properties so that the generation type can be configured in the yaml. |
OK it was agreed in IRC to call this functionality "Stealth Field Generation". I will make the necessary changes and send another commit. |
OK, the other PRs that were related to this one are merged so I started working on this again. Has dependencies label can be removed. Now I have another naming question: So the general trait will be called CreatesStealth, it is a general stealth field. And then there will be two types of stealth. One is the current gap generator, it will generate shroud, so it's called Type: Shroud. But what should the new type be called? The new gap generator behaviour will act as frozen actor remover from fog, prevent units from being auto targeted and also remove satellite icons. Sensor disruptor was suggested in IRC. What do you think? |
👎 for |
This is basically what I've gathered from all the discussions about how it should work when done:
|
Yes. Or simpler: if fog is enabled, gap gen acts as disruption generator. If fog is disabled but shroud is enabled, it generates shroud. |
@phrohdoh: So what should it be called instead of CreatesStealth.cs? |
@@ -349,4 +349,4 @@ | |||
</Target> | |||
--> | |||
<ItemGroup /> | |||
</Project> | |||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What needs to be reverted? Those changes in the project file were done by Visual Studio automatically when I renamed CreatesShroud.cs to CreatesConcealment.cs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should stage just the <Compile Include="Traits\CreatesDisruptionField.cs" />
line. We're trying to avoid staging other lines the different IDEs keep automatically changing so they don't go back and forth all the time.
I made the changes you requested. While I was doing that I also realized that there should also be a separate RenderDisruptionCircle trait to draw the range circle. And shouldn't this and RenderShroudCircle be in Mods.Common? Or even in the engine itself. Because they are part of CreatesShroud and CreatesDisruptionField traits. |
And the developing continues... |
ef8a57d
to
96d7b08
Compare
I have changed this to use ActorPreview for rendering the frozen actor. I also added some checks so that it is only rendered when the building state has changed. |
@@ -1,4 +1,4 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be undone.
…Gens work when shroud is disabled. Performance optimization Uses IsDisabled() method for simplification. Optimizes fog/shroud checks. Adds a new trait called RenderDisruptionCircle. Converts some IEnumerables to array. Simplifies if statements. Uses ActorPreview for rendering. Shortens some lines. Reverts faulty project file changes.
@@ -25,6 +26,7 @@ public class FrozenActorLayerInfo : ITraitInfo | |||
public class FrozenActor | |||
{ | |||
public readonly MPos[] Footprint; | |||
public readonly CPos[] OccCells; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is Occ
, Occupied?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I am closing this in an attempt to clean up the PR queue a bit. Feel free to reopen it once the conceptual problems are cleared up. |
This apparently depends on satellite code being fixed to handle visibility and stuff. Related: #8203 |
Fixes #6123. This will look at game settings, if fog of war is disabled, gap gens will work like they do now. If fog of war is enabled, gap gens will generate fog and there won't be any frozen actors visible while they are operational. Also this commit disables auto targeting for units/structures under gap, even if you have satellite. So satellite icons will also not appear where gap gen is active.
I tested this and seems to work.