Refactor planet orbital assignment

[dbenage-cx]

Fixes issue with orbits not persisting after planet creation.
As part of the PR, new planets are assigned a random free orbit, consistent with universe generation.
Planets created with a specific orbit are only assigned a random free orbit if the requested orbit is occupied.
If the system has no free orbits for a new planet, it is not added to system.

The ordering of planets in the side panel is by orbit position, since they currently return -1, the perceived order is due to the ID.
Planets from universe generation are named in order of creation, without regard to orbit position.
As this PR corrects the orbit reported, planets will seem to be in the wrong order. PR #761 corrects the naming.

[dbenage-cx]

Master with this PR / 761 without this PR / 761 with this PR

[Vezzra]

Typo: oribt -> orbit

[dbenage-cx]

My fingers take exception to this, apparently determined that this is the correct spelling. Thank you, will fix.

[Vezzra]

Ok, @dbenage-cx, I think I need a little help to understand what this PR exactly tries to achieve.

Obviously your refactored code is more streamlined/efficient, but the only functional difference I could glance from the changes is how the case is handled when trying to insert a planet at a specific orbit that is already occpied by another planet.

The old code apparently did nothing in that case. Your code tries to insert the planet at a randomly chosen different orbit, and making sure that planet gets removed from any orbit it might have occupied before.

So, if I understand correctly, there are actually two cases where old and new code will yield different results:

1. Trying to insert a planet that does not currently occupy another orbit in that system at an orbit that is already occupied by another planet:

• Old code: planet gets added to the system, but not assigned to an orbit. Either this has been overlooked (meaning, it's a full blown bug), or Insert has never been intended to be used for that case. Which means that this case is taken care of somewhere else, or supposed to never happen.
• New code: planet gets added to the system, but assigned a different orbit than requested. In any case most likely the more robust implementation.

2. Trying to insert a planet that currently occupies another orbit in that system at an orbit that is already occupied by another planet:

• Old code: planet remains at its original orbit, nothing happens.
• New code: planet gets assigned to a randomly chosen new orbit. However, if I understand correctly, the purpose of this PR is to ensure orbits persist after planet creation, so in this particular case the old code seems to do a better job - unless I'm missing something here?

Hence my question: what exactly is the issue this PR tries to address? Have you came across cases where 1) actually lead to buggy results/behavior?

I have no idea if those edge cases actually even ever happen, if there are any instances where Insert is used to add a planet to a specific orbit that is already occupied. If not, old and new code have no functional difference, right...?

[dbenage-cx]

@Vezzra Apologies for the long delay.
Chief concern of this PR is to ensure System correctly assigns orbits.

To address this, I made some assumptions/descisions on the scope of Insert:

• If something should be done with an existing object, it should be done prior to insertion or handled in another function (move to another orbit/system, cause a collision, swap orbits, etc).
• When a system is at capacity, the imposed limit should be obeyed. (maintains that different systems may have specific limits, also influenced by 761)

Issues with old code:

• Net result of all planets reporting an orbit of -1 (no orbit)
• When not given a specific orbit, assigns to the first orbit (replacing any existing). I don't believe the block at (old code) line 341 ever executes.
• When given a specific orbit, which is assigned to a different object, the existing object is left with no orbit.
• Copy clears the orbit assignments, without resetting them to INVALID_OBJECT_ID. This potentially leads to this and other code assuming the orbit is assigned.

The current method to name planets in universe creation should result in a disordered display of planets in the sidepanel. Since orbits all return -1, the sidepanel display falls back to order by ID, which ends up working out to an ordered display.

Looking at the existing code, there should be at least one planet with a valid orbit (in each system with any planets). I failed to find any planets with a valid orbit. With the new code I have not found any planets without an orbit.

I need to address 2) and a couple of issues noticed, but wanted to respond before making changes. Please let me know if I missed any questions/concerns.

[Vezzra]

@dbenage-cx:

Apologies for the long delay

No problem. You're not getting paid for this, after all 😉

Ok, I think that it begins to dawn on me where we're heading, but there is still a lot of confusion left, so please bear with me 😉

