FreeOrion

Forums for the FreeOrion project
It is currently Wed Nov 22, 2017 9:17 am

All times are UTC




Post new topic Reply to topic  [ 4 posts ] 
Author Message
 Post subject: Splitting up ALL_EMPIRES
PostPosted: Wed Oct 18, 2017 2:14 am 
Offline
Programmer

Joined: Sun Feb 14, 2016 12:08 am
Posts: 342
I'd like to propose splitting up ALL_EMPIRES into more distinct definitions.

Currently this variable is used for at least:
  • References to ownership by the server (Unowned)
  • A flag for no empire selection
  • An invalid value
  • All playable empires (human and ai)
  • All empires (including Unowned)
  • Any single empire

This has caused a number of bugs and confusion on the intended function of various code.

I'm looking at putting these into a common UniverseFwd.h replacing the various "extern ALL_EMPIRES" with an include.
Will retain the int type instead of an enum, so it is easier to transition to a UUID if that is done later.
Code:
// UniverseFwd.h
namespace FactionID {
    extern FO_COMMON_API const int ANY_OWNED;  // 0 Any single empire controlled by human or AI
    extern FO_COMMON_API const int INVALID;  // -1 Bad return value, issue a warning when received or an error if initially returning
    extern FO_COMMON_API const int NONE;  // -2 No empire
    extern FO_COMMON_API const int UNIVERSE;  // -10 Universe as an empire aka Unowned
    extern FO_COMMON_API const int ALL;  // -20 All empires, including UNIVERSE
    extern FO_COMMON_API const int ALL_OWNED; // -21 All empires controlled by human or AI
}

These are the cases I've come across during some initial work, more may be desired (possibly a separate namespace if they do not have relation to empires).
Values are grouped so code can refer to Foo < FactionID::ALL or Foo <= FactionID::INVALID
Those portions of code may need to further change if and how UUIDs are implemented, but the determination will be clearer.
The definitions would remain in Universe.cpp

I'm in favor of namespace for linting and ease of determining options for type vs. consistency with the other forms (INVALID_EMPIRE_ID, ALL_EMPIRE_IDS, etc).

_________________
Any content posted should be considered licensed GNU GPL 2.0 and/or CC-BY-SA 3.0 as appropriate.


Top
 Profile  
 
PostPosted: Wed Oct 18, 2017 7:51 am 
Offline
Programming, Design, Admin
User avatar

Joined: Wed Oct 08, 2003 1:33 am
Posts: 12016
Location: Munich
I'm reluctant, because if ALL_EMPIRES is split up into so many different values, then all the code that handles empire IDs needs to handle all these cases, which is a lot more complicated and cumbersome.

Maybe I'd support this if it's just ANY_EMPIRE and NO_EMPIRE, which I think will cover most use cases where there's ambiguity about what ALL_EMPIRES means (which in current usage means NO_SINGLE_EMPIRE) in a particular context. This would use ANY_EMPIRE for the invalid / error case when an empire ID is returned, would use NO_EMPIRE when there is no owner, and both would have distinct clear meanings when passed as parameters to something requesting en empire ID. If necessary, the two sentinel values could instead be named ANY_EMPIRE_OR_INVALID and NO_EMPIRE.

Alternatively, rename ALL_EMPIRES to NO_SINGLE_EMPIRE, and continue to use it with that meaning in all cases where an empire id is returned, as in general, that's all that's important in those cases... ie. some function or script is returning a single empire ID. If no single empire id can be returned from that function, it doesn't matter exactly why, and the calling code just needs to handle the case where no single empire ID could be returned for whatever reason. However, in cases where an empire ID is passed as a paremter, and NO_SINGLE_EMPIRE doesn't adequately specify the meaning (eg. select objects owned by no empire vs. objects owned by any empire vs. objects with any or no owner), modify those functions to instead take an EmpireAffiliationType enum, with the empire ID being an optional additional parameter. If a single empire is meant, then specify AFFIL_SELF. If any empire or no empire are meant, then specify AFFIL_ANY or AFFIL_NONE, with the other EmpireAffiliationType values being treated in locally appropriate ways, mostly as invalid / error cases, and the empire id parameter being ignored / unnecessary.


Top
 Profile  
 
PostPosted: Thu Oct 19, 2017 3:23 pm 
Offline
Programmer

Joined: Mon Feb 29, 2016 8:37 pm
Posts: 199
I agree with Geoff, this a lot of work.

There are payoffs in code quality and future features. In new future features, this change would allow monster on monster conflict and diplomacy with natives.

The remainder of this post is technical issues/concerns, which you can ignore as you see fit.

I agree with Geoff defining a bunch of new constants and leaving the interpretation of the boolean logic to other programmers will likely introduce future bugs as different programmers interpret the constants differently.

Defining the constants as extern is a older idiom, designed to prevent multiple definitions of identical constants in each compilation unit. This a) prevents any code from being optimized/inlined b) creates an implicit memory collisions for multi-threaded code. A better way is to use the C++11 constexpr, which will not create a definition during compilation and will be trivially inlined.

Elevate empire_id from an int to its own type FactionID which contains only an int. There will be no more accidental comparisons with int, object_ids or INVALID_OBJECT_ID.

The FactionID type needs to be ordered and hashable, so that it works in both std::map and std::unordered_map.

The type needs to be streamable and otherwise printable as a int so that it works with the current display/logging.

Add the following member functions to implement the decision logic.

faction_id.IsEmpire() -- is this a client controlled in game entity?

faction_id.IsFaction() -- is this a non-client controlled in game entity? This would be a new distinction. Non-client controlled in game entities could be the Experimentors and all their ships/colonies, individual native planets, individual species of monsters. This would enable monster on monster attacks and taking diplomatic action with natives. New factions would be created by the FOCS content code.

faction_id.IsUniverse() -- is this a game engine/universe created object/initiated action? This is for all of the actions where the universe/game engine create content. This would in fact be two values for client and server created universe content, so faction_id.IsUniverse() === (faction_id.IsServer() || faction_id.IsClient())

faction_id.IsServer(), faction_id.IsClient() -- Currently, code in universe/ uses ALL_EMPIRES to refer to changes initiated by both the client and the server. Sometimes the code is expected to be called with ALL_EMPIRES on the client in order to immediately make visible changes for the human/AI, which are expected to be overwritten out by permanent changes from the server during the turn update. Currently, these portions of the code are at best indicated by comments and sometimes are merely implicit.


Top
 Profile  
 
PostPosted: Sat Oct 21, 2017 5:08 pm 
Offline
Programmer

Joined: Sun Feb 14, 2016 12:08 am
Posts: 342
Thank you both for excellent pointers, I'll need to mull over this a bit, so want to note I may not start working on it in near future.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 4 posts ] 

All times are UTC


Who is online

Users browsing this forum: Google [Bot] and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
cron
Powered by phpBB® Forum Software © phpBB Group