Author Topic: Extending ColonyType.h/.cpp from OP2Helper  (Read 1359 times)

Online Vagabond

  • Sr. Member
  • ****
  • Posts: 336
Extending ColonyType.h/.cpp from OP2Helper
« on: March 21, 2016, 12:59:54 PM »
I am looking at adding the following 3 functions to ColonyType.h and ColonyType.cpp underneath OP2Helper project. These 3 functions allow testing a map_id to see if it represents a building, vehicle, or unit.

Note: Doesn't include current code in ColonyType.h
Code: [Select]
bool IsBuilding(map_id mapID);
bool IsVehicle(map_id mapID);
bool IsUnit(map_id mapID);

Note: Doesn't include current code in ColonyType.cpp
Code: [Select]
// Returns true if the map_id is a building.
bool IsBuilding(map_id mapType)
{
switch (mapType)
{
case mapCommonOreMine:
case mapRareOreMine:
case mapGuardPost:
case mapLightTower:
case mapCommonStorage:
case mapRareStorage:
case mapForum:
case mapCommandCenter:
case mapMHDGenerator:
case mapResidence:
case mapRobotCommand:
case mapTradeCenter:
case mapBasicLab:
case mapMedicalCenter:
case mapNursery:
case mapSolarPowerArray:
case mapRecreationFacility:
case mapUniversity:
case mapAgridome:
case mapDIRT:
case mapGarage:
case mapMagmaWell:
case mapMeteorDefense:
case mapGeothermalPlant:
case mapArachnidFactory:
case mapConsumerFactory:
case mapStructureFactory:
case mapVehicleFactory:
case mapStandardLab:
case mapAdvancedLab:
case mapObservatory:
case mapReinforcedResidence:
case mapAdvancedResidence:
case mapCommonOreSmelter:
case mapSpaceport:
case mapRareOreSmelter:
case mapGORF:
case mapTokamak:

return true;
}

return false;
}

// Returns true if the building is a vehicle.
bool IsVehicle(map_id mapType)
{
switch (mapType)
{
case mapCargoTruck:
case mapConVec:
case mapSpider:
case mapScorpion:
case mapLynx:
case mapPanther:
case mapTiger:
case mapRoboSurveyor:
case mapRoboMiner:
case mapGeoCon:
case mapScout:
case mapRoboDozer:
case mapEvacuationTransport:
case mapRepairVehicle:
case mapEarthworker:

return true;
}

return false;
}

// Returns true if the map_id is either a vehicle or a building.
bool IsUnit(map_id mapType)
{
return IsBuilding(mapType) || IsVehicle(mapType);
}

Online Vagabond

  • Sr. Member
  • ****
  • Posts: 336
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #1 on: March 21, 2016, 01:37:30 PM »
Scratch the first draft, I think the following would be better.

Only thing I don't understand is that currently ColonyType.cpp does not include ColonyType.h. Would it be a problem to restructure ColonyType.cpp to include ColonyType.h?

Proposed code for ColonyType.h and ColonyType.cpp below. This time the full files are posted.

ColonyType.h
Code: [Select]
#pragma once

#include <Outpost2DLL/Outpost2DLL.h>
#include <array>

// Details of map_id enum are not needed here so just forward declare it
// This way we don't need to include Outpost2DLL.h (which speeds up the compile)
//enum map_id;


bool IsEdenOnlyWeapon(map_id weaponType);
bool IsPlymouthOnlyWeapon(map_id weaponType);
bool IsCommonWeapon(map_id weaponType);

bool IsEdenWeapon(map_id weaponType);
bool IsPlymouthWeapon(map_id weaponType);

bool IsBuilding(map_id mapID);
bool IsVehicle(map_id mapID);
bool IsUnit(map_id mapID);

extern std::array<map_id, 38> BuildingTypes;
extern std::array<map_id, 15> VehicleTypes;

ColonyType.cpp
Code: [Select]
#include "ColonyType.h"
//#include <Outpost2DLL/Outpost2DLL.h> // Main Outpost 2 header to interface with the game