From what I've understood so far, apparently you ran into that whole mess with the orbits when you tried to fix the issue that all planets and asteroid belts that get created by FOCS effects (currently only this nebula thing) get the same name (which is the name of the system without roman numeral suffix).

Then your attempts to find a solution didn't work, and you traced that back to planets getting ordered weirdly, and not being able to get consistent odering between universe generation and FOCS, which ultimately lead to the orbit stuff.

Right so far? Hopefully 😉

Net result of all planets reporting an orbit of -1 (no orbit)

That's... utterly baffling. Looking at the code (both old and new) I don't understand how that could happen, and how the changes you made might have fixed it.

Where/how exactly did you observe that? Client side or serve side? Because server side that should not be happening. The universe generation scripts dutifully iterate over the available orbits when trying to populate the star systems, and specify the orbit when creating and inserting a planet. Furthermore, the interface code specifically checks if an orbit is free before calling System::Insert and would fail with an error if that condition isn't met.

That's at least two safety nets that have to malfunction (without reporting any errors in any way on top of that), which strikes me as highly unlikely. Furthermore, map generation would be severely impaired, especially planet placement would be pretty much screwed.

As your changes to System::Insert don't really deal with that specific issue, I assume it's your change to System::Copy that is supposed to fix that. I don't think that System::Copy is the source of the problems here, at least not server side, see below.

When not given a specific orbit, assigns to the first orbit (replacing any existing). I don't believe the block at (old code) line 341 ever executes.

Because of the glitch in System::Copy? Do you think the if (m_orbits[o] == INVALID_OBJECT_ID) in line 343 (old code) never evaluates to true because of this? If that glitch would affect things on server side (and therefore the orbit set not contain INVALID_OBJECT_ID for each emtpy orbit), then yes, that would have the potential to lead to all planets not getting assigned an orbit ever (because OrbitOccupied would malfunction). But see below, that should not be the case...

When given a specific orbit, which is assigned to a different object, the existing object is left with no orbit.

