initiate combat for hidden blockade #4835

agrrr3 · 2024-01-05T06:47:17Z

should fix #4834

this was v1 (e9c7496)

 Do also initiate combat if there is a blockade
    
    * check all fleets for blockade and signal CombatConditions if so
    * extend the first pass to look at all fleets, not just empire fleets
    ** move the check if we are handling an empire inside
    ** return CombatConditions signal if we find a blockade
    ** after the pass, only continue if we are not handling empire fleets (i.e. unowned/monsters fleets)

QA status: Tested, good to go

now it is v2

 Do also initiate combat if there is a blockade
    * add a pass to find out which empires have (dangerous) obstructing fleets in the system
    * then determine if there is an empire A which is at war with an empire B where A has an aggressive fleet and B has an obstructing fleet in the system
    * if so, also initiate combat

Note this has slightly different behavior from first implemtation (v1)

prototypical implementation v1 triggered combat, if at least fleet was actually obstructed
this triggers if a dangerous enemy fleet is set to blockade.

So if the obstructing empire did not see anything to obstruct, v1 did not trigger combat.
v2 actually does

agrrr3 · 2024-01-05T10:26:27Z

Ok, i tested and fixed a small bug and retested and everything looks good.

Hidden obstruction triggers combat, hidden hide does not (both for monsters and empires).
Normal obstruction triggers combat.
Non-armed non-aggressive ships do not trigger combat.

This is ready to merge

Vezzra · 2024-01-08T10:49:01Z

@agrrr3, @o01eg, @geoffthemedio: The issue this PR addresses is assigned to the 0.5.0.1 milestone. This PR however has been assigned to the 0.5.1 milestone.

So, my question is: What should it be? Should the issue also be addressed in the release branch, or only in master? In the first case this PR needs also needs to be assigned to 0.5.0.1, and marked as "cherry-pick for release", in the latter case the corresponding issue should be reassigned to 0.5.1.

agrrr3 · 2024-01-08T14:23:32Z

So, my question is: What should it be? Should the issue also be addressed in the release branch, or only in master?

i did not check by playtesting if the issue really exists in 0.5, but it is a blocker.
And code-wise this should be broken (obstructive stance already exists) and the fix the same, so I'd say:

PR needs also needs to be assigned to 0.5.0.1, and marked as "cherry-pick for release"

agrrr3 · 2024-01-09T13:06:27Z

@Vezzra can you give me the right to add the "cherry-pick for release" ? do I need to squash the two commits for that?

Vezzra · 2024-01-15T14:06:11Z

@Vezzra can you give me the right to add the "cherry-pick for release" ?

Unfortunately I don't know how to do that (adding just the right to manage issue/PR labels), if that level of granular control is even possible.

I'll add that label myself.

do I need to squash the two commits for that?

While not strictly necessary, it makes the cherry picking easier if there is only one commit. So unless there is a good reason not to, please squash.

geoffthemedio · 2024-01-15T17:19:09Z

server/ServerApp.cpp

@@ -2288,36 +2288,42 @@ namespace {
        }
    }