// Returns true only if the weapon is Eden specific
bool IsEdenOnlyWeapon(map_id weaponType)
{
// Use switch case fallthrough (no break statement)
switch(weaponType)
{
case mapLaser:
case mapRailGun:
case mapThorsHammer:
case mapAcidCloud:
return true; // Yep
}

return false; // Nope
}

// Returns true only if the weapon is Plymouth specific
bool IsPlymouthOnlyWeapon(map_id weaponType)
{
// Use switch case fallthrough (no break statement)
switch(weaponType)
{
case mapMicrowave:
case mapStickyfoam:
case mapRPG:
case mapESG:
case mapSupernova:
case mapSupernova2:
case mapEnergyCannon:
return true; // Yep
}

return false; // Nope
}

// Returns true if weapon can be built by both colonies
bool IsCommonWeapon(map_id weaponType)
{
// Use switch case fallthrough (no break statement)
switch(weaponType)
{
case mapEMP:
case mapStarflare:
case mapStarflare2:
case mapNormalUnitExplosion:
return true; // Yep
}

return false; // Nope
}

// Returns true if Eden can build the weapon
bool IsEdenWeapon(map_id weaponType)
{
return (IsCommonWeapon(weaponType) || IsEdenOnlyWeapon(weaponType));
}

// Returns true if Plymouth can build the weapon
bool IsPlymouthWeapon(map_id weaponType)
{
return (IsCommonWeapon(weaponType) || IsPlymouthOnlyWeapon(weaponType));
}

// Returns true if the map_id is a building.
bool IsBuilding(map_id mapType)
{
return std::find(BuildingTypes.begin(), BuildingTypes.end(), mapType) != BuildingTypes.end();
}

// Returns true if the building is a vehicle.
bool IsVehicle(map_id mapType)
{
return std::find(VehicleTypes.begin(), VehicleTypes.end(), mapType) != VehicleTypes.end();
}

// Returns true if the map_id is either a vehicle or a building.
bool IsUnit(map_id mapType)
{
return IsBuilding(mapType) || IsVehicle(mapType);
}

std::array<map_id, 38> BuildingTypes{
mapCommonOreMine,
mapRareOreMine,
mapGuardPost,
mapLightTower,
mapCommonStorage,
mapRareStorage,
mapForum,
mapCommandCenter,
mapMHDGenerator,
mapResidence,
mapRobotCommand,
mapTradeCenter,
mapBasicLab,
mapMedicalCenter,
mapNursery, mapSolarPowerArray,
mapRecreationFacility,
mapUniversity,
mapAgridome,
mapDIRT,
mapGarage,
mapMagmaWell,
mapMeteorDefense,
mapGeothermalPlant,
mapArachnidFactory,
mapConsumerFactory,
mapStructureFactory,
mapVehicleFactory,
mapStandardLab,
mapAdvancedLab,
mapObservatory,
mapReinforcedResidence,
mapAdvancedResidence,
mapCommonOreSmelter,
mapSpaceport,
mapRareOreSmelter,
mapGORF,
mapTokamak
};

std::array<map_id, 15> VehicleTypes{
mapCargoTruck,
mapConVec,
mapSpider,
mapScorpion,
mapLynx,
mapPanther,
mapTiger,
mapRoboSurveyor,
mapRoboMiner,
mapGeoCon,
mapScout,
mapRoboDozer,
mapEvacuationTransport,
mapRepairVehicle,
mapEarthworker
};
« Last Edit: March 21, 2016, 01:55:32 PM by Vagabond »

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3775
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #2 on: March 21, 2016, 09:46:16 PM »
I can see those functions being useful.


I'm going to suggest an alternate implementation. The buildings and vehicles take up consecutive numbers within the map_id enum, so you can just perform a simple range check. That would be more efficient than iterating over an array and checking if a given element is found. Here's an example:
Code: [Select]
bool IsVehicle(map_id mapType)
{
    return ((mapType >= mapCargoTruck) || (mapType <= mapEarthworker));
}

