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 diplomatic stances to JamsRadar #11926
Conversation
@@ -18,6 +18,9 @@ public class JamsRadarInfo : TraitInfo<JamsRadar> | |||
{ | |||
[Desc("Range for jamming.")] | |||
public readonly WDist Range = WDist.Zero; | |||
|
|||
[Desc("What diplomatic stances are affected.")] | |||
public readonly Stance JamsRadarStances = Stance.Ally | Stance.Neutral | Stance.Enemy; |
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.
Please make this default to Neutral | Enemy or add an upgrade rule because this regresses third-party mods as-is.
432b5ec
to
fd16974
Compare
updated. |
@@ -18,6 +18,9 @@ public class JamsRadarInfo : TraitInfo<JamsRadar> | |||
{ | |||
[Desc("Range for jamming.")] | |||
public readonly WDist Range = WDist.Zero; | |||
|
|||
[Desc("What diplomatic stances are affected.")] | |||
public readonly Stance JamsRadarStances = Stance.Ally | Stance.Neutral; |
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.
Uhm, you did this wrong. Neutral | Enemy, not Ally | Neutral was my request (you can remove the YAML definition thenafter as well.)
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.
Oops, sry :/
fd16974
to
25c7425
Compare
@GraionDilach fixed. |
@@ -544,6 +544,7 @@ MRJ: | |||
Offset: -256,0,256 | |||
JamsRadar: | |||
Range: 15c0 | |||
JamsRadarStances: Enemy, Neutral |
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 became redundant now.
25c7425
to
34a7071
Compare
And changed again. |
👍 |
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 the same change as JamsMissiles did: #11983
34a7071
to
f295f04
Compare
Updated. |
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.
The return statement is quite complex and the All
doesn't seem right to me, but I can't spend much time trying to make heads or tails of it currently, so won't be able to approve this.
{ | ||
[Desc("Range for jamming.")] | ||
public readonly WDist Range = WDist.Zero; | ||
|
||
[Desc("What diplomatic stances are affected.")] | ||
public readonly Stance JamsRadarStances = Stance.Enemy | Stance.Neutral; |
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.
s/What/Which
And this should be named Stances
(the JamsRadar part is implied).
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.
We have DeflectionStances
in JamsMissiles
, so JamStances
here for "consistency"?
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.
Basically you're right, though. It doesn't make much sense to include more than just Stances
.
f295f04
to
309407d
Compare
Should I do the same change for |
{ | ||
[Desc("Range for jamming.")] | ||
public readonly WDist Range = WDist.Zero; | ||
|
||
[Desc("What diplomatic stances are affected.")] |
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.
"Which"
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.
changed.
Looks good except for the typo, 👍
I'd say no. It's unnecessary churn for now. |
309407d
to
353e613
Compare
Updated. |
No description provided.