Bug description

Fighters hang if the flagship has fighter bays available. The fighters will race to the flagship and join. If the flagship fills up with bays the fighters do not try to find a new parent. Even if escorts have free bays the fighter will not try to find a new bay to join.

Expected behavior

After a ship's bays are filled the fighter should try to find a new parent among player escorts. If an escort has an available bay then the fighter (or drone) should board that escort.

If there are no bays among escorts then the abandoned fighter should fall back to keeping station with the player flagship.

Actual behavior

Fighters race to flagship. When bays are filled the fighters keep station with the flagship. Even if bays are available among escorts the fighter or drone will not attempt to board amongst a new parent.

Save game

You can test with this save game shared in discord.

Testing

Tested the following scenarios where a player fleet attempted to pick up two (2) abandoned fighters with various fleet configurations with free bays.

Abandoned fighters	Flagship Free Bays	Escort Free Bays
2	2	0
2	1	6
2	1	0
2	1	1
2	0	2
2	0	1
2	0	0

• In all above tested scenarios, if the flagship has a bay the fighters would attempt to board.
• If the flagship was full or the flagship has no bays then the fighters would board an escort.
• If no bays are available then the abandoned fighters would keep station with the player flagship.

Performance Impact

None. ~~The way Ship::CanCarry has been rewritten it should theoretically be more performant than the old method as the number of player escorts grows.~~

I reverted CanCarry changes. Unless I get feedback to restore my changes I'll leave it. I think my version is better performance but it has its own drawbacks.

Posted a video in discord https://discordapp.com/channels/251118043411775489/266345072554016768/979119391532798042

https://discord.com/channels/251118043411775489/266345072554016768/979228330475794462 a review from the original person reporting this which caused me to look into it:

matthew.najmon says:

samrocketman btw, got a chance to test out the new fighter docking behavior, and it works beautifully. much thanks, for a fix I've been wanting for years now.

Warning

After some more extensive play testing I've found this change to be unstable. government segfaults (which I removed to see if the issue was fixed). Upon removal then HasBays() segfaults. I don't understand why this is segfaulting. Perhaps someone with more experience could explain to me.

Locally, I completely reverted source/Ship.cpp and still got a segfault. I think this has to do with how I'm calling CanBeCarried in source/AI.cpp and Ship.cpp is just a symptom.

Fixed segfault

The segfault was indeed caused by AI. 90a54bce6d340b8c302250d884b70c096ec383d3 is the fix.

Question

I went ahead and reverted CanCarry. Should I use my original CanCarry implementation which I introduced in 0028fa43ed56ba24b2f52f06157bd81b67c38078 and later reverted?

Currently, this PR does not update CanCarry. I feel like my version would be a little more performant but the draw back is ships may race to board escorts.

I am not convinced this is stable, yet so am converting to a draft.

Ready for review

I have resolved all stability issues and outstanding bugs. After play testing this feels good so am comfortable with a general review/merge.

@quyykk

I think the better way to solve this is to look at the code that is responsible for finding a new parent. Probably the reason why fighters don't seem to reparent is because of the random call that triggers on average every 30s, or something else (I haven't looked at that code in detail).

I believe this section of code that I'm editing is that code which triggers roughly every 30s (inside random int). I browsed the code extensively for parent boarding and this is what I found. If you know a better section I'm open to looking at what you reference.

---

Can you expand on what you mean by me reverting previous behavior? Are you referring to me reverting my own code within this PR or further back in the history.

---

IMO rehoming a fighter should be pretty immediate. I found the delay in rehoming to be pretty frustrating. There's no reason it should run every 30s to find a new parent. If a fighter has a valid parent it will never trigger the logic path for finding a new parent.

Worst case scenario is it calculates every time for abandoned fighters. I kept capturing korath fighters over 30 abandoned fighter more than my fleet bay capacity and saw no performance difference.

Can you expand on what you mean by me reverting previous behavior? Are you referring to me reverting my own code within this PR or further back in the history.

I mean that you're doing the opposite of the code a couple of lines above the SetParent call. For example the CanCarry check.

A couple lines above there's a comment explaining that if there is no suitable parent it assigns the flagship, even if it can't carry. But then later on you changed it to not update the parent.

There's no reason it should run every 30s to find a new parent.

Yeah I agree.

A couple lines above there's a comment explaining that if there is no suitable parent it assigns the flagship, even if it can't carry. But then later on you changed it to not update the parent

Ok I will check at a computer it should be more clear in an IDE. I'll look at cleaning up the full logic path since it is hard to tell from phone. Thanks

I started from scratch based on what I've learned about performance tuning in #6726 .

@quyykk ready for a fresh review

Something I learned about how random works in endless sky. !Random::Int(1800) will roll a random number between 0 and 1800. It has to be 0 for it to succeed. It's not on average every 30s to search for new parent. Instead, it is more accurate to say 1:1800 chance to actually re-home a fighter.

In practice this ends up being much longer than 30s.

BaysFree does not appear to work but CanCarry does. CanCarry is more expensive since it iterates across all escorts. I am going to look at why BaysFree does not work.

I have figured out the problem (since I am testing a few PRs there was a bug in another PR).

I plan to use CanCarry but only for player escorts. NPC escorts should use HasBays to keep game performance good.

Pushed a fix where if the player has a large number of excess fighters they do not scramble in circles.

This PR solves some issues discussed in #6329

I think I have addressed all of the feedback. Also, this PR has been pretty thoroughly play tested at this point. I haven't found anything else to fix.