The colony type check for the weapons can't use such a simple range check since the indexes of the weapons for different colonies are intermixed. There is no one clear range of Eden only or Plymouth only weapons. Hence the code needs a more general solution.
Code: [Select]
	mapAcidCloud,					// 3B  Eden
mapEMP, // 3C  Both
mapLaser, // 3D  Eden
mapMicrowave, // 3E  Plymouth
mapRailGun, // 3F  Eden
mapRPG, // 40  Plymouth
mapStarflare, // 41 Vehicle Starflare  Both
mapSupernova, // 42 Vehicle Supernova  Plymouth
mapStarflare2, // 43 GuardPost Starflare  Both
mapSupernova2, // 44 GuardPost Supernova  Plymouth
mapNormalUnitExplosion, // 45  Both
mapESG, // 46  Plymouth
mapStickyfoam, // 47  Plymouth
mapThorsHammer, // 48  Eden
mapEnergyCannon, // 49  Plymouth

Functions for Eden only or Plymouth only buildings or vehicles might make sense. It has the same intermixing problem as the weapons, so a simple range based check won't work there either. I would recommend a switch statement over the array searching there too though, and again for efficiency. The switch statement should compile down to a binary search, which runs in O(log(n)) time, while iterating over an array is a linear search, which runs in O(n) time (slower).

You could use a binary search on a sorted array, which would then be O(log(n)) time. Such a sorted assumption would need to be documented, since any edits to the array that break the sort order will also cause the code to return incorrect results.

Another note is the "ColonyType" file name is not very descriptive of the IsBuilding or IsVehicle functions. Perhaps the name was not very descriptive of the old functions either. It might make sense to rename the file, rather than split the functions into separate files.

Online Vagabond

  • Sr. Member
  • ****
  • Posts: 336
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #3 on: March 24, 2016, 02:17:07 PM »
@Hooman,

I didn't think about taking advantage of vehicles and units being consecutive in the map_id enum. Below is code to reflect this change. I also filled in other similar functions for completion's sake, to include IsEdenOnlyVehicle, IsEdenVehicle, IsPlymouthOnlyUnit, etc. I moved the function summaries to the header file so a user can review the .h file for an overview of the functions' use cases.

I think you mean && and not || in the sample you provided.

I'll sit on this post for a day or two before posting to the repository in case anyone has better naming conventions or implementation suggestions.

ColonyType.h
Code: [Select]
#pragma once

#include <Outpost2DLL/Outpost2DLL.h>

// Returns true if the map_id is a weapon native only to Eden
bool IsEdenOnlyWeapon(map_id weaponType);

// Returns true if the map_id is a weapon native only to Plymouth
bool IsPlymouthOnlyWeapon(map_id weaponType);

// Returns true if the map_id is a weapon that is native to both colonies
bool IsCommonWeapon(map_id weaponType);

// Returns true if the map_id is a weapon that can be built by Eden
bool IsEdenWeapon(map_id weaponType);

// Returns true if the map_id is a weapon that can be built by Plymouth
bool IsPlymouthWeapon(map_id weaponType);

// Returns true if the map_id is a building that is native only to Eden
bool IsEdenOnlyBuilding(map_id buildingType);

// Returns true if the map_id is a building that is native only to Plymouth
bool IsPlymouthOnlyBuilding(map_id buildingType);

// Returns true if the map_id is a building that Eden can construct
bool IsEdenBuilding(map_id buildingType);

// Returns true if the map_id is a building that Plymouth can construct
bool IsPlymouthBuilding(map_id buildingType);

// Returns true if the map_id is a vehicle that is native to Eden only
bool IsEdenOnlyVehicle(map_id vehicleType);

// Returns true if the map_id is a vehicle that is native to Plymouth only
bool IsPlymouthOnlyVehicle(map_id vehicleType);