-    void GetFleetsVisibleToEmpireAtSystem(std::set<int>& visible_fleets, int empire_id, int system_id,
-                                          const ScriptingContext& context)
+    bool HasBlockadeOrGetFleetsVisibleToEmpireAtSystem(std::set<int>& visible_fleets, int empire_id,


this is not a good function design... it's a weird mix of return value and in/out parameters, and it's very confusingly doing... something? name not at all clear.

granted, the existing out parameter isn't a good idea, but mixing both is worse, I think.

also, the name "HasBlockade" is unclear... the empire_id has a ship that is blockaded, or that empire has a ship that is blockading something else there?

are we at C++17 ? we could probably return std::variant<std::set<int>, boolean> , the intention here is that only one of the results is meaningful. Actually it should be either a set or a signal that there is a blockade (in which case visibility does not matter in this case).

we certainly could also do an extra pass just to check for blockade, but that seems unnecessarily wasteful.

An old school recommendation seems: always return out params as return value and put those into a kind of structure when there are multiple out params
according to https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-out-multi

I am not sure if this is possible, but something like the following would be my preferred signature (in pseudo c++):
std::variant<std::set<int>, ProcessingShortcut::AbortedDueToBlockadeAtSystem> GetFleetsVisibleToEmpireAtSystem(int empire_id, int system_id, const ScriptingContext& context)
The function name would explain the standard return value (a set of fleets) and the enum value AbortedDueToBlockadeAtSystem would self-describe the non-standard/abort result. And it would also not return a half-finished set of visible fleets.

Another idea would be to do the further processing/abort decision at the call site using a lambda or so. But have even less of a clue if that makes sense.

are we at C++17 ?

In FreeOrion code (not GiGi code), standard is C++20

we could probably return std::variant<std::set<int>, boolean> , the intention here is that only one of the results is meaningful. Actually it should be either a set or a signal that there is a blockade (in which case visibility does not matter in this case).

Could work, but I think just pair<set, bool> would be fine. The set can be ignored if not needed.

we certainly could also do an extra pass just to check for blockade, but that seems unnecessarily wasteful.

I'd rather do that: separate functions to determine what is visible at a system to an empire, and to check if there is a blockade at a system.

geoffthemedio · 2024-01-15T17:32:23Z

server/ServerApp.cpp

+                continue;
+            if (fleet->Blockaded(context)) {
+                TraceLogger(combat) << "\t\tfleet (" << fleet->ID() << ") is blockaded. Shortcut determining visibility. ";
+                return true; // found a blockade -> combat ensues


this seems problematic... you're returning true to indicate there is a blockade in a system if you find any fleet that is blockaded. But the blockaded and blockading fleets might not want to initiate combat in that case (unless one or both are aggressive). I guess it's only called for empires that have aggressive fleets present, but that's not indicated in the function name.

Non-armed non-aggressive ships do not trigger combat.

So in cases where an unarmed or non-aggressive ship is blockaded, the blockader won't initiate combat, and the blockaded ship won't be considered aggressive and thus the check for blockades of it won't be done.

Seemingly the blockading fleet should still be made visible to the blockaded empire in that case, though?

That could lead to a bit of an inconsistency, in that when a blockade of an aggressive armed fleet causes combat to happen, the blockading ships potentially initially not visible (depending on their stealth), and any ship that doesn't attack during the combat wouldn't be revealed by it. But if a non-aggressive/non-armed fleet is blockaded by an obstrutive fleet, and no combat happens, and (somehow) the blockading fleet is thus revealed, presumably the whole blockadaing fleet would be made visible...

Seemingly the blockading fleet should still be made visible to the blockaded empire in that case, though?

I am not convinced it should. If we do that disrupting supply should also make the blockader visible for consistency.

That could lead to a bit of an inconsistency, in that when a blockade of an aggressive armed fleet causes combat to happen, the blockading ships potentially initially not visible (depending on their stealth), and any ship that doesn't attack during the combat wouldn't be revealed by it. But if a non-aggressive/non-armed fleet is blockaded by an obstrutive fleet, and no combat happens, and (somehow) the blockading fleet is thus revealed, presumably the whole blockadaing fleet would be made visible...

maybe only one (random) blocking ship would need to be made visible if non-military ships/supply or obstructed

For the moment (i.e. this PR) I would not change this, rather start discussion/making a use case matrix.

That one is not able to break a hidden blockade at all is almost game breaking - i think it is as bad as the invincible hidden carriers we had once.

geoffthemedio · 2024-01-15T17:57:11Z

Regardless, I don't think this needs to be applied to v0.5.0

Note this has slightly different behavior from first implemtation (v1) prototypical implementation v1 triggered combat, if at least fleet was actually obstructed this triggers if a dangerous enemy fleet is set to blockade. So if the obstructing empire did not see anything to obstruct, v1 did not trigger combat. v2 actually does * do an extra pass, find

agrrr3 · 2024-01-23T07:53:19Z

@geoffthemedio I pushed a new implementation. Note the behavior change for double-hidden case (hidden aggressive empire A, hidden obstructing enemy B). I am not sure i like it. I think relying on actual obstruction is better, what you think

agrrr3 · 2024-01-23T07:54:37Z

side-note bug: when loading a save-game for testing, the resulting ship damage of a medium krill swarm was zero

agrrr3 · 2024-01-31T07:20:51Z

just a note from the dev meeting, @geoffthemedio implemented something in #4765 which could make this obsolete
so the decision was to check that out first

agrrr3 · 2024-01-31T10:56:05Z

@geoffthemedio I compiled #4835 and besides the UI problems, also no combat was triggered by. Think we should fix this besides your changes(?)

agrrr3 force-pushed the 20240104_InitiateCombatForHiddenBlockade branch 2 times, most recently from 72cfd29 to e9c7496 Compare January 5, 2024 10:23

o01eg added this to the v0.5.1 milestone Jan 5, 2024

o01eg added category:feature The Issue/PR describes or implements a new game feature. component:game mechanic The Issue/PR deals with the currently used or planned game mechanics. labels Jan 5, 2024

Vezzra added the status:cherry-pick for release The PR should be applied to the currently open release branch. label Jan 15, 2024

Vezzra modified the milestones: v0.5.1, v0.5.0.1 Jan 15, 2024

geoffthemedio reviewed Jan 15, 2024

View reviewed changes

agrrr3 force-pushed the 20240104_InitiateCombatForHiddenBlockade branch from e9c7496 to db685c7 Compare January 22, 2024 16:29

o01eg removed the status:cherry-pick for release The PR should be applied to the currently open release branch. label Jan 30, 2024

o01eg modified the milestones: v0.5.0.1, v0.5.1 Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initiate combat for hidden blockade #4835

initiate combat for hidden blockade #4835

agrrr3 commented Jan 5, 2024 •

edited

agrrr3 commented Jan 5, 2024

Vezzra commented Jan 8, 2024

agrrr3 commented Jan 8, 2024

agrrr3 commented Jan 9, 2024

Vezzra commented Jan 15, 2024

geoffthemedio Jan 15, 2024 •

edited

geoffthemedio Jan 15, 2024

agrrr3 Jan 16, 2024

agrrr3 Jan 16, 2024 •

edited

geoffthemedio Jan 17, 2024 •

edited

geoffthemedio Jan 15, 2024 •

edited

geoffthemedio Jan 15, 2024 •

edited

agrrr3 Jan 23, 2024

geoffthemedio commented Jan 15, 2024

agrrr3 commented Jan 23, 2024

agrrr3 commented Jan 23, 2024

agrrr3 commented Jan 31, 2024

agrrr3 commented Jan 31, 2024

initiate combat for hidden blockade #4835

Are you sure you want to change the base?

initiate combat for hidden blockade #4835

Conversation

agrrr3 commented Jan 5, 2024 • edited

agrrr3 commented Jan 5, 2024

Vezzra commented Jan 8, 2024

agrrr3 commented Jan 8, 2024

agrrr3 commented Jan 9, 2024

Vezzra commented Jan 15, 2024

geoffthemedio Jan 15, 2024 • edited

Choose a reason for hiding this comment

geoffthemedio Jan 15, 2024

Choose a reason for hiding this comment

agrrr3 Jan 16, 2024

Choose a reason for hiding this comment

agrrr3 Jan 16, 2024 • edited

Choose a reason for hiding this comment

geoffthemedio Jan 17, 2024 • edited

Choose a reason for hiding this comment

geoffthemedio Jan 15, 2024 • edited

Choose a reason for hiding this comment

geoffthemedio Jan 15, 2024 • edited

Choose a reason for hiding this comment

agrrr3 Jan 23, 2024

Choose a reason for hiding this comment

geoffthemedio commented Jan 15, 2024

agrrr3 commented Jan 23, 2024

agrrr3 commented Jan 23, 2024

agrrr3 commented Jan 31, 2024

agrrr3 commented Jan 31, 2024

agrrr3 commented Jan 5, 2024 •

edited

geoffthemedio Jan 15, 2024 •

edited

agrrr3 Jan 16, 2024 •

edited

geoffthemedio Jan 17, 2024 •

edited

geoffthemedio Jan 15, 2024 •

edited

geoffthemedio Jan 15, 2024 •

edited