Without reporting the failure back to the calling code (just logging an error message), which is definitely a problem. Your code is an improvement insofar as it tries to find another orbit, and if that fails too, doesn't insert the planet at all (also doesn't move it). However, still no feedback to the calling code, so only a partial fix (I'm aware though that a more thorough fix will require some substantial refactoring, so I agree that is the best you could do without having to fix things on a much larger scale).

Still, I wonder about the side effects in the various places where System::Insert is called to insert planets. I assume that the original (different) behaviour is expected there, which means planet gets inserted, if it can't get assigned an orbit, it is left without one. With your code the planet does not get inserted nor moved, which potentially leads to unexpected behaviour of the calling code.

Copy clears the orbit assignments, without resetting them to INVALID_OBJECT_ID. This potentially leads to this and other code assuming the orbit is assigned.

AFAIK UniverseObject::Copy as well as descendants and corresponding member functions in other classes all serve only the purpose of copying the subset of the universe that is visible to a certain player/empire (and which is sent to the respective client). So, while your addition certainly is an improvement and addresses potential issues client side (although I can't think of any case where System::OrbitOccupied would get called client side), I don't know how that should have any effects server side.

The current method to name planets in universe creation should result in a disordered display of planets in the sidepanel. Since orbits all return -1, the sidepanel display falls back to order by ID, which ends up working out to an ordered display.

Looking at the existing code, there should be at least one planet with a valid orbit (in each system with any planets). I failed to find any planets with a valid orbit. With the new code I have not found any planets without an orbit.

This lets me believe that you ran into all those issues client side, specifically when examining what happens in the sidepanel code. Am I correct in assuming that it's there where all orbits return -1 with the old and a valid planet ID with your new code?

That would indicate that System::Copy is indeed at fault here, but only as far as the client side is concerned. Although I still don't understand why all orbits return -1, looking at the old code only those orbits that are empty on the server side should have a wrong value. All the others should have copied over the correct planet IDs from the server - weird.

So basically I think we need to further investigate what really goes wrong here and where. I guess some debugging to check if orbit assignment server side really works correctly as I'm assuming, or if the problem actually does has its source there.

I'll help you trying to get to the bottom of this, as far as I can. I'm really curious now, so I guess I'm going to do some debugging in the universe generation code and see what happens to orbit assignment there. Expect to get to that today in the evening and tomorrow, so should be able to report back soon.

Regarding the scope of System::Insert, I think your assumptions/decisions are right. It should not be responsible for handling inserting at already occupied orbits or in systems with no free orbits. That's the responsibility of the calling code.

If System::Insert is called with an orbit that is already occupied by another planet than the one being inserted, or with no orbit specified but no free orbits to place the planet randomly, it should simply fail and report back an error to the calling code (logging an error message too is fine of course). Either by throwing or by return value.

As that will require a more comprehensive refactoring of all the code where System::Insert is called, a stop-gap fix that replicates the original behaviour is required. Which means it must not fail with an error in these cases, but somehow handle them by falling back on something failsave.

We can either stick with having no orbit assignment for planets we can't place in an orbit (which I don't consider a good idea), or by extending the set of orbits in that case and assign the planet to the newly added orbit.

The most obvious in-game case that comes to mind that can trigger this is the Planetary Starlane Drive. It potentially enables the player to stack an unlimited number of planets in one system. If System::Insert just fails to relocate the planet in that case, I foresee buggy behaviour.

[dbenage-cx]

Net result of all planets reporting an orbit of -1 (no orbit)

Where/how exactly did you observe that?

Attempts to rely on orbit kept failing, so I started using the debug dump to check the current orbit (will post PR shortly). I did not suspect to be working off of a copy here, the change in Copy was to eliminate any potential problems in code modifying m_orbits.

When not given a specific orbit, assigns to the first orbit (replacing any existing). I don't believe the block at (old code) line 341 ever executes.

Because of the glitch in System::Copy?

L334 is an assignment to the first orbit, a pretty simple fix by itself.

Planetary Starlane Drive.

I actually checked with this to troubleshoot moving into a full system. To my surprise (I expected something bad), the planet bumped right back to its original system with a correct orbit. It still incurred the costs (effects) of a move, which was a concern, though minor for now due to the rarity of such a situation. This PR has changed since, though I suspect in the worst case it is the same outcome.

I agree with your points here. Will increase the system orbits as a fallback to insertion in a full system (as a stopgap).

[Vezzra]

L334 is an assignment to the first orbit, a pretty simple fix by itself.

Oh crap! I've completely missed all the time that this is an assignment, not a comparison here (obviously didn't notice the missing second =). Well, that changes a lot of course, at least with planet insertions that don't specify an orbit.

Honestly, I don't think that this has ever been intended as a fix, but is just a typo and a bug. And needs to be fixed, no question.

[dbenage-cx]

Recreated branch with smaller changes (assumed new PRs were not desired in this instance).

To summarize:

• Copy appears to be the cause of most of my confusion, addressing that solves the client display.
• Effects::MoveTo fails to guard from overfilling systems only due to L334.
• While System::Insert should guard from adding planets without an orbit, addressing this is left for later work since there is no immediate concern.
• Planets requesting an occupied orbit should fail. For now this will log an error and attempt the first free orbit.

[Vezzra]

Nifty. So you went with the bare minimum of changes and decided not to expand the set of orbits to accomodate planets if the system is full. Instead, if System::Insert needs to add a planet to an already full system, the new planet simply gets no orbit assigned. Together with the fixed

if (m_orbits[o] == obj->ID())

line and the fix to System::Copy orbits should work better now.

I went ahead and merged so we can get on with #761. Of course, now we need to address two issues:

• In the planets sidebar the order of planets is jumbled, but Append planet ordinal on creation #761 (or a revised version of it) should fix that.
• Effects::MoveTo no longer stacks planets beyond system capacity. Meaning, if a player tries to move a planet (via the PSD) into an already full system, the operation fails. The code handles the case correctly, the only issue here is that the player gets a confusing sitrep telling him that his planet X arrived at system Y by use of the PSD. However, system Y is the same the planet has been in before, and no other explanation is given. If there is an easy fix for that (maybe the sitrep generating effects group can check if the system actually changed?), this fix should go in too for the release.