// Returns true if the map_id is a vehicle that can be built by Eden
bool IsEdenVehicle(map_id vehicleType);

// Returns true if the map_id is a vehicle that can be built by Plymouth
bool IsPlymouthVehicle(map_id vehicleType);

// Returns true if the map_id is a building
bool IsBuilding(map_id mapID);

// Returns true if the map_id is a vehicle
bool IsVehicle(map_id mapID);

// Returns true if the map_id is either a vehicle or a building
bool IsUnit(map_id mapID);

ColonyType.cpp
Code: [Select]
#include "ColonyType.h"

bool IsEdenOnlyWeapon(map_id weaponType)
{
// Use switch case fallthrough (no break statement)
switch(weaponType)
{
case mapLaser:
case mapRailGun:
case mapThorsHammer:
case mapAcidCloud:
return true;
}

return false;
}

bool IsPlymouthOnlyWeapon(map_id weaponType)
{
// Use switch case fallthrough (no break statement)
switch(weaponType)
{
case mapMicrowave:
case mapStickyfoam:
case mapRPG:
case mapESG:
case mapSupernova:
case mapSupernova2:
case mapEnergyCannon:
return true;
}

return false;
}

bool IsCommonWeapon(map_id weaponType)
{
// Use switch case fallthrough (no break statement)
switch(weaponType)
{
case mapEMP:
case mapStarflare:
case mapStarflare2:
case mapNormalUnitExplosion:
return true;
}

return false;
}

bool IsEdenWeapon(map_id weaponType)
{
return (IsCommonWeapon(weaponType) || IsEdenOnlyWeapon(weaponType));
}

bool IsPlymouthWeapon(map_id weaponType)
{
return (IsCommonWeapon(weaponType) || IsPlymouthOnlyWeapon(weaponType));
}

bool IsEdenOnlyBuilding(map_id buildingType)
{
switch (buildingType)
{
case mapMagmaWell:
case mapMeteorDefense:
case mapGeothermalPlant:
case mapConsumerFactory:
case mapObservatory:
case mapAdvancedResidence:
return true;
}

return false;
}

bool IsPlymouthOnlyBuilding(map_id buildingType)
{
switch (buildingType)
{
case mapForum:
case mapMHDGenerator:
case mapArachnidFactory:
case mapReinforcedResidence:
return true;
}

return false;
}

bool IsEdenBuilding(map_id buildingType)
{
return IsBuilding(buildingType) && !IsPlymouthOnlyBuilding(buildingType);
}

bool IsPlymouthBuilding(map_id buildingType)
{
return IsBuilding(buildingType) && !IsEdenOnlyBuilding(buildingType);
}

bool IsEdenOnlyVehicle(map_id vehicleType)
{
switch (vehicleType)
{
case mapGeoCon:
case mapRepairVehicle:
return true;
}

return false;
}

bool IsPlymouthOnlyVehicle(map_id vehicleType)
{
switch (vehicleType)
{
case mapSpider:
case mapScorpion:
return true;
}

return false;
}

bool IsEdenVehicle(map_id vehicleType)
{
return IsVehicle(vehicleType) && !IsPlymouthOnlyVehicle(vehicleType);
}

bool IsPlymouthVehicle(map_id vehicleType)
{
return IsVehicle(vehicleType) && !IsEdenOnlyVehicle(vehicleType);
}

bool IsBuilding(map_id buildingType)
{
// mapCommonOreMine and mapTokamak are respectively the first and last building indices
return ((buildingType >= map_id::mapCommonOreMine && buildingType <= map_id::mapTokamak));
}

bool IsVehicle(map_id vehicleType)
{
// mapCargoTruck and mapEarthworker are respectively the first and last building indices
return ((vehicleType >= map_id::mapCargoTruck) && (vehicleType <= map_id::mapEarthworker));
}

bool IsUnit(map_id unitType)
{
return IsBuilding(unitType) || IsVehicle(unitType);
}

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3775
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #4 on: March 25, 2016, 06:14:42 AM »
Quote
I think you mean && and not || in the sample you provided.
*Facepalm* You're right.

Quote
I'll sit on this post for a day or two before posting to the repository in case anyone has better naming conventions or implementation suggestions.
You can commit now, and then in a few days later make another commit to update the names. There's often a lot of clarity added when updates like that are done as separate commits, and reasons for the changes are added to the commit message.

I like how you implemented IsEdenVehicle, IsPlymouthVehicle, IsEdenBuilding, and IsPlymouthBuilding. Now, how might you implement IsCommonVehicle, and IsCommonBuilding?


Looking at the header file, I found myself wondering if the comments might be better at the end of the line. I like to keep header files a bit more compressed so I don't need to scroll through so much. Conventional practice though is to put a lot of function documentation ahead of the function declaration, so maybe not. Of course my other thought is, do the comments really add anything over the name of the function? I think self documenting code should be a greater aim than simple documentation. I'd be curious what other people think on the matter.

Online Vagabond

  • Sr. Member
  • ****
  • Posts: 336
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #5 on: March 25, 2016, 06:07:47 PM »
Code added to the repository. I also included functions IsCommonBuilding and IsCommonVehicle. I am on the fence about commenting what these functions do in the header file since they are fairly self explanatory. I kept comments since the original author had put comments on their functions.

Code: [Select]
bool IsCommonBuilding(map_id buildingType)
{
return IsBuilding(buildingType) && !IsEdenOnlyBuilding(buildingType) && !IsPlymouthOnlyBuilding(buildingType);
}

bool IsCommonVehicle(map_id vehicleType)
{
return IsVehicle(vehicleType) && !IsEdenOnlyVehicle(vehicleType) && !IsPlymouthOnlyVehicle(vehicleType);
}

Offline Sirbomber

  • Hero Member
  • *****
  • Posts: 3169
    • http://
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #6 on: March 25, 2016, 07:34:11 PM »
You know that IsVehicle and IsBuilding are already members of the Unit class right?  You might want to change the names of these functions to avoid confusion.
"As usual, colonist opinion is split between those who think the plague is a good idea, and those who are dying from it." - Outpost Evening Star

Outpost 2 Coding 101 Tutorials

"Outpost 2: The Campaigns Are Okay, But The Novella Just Flames Everyone" progress:
Campaign 1 - 40%
Campaign 2 - 0%
Etc. - (insert arbitrary value here)%

It could only cost you your life, and you got that for free!

Online Vagabond

  • Sr. Member
  • ****
  • Posts: 336
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #7 on: March 26, 2016, 12:26:18 PM »
Sirbomber, good point.

How about appending ID to the end of all the functions. We leave the original functions without appending them with ID and mark them as deprecated.

Code: [Select]
IsUnitID
IsVehicleID
IsCommonBuildingID
IsPlymouthOnlyBuildingID
IsEdenVehicleID
... etc.

Offline Sirbomber

  • Hero Member
  • *****
  • Posts: 3169
    • http://
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #8 on: March 26, 2016, 05:10:43 PM »
...No.  Why would we do that?  IsBuilding, IsVehicle, etc are members of OP2's Unit class.  As in, when you call (unit).IsBuilding(), you are calling the game's code.  Why would we ever want to deprecate that and force people to use this instead?  What does this accomplish that we aren't already capable of?
"As usual, colonist opinion is split between those who think the plague is a good idea, and those who are dying from it." - Outpost Evening Star

Outpost 2 Coding 101 Tutorials

"Outpost 2: The Campaigns Are Okay, But The Novella Just Flames Everyone" progress:
Campaign 1 - 40%
Campaign 2 - 0%
Etc. - (insert arbitrary value here)%

It could only cost you your life, and you got that for free!

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3775
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #9 on: March 26, 2016, 07:11:59 PM »
Quote
I kept comments since the original author had put comments on their functions.
According to "svn blame" (or "svn praise" for the glass-is-half-full types), you added those comments in revision 1074.

There are functions Unit::IsBuilding and Unit::IsVehicle, but remember these work on already created Unit objects. The new methods work on map_id values, and so don't need a unit to already exist. The new methods provide new functionality, while the old methods are still a handy shortcut for that particular usage.

I'd say don't both bother updating the names to reflect the difference. The compiler can figure out the difference based on function parameters. The programmer cares more about intent, which will be clear anyway. If anything, I think it makes a lot of sense to purposefully choose the same function name.

Online Vagabond

  • Sr. Member
  • ****
  • Posts: 336
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #10 on: March 27, 2016, 10:24:42 AM »
@Hooman,

It doesn't really matter, but I was referring to the function comments added at revision 689 by you in 2010, "Added ColonyType.cpp to project. Includes functions IsEdenOnlyWeapon, IsPlymouthOnlyWeapon, and IsCommonWeapon."

Code: [Select]
// Returns true if and only if the weapon if Eden specific
bool IsEdenOnlyWeapon(map_id weaponType)
{
    ....
}

I moved the comments to the header file declarations instead of leaving them on the function definitions. I probably also killed a compile time efficiency when I added function declarations to the header file, since none of the functions were originally declared in the header file.

@Sirbomber,

I was considering deprecating IsEdenOnlyWeapon, IsPlymouthOnlyWeapon, and IsCommonWeapon and renaming them IsEdenOnlyWeaponID, IsPlymouthOnlyWeaponID, and IsCommonWeaponID. This wouldn't have involved any of the members of the Unit class. I could have been more clear in my last post on what I meant. However, if Hooman is satisfied there isn't enough of a conflict to change them, then perhaps they are best just left alone.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3775
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #11 on: March 29, 2016, 02:55:27 AM »
Ahh. Well, the comments are simple enough they can probably just be removed. They don't say much more beyond what the name of the function says.

Function declarations should go in header files. Function definitions (implementation) shouldn't, unless it's for an inline function, in which case it has to go in a header file.

Bringing in the STL would probably do the most harm to compile time efficiency. Damn STL! :P

Seriously though, I would prefer keeping STL out of header files. Anything importing those header files will also be slow to compile. It's not such an issue to use the STL in an implementation file, since you compile it once and forget about it, but the header files are recompiled for every compilation unit that includes them.

Correctness over efficiency though.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3775
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #12 on: December 27, 2016, 03:27:15 AM »
I was looking over the ColonyType code today, and noticed a couple of things.


Back in revision 1071, an include was added:
Code: [Select]
#include <Outpost2DLL/Outpost2DLL.h>
Meanwhile, the forward declare of the map_id enum was removed:
Code: [Select]
//enum map_id;

I believe this change was related to the addition of the BuildingTypes and VehicleTypes variables (extern std::array<map_id, N>), which were later removed. It may make sense to revert the #include and forward declare changes.

Looking into why this change may have taken place, I noticed ColonyTypes.cpp does not include Outpost2DLL.h, which it probably should. In ColonyTypes.cpp there are references to specific enum values, such as mapLaser, mapRailgun, etc. For this to work, the actual full enum needs to be included to reference those names, rather than just a simple forward declare of the enum. I suspect the include was added to the header file to avoiding a compilation error, when it would have made more sense to add the include to the implementation file. Moving the include to the implementation file can have compile time benefits, as there is less cascading of includes.


The second thing I noticed was the IsWeapon function could make use of first and last indices, rather than calling 3 separate functions to test for each of the values in that range individually. Basically use the same idea behind IsBuilding, and IsVehicle.


A side note, I also noticed use of the map_id:: prefix in some of the new functions. I just wanted to point out using the enum name as a prefix is a recent addition to the C++ standard. One that helps fix some longstanding problems. This is something we should consider using more. Though we can't really fully benefit for things like the map_id enum, since you'd need to declare it as "enum class", which I suspect will lead to link issues. This may be worth a side discussion.

Online Vagabond

  • Sr. Member
  • ****
  • Posts: 336
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #13 on: December 27, 2016, 11:53:44 AM »
I believe this change was related to the addition of the BuildingTypes and VehicleTypes variables (extern std::array<map_id, N>), which were later removed. It may make sense to revert the #include and forward declare changes.

...

I suspect the include was added to the header file to avoiding a compilation error, when it would have made more sense to add the include to the implementation file. Moving the include to the implementation file can have compile time benefits, as there is less cascading of includes.

I was responsible for changing how map_id was declared so many times. I'm having trouble understanding when it is more appropriate to put an include in the .cpp file versus the .h file, which is probably why it ended up in the wrong place. I'm gathering it affects compilation time where it is placed, but I seem to remember seeing them always in the header file in basic code examples.

The second thing I noticed was the IsWeapon function could make use of first and last indices, rather than calling 3 separate functions to test for each of the values in that range individually. Basically use the same idea behind IsBuilding, and IsVehicle.

This sounds good. I'm using this function in RescueEscort to sort through combat vs non-combat units. Here is the proposed change:

Code: [Select]
bool IsWeapon(map_id mapID)
{
// mapAcidCloud and mapEnergyCannon are respectively the first and last weapon indices
return (mapID >= map_id::mapAcidCloud && mapID <= map_id::mapEnergyCannon);
}

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3775
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #14 on: January 02, 2017, 11:30:26 AM »
Quote
I'm having trouble understanding when it is more appropriate to put an include in the .cpp file versus the .h file

If an include is needed for all users of the header file, in particular for the header file to be valid code, then include from the header file, otherwise include from the source file. Here's some more specific examples.

For enums, if you're using the name of the enum only, such as for parameter type declarations of functions, you can just forward declare the enum. If you use the named values of the enum, such as setting a default parameter value, you much include the full enum.
Code: [Select]
enum EnumName;

void SomeFunction(EnumName parameter); // Doesn't need to see full enum. Actual enum values are not a concern here

void SomeFunction(EnumName parameter = enumValueName); // Full include needed, otherwise enumValueName won't be seen

For structs and classes, if you only need the type name to declare pointers to that type, you can just forward declare. If you declare actual instances, you need to include the full struct or class. As a side note, if you declare an instance, the size of that instance needs to be known, which means the full definition needs to be known to calculate that size. Side-side note, private fields also need to be seen to calculate a class or struct size, which kind of makes them not entirely private.
Code: [Select]
struct SomeStruct {
  int field1;
  int field2;
};

void SomeFunction(SomeStruct* structParameter); // Forward declare fine, it's just a pointer to some struct, the details of which are not important here
void SomeFunction(SomeStruct& structParameter); // Forward declare fine, same reason

SomeStruct structInstance; // Full include needed, as space must be allocated for this struct, which can't be calculated without knowing all the fields

Code: [Select]
class SomeClass {
  void MemberFunction();
};


void SomeMethod(SomeClass* pointerToClass) // Forward declare fine for this
{
  // ...
  pointerToClass->MemberFunction(); // Full include needed for this, otherwise compiler won't know MemberFunction exists for this class type
}

For functions, you can forward declare any function which is not inline. If you just need access to one function from another file, you can forward declare it, rather than declare it in a header file and include the header. Doing this you can also avoid creating a header file if you're only exporting one function. Function declarations tend to be long though, so unless it's a single function with a short signature, used from only one external file, I don't recommend forward declaring functions.
Code: [Select]
void CreatePreGameDialogue(DialgueData* data); // Just one function, no header file

// ...
{
  // ...
  CreatePreGameDialogue(&data); // Access external function here
}

For global variables, you can forward declare and mark as extern (to prevent redefinition) any variable that is defined in another source file. This is similar to the above concerning functions, in that you'd normally only do this to access a single variable, otherwise it's easier to include a header containing a collection of variables or functions. (I used this technique to import data from BaseData.cpp, making a single declare for the outermost data structure, and avoided using any BaseData.h). This may not work so well if the compiler determines an array size based on declared data, and that size is needed at the point of use.
Code: [Select]
// Declare the StartLocation array in BaseData.cpp. (So it is available in this file).
extern StartLocation startLocation[];

Export int InitProc()
{
  // Randomize starting locations
  RandomizeList(4, startLocation); // Randomize (first) 4 starting locations (Note: 4 is hardcoded here)
  //RandomizeList(AutoSize(startLocation)); // Need compile time array size info for this to work
}
« Last Edit: January 02, 2017, 11:37:25 AM by Hooman »

Online Vagabond

  • Sr. Member
  • ****
  • Posts: 336
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #15 on: January 03, 2017, 11:00:31 PM »
I committed an updated ColonyTypes.h/.cpp file that places the include for Outpost2DLL in the .cpp file with a forward declaration of map_id in the .h file. The IsWeapon() function was updated to the more efficient implementation discussed.

Hooman,

Thank you for the thorough explanation. Much clearer to me now how to implement! I'm still struggling a little to understand why it helps the program compile faster to push the includes into the .cpp file instead of the header file.

I'm guess the .cpp files only have to be recompiled when you change something in them but the .h files are always recompiled? Is this the case because main.cpp only contains an include for colonytypes.h and not colonytypes.cpp? Does this mean whenever main.cpp is compiled, colonytypes.h has to be recompiled but colonytypes.cpp doesn't have to be?

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3775
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #16 on: January 04, 2017, 06:47:32 AM »
You're on the right track. Code in header files must be parsed and compiled possibly many times. The compiler itself doesn't understand "header files". That's a programmer construct to make working with the compiler easier. A compiler will only compile source files. When you #include something, it's like pasting the contents of that file in the place of the #include line. The header files then become the common bits that glue different compilation units together. A compilation unit is basically one invocation of the compiler, which is roughly one source file, and all of it's cascaded includes. Each invocation of the compiler must read and process code from scratch. There is no sharing of compiled information between compilation units.

From this you can infer header files should only contain declarations, and never definitions. If a header file contained any definitions, such as a (non-extern) variable, or a function body, it would be compiled and generated again for each compilation unit that included that header file. This would result in link errors later on due to duplicate definitions.

Hopefully you can also start to see how this affects compilation speed. The header files for Outpost2DLL collectively come to about 52KB of code. Every time you #include Outpost2DLL, you're basically pasting in 52KB of code into your source file, which must then be parsed and passed through the compiler. Also remember that C++ compilers are not typically very fast at parsing and compiling code.

For a more extreme example, consider what happens when you #include <windows.h>, or certain files from the STL. It can be hundreds of kilobytes. If you want to investigate, there are compiler options to output the result of the pre-processor step. This will expand all #includes, and replace all the #defined values. Take a look at the resulting file sizes. That's what the compiler sees and has to process.

An obvious approach to this problem, for compiler writers, is to pre-compile header files, and use the precompiled results, rather than pasting the source text in-place and recompiling. There are complications with this approach. C++ isn't a single language, but rather a layered set of two languages, the pre-processor language, followed by the C++ language. If you change one pre-processor definition (say #define Linux or #define Windows), it can drastically change the source the compiler will see. Most other languages don't have this pre-processor language layered on top, and so they're better able to make use of pre-compiling. They also tend to be better at sharing information between compiled units, and have better module systems.

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 1441
    • LairWorks Entertainment
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #17 on: February 08, 2017, 09:19:09 AM »
There's also the templating language which is turing complete... so there's that too.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 3775
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #18 on: February 17, 2017, 12:37:05 AM »
Hah, yes. Code an exponential algorithm into a header file and #include it. See how long that takes to compile.  :o

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 1441
    • LairWorks Entertainment
Re: Extending ColonyType.h/.cpp from OP2Helper
« Reply #19 on: February 17, 2017, 11:45:38 AM »
About 14 year or so. Or until the stack overflows and you end up with a core dump.