As one who survived the "everything needs to become an object!" era, I've got somewhat of a different view of what "object oriented" means.
Back before it was a thing, people used objects. Objects were blocks of memory.
Functions were the form:
NounVerb( Noun* target, ...);
C-based APIs, Pascal based APIs, and other assorted old school stuff still follow that model. Open up the Windows API and you see FileOpen(), FileClose(), FileWhatever().
The key thing to note that was even though things were "objects", functions were standalone and you needed to properly pair up your data structure with your function name.
Virtual functions existed but were just called function tables or jump tables. They were part of the structure:
typedef struct mystruct {
void (*OnCallback)( const mystruct*, int, int, int );
void (*SomeOtherFuction)(const struct myStruct*);
...
Allocation and initialization was of the form:
ptr = malloc( somesize );
memset( ptr, 0, somesize );
or of the form:
ptr = malloc( somesize );
memcpy( ptr, template, somesize ); /* I still sometimes resent that template became a keyword, copies a pre-initialized structure */
or of the form:
NounInitialize( &ptr ); /* much like a modern constructor, calls malloc and initializes member variables including function pointers */
The key thing to note there is that allocation and initialization required two or more consecutive calls.
When I think "object oriented", I see only a few minor differences between the old and new practices.
First, putting things into classes gives a cleaner interface. You type the name of the object and the associated verb. The object is still passed as the first parameter, but it happens behind your back.
Second, it simplified the dispatch of jump tables with seven little letters: virtual.
Third, it made initialization much more clean and tidy, especially when sizes changed or other features changed.
The biggest benefits of the newer language was encapsulation, meaning everything was bundled together. Some of the better-defined APIs already did that, much like you see in the Windows API following the NounVerb pattern, but badly designed systems needed the kick. Inheritance helped with specialization but in some cases actually hurt things since programmers under the new system were less likely to reuse modular functionality, but most people learned pretty quick. It was really cool that access control could also be added to help reduce the risk of people modifying the structure's internals. Often times members would get names like allocation_size_internal_dont_modify. Of course, the longer names didn't really hit until after the 6-char name limit went away...
So why do I bring all that up?
New programmers who never knew about the older ways of doing things hear the expressions and mantras but think they mean something different. Many of them were applied to the old way of doing things, the new ways automatically do it.
RAII is another oft-discussed thing. A constructor that initializes all values to a known sane value, much like the memset to zero is a perfectly acceptable initialization for the second "I" in RAII. The destructors are automatically called so the cleanup takes place without jumping to a large cleanup block. No need to make multiple trips out to disk or across the network as part of initialization.
Virtual functions are overused by many new programmers. Use them to override behaviors within key functions, don't use them to replace the interface.
And finally, when it comes to the array of structures versus structures of arrays.... they don't matter when it comes to object oriented. That was something occasionally discussed with beginners but never a concern for experienced developers. They knew to encapsulate into classes based on usage because everyone had a rock-solid grounding that the algorithms and data structures are the key elements. Everything else is a tool to support them.
Which you choose (SoA or AoS) should be selected because of usage patterns and performance needs.
If your usage pattern is to have a bunch of freestanding little pieces, and individual sets of data get passed around, created, and destroyed individually, then go with an array of structures, now more commonly an array of object instances or a container class.
If your usage pattern is to have an operation that results in bulk processing and running the arrays, and the individual parts are not used independently, then go with a structure of arrays.
And as for the OP, where you are just trying to put things into little buckets because you think that is how others do it? That's an unnecessary modification to the code by itself. Make changes to add a feature or to remove a feature, or in modifications you do both, but don't just change things for the purpose of changing them alone. If it works, leave it alone.
porting procedural c++ to oo syntax
The things that stand out for redesign in the OP are:
- There's a global (well, file-scope) variable which all of the functions are tightly coupled to. It would be cleaner to pass a CMRec reference to each function instead of the index. (or the entire array if you ever need to access more than one index in a function - I didn't thoroughly analyze it to see if that ever happened or not). If you are INTENTIONALLY using file scope for encapsulation, then this doesn't matter as much.
- There are a large number of parallel x,z operations which could become a lot cleaner to look at with a vector2 struct with some methods/operators.
- The numeric #defines should be changed to consts.
- Change C-style local variable declarations to C++ style define-with-initialization. Why? Keeping all variables in a good state at all times is a good idea, even if it is fairly minor in this case. ex:
Overall, the code isn't really screaming "OOP me!" Just some minor tidying up and it would be fine.
- There's a global (well, file-scope) variable which all of the functions are tightly coupled to. It would be cleaner to pass a CMRec reference to each function instead of the index. (or the entire array if you ever need to access more than one index in a function - I didn't thoroughly analyze it to see if that ever happened or not). If you are INTENTIONALLY using file scope for encapsulation, then this doesn't matter as much.
- There are a large number of parallel x,z operations which could become a lot cleaner to look at with a vector2 struct with some methods/operators.
- The numeric #defines should be changed to consts.
- Change C-style local variable declarations to C++ style define-with-initialization. Why? Keeping all variables in a good state at all times is a good idea, even if it is fairly minor in this case. ex:
int x1,z1,x2,z2,x3,z3;
x1=x-rad;
x2=x+rad;
z1=z-rad;
z2=z+rad;
->
int x1 = x-rad;
int x2 = x+rad;
int z1 = z-rad;
int z2 = z+rad;
Overall, the code isn't really screaming "OOP me!" Just some minor tidying up and it would be fine.
Here's a quick effort at rewriting it to be a bit more 'OO', turning some globals into arguments and killing off pages of code duplication:as a design exercise, i've decided to try porting the procedural c++ code for my new collision maps API to oo syntax.
the procedural code is an array of structs, routines that operate on a single struct, and routines that operate on the array:
namespace CollisionValue
{
enum Type
{
Tree = 1,
Cave = 2,
Rock = 3,
FruitTree = 4,
}
}
struct ItemMapDescriptor
{
struct Item
{
int x, z, radius;
};
int numItems;
Item* items;
int size;
CollisionValue::Type type;
int rarity;
bool checkForWaterCollision;
bool checkForTerrainCollision;
bool checkForStorepitCollision;
};
class World
{
...
public:
bool water_in_way(int mx, int mz, int x, int z);
bool storepit_in_way(int mx, int mz, int x, int z);
};
class CollisionMap
{
const static int s_size = 300;
const static int s_cacheSize = 20;
struct Cache
{
bool active;
int age;
int mx, mz;
int cx, cz;
CollisionValue::Type data[s_size][s_size];
};
Cache m_map[s_cacheSize];
std::bitset<s_cacheSize> m_active;
public:
CollisionMap()
{
memset(&m_map[0], 0, sizeof(Cache)*s_cacheSize);
m_active.reset();
}
void clear_cache(int cache_index)
{
memset(&m_map[cache_index], 0, sizeof(Cache));
m_active.reset(cache_index);
}
void clear_cache()
{
for (int a=0; a!=s_cacheSize; ++a)
clear_cache(a);
}
void activate( int cache_index, int mx, int mz, int cx, int cz);
{
Cache& map = m_map[cache_index];
assert( m_active.test(cache_index) == false );
map.mx=mx;
map.mz=mz;
map.cx=cx;
map.cz=cz;
}
void add(int cache_index, int x, int z, int rad, CollisionValue::Type value)
{
assert( cache_index < s_cacheSize && m_active.test(cache_index) == true );
Cache& map = m_map[cache_index];
int x1=x-rad, x2=x+rad;
int z1=z-rad, z2=z+rad;
x1 = max(0,x1);
z1 = max(0,z1);
x2 = min(s_size-1,x2);
z2 = min(s_size-1,z2);
for(int z=z1; z<=z2; ++z)
{
for(int x=x1; x<=x2; ++x)
map.data[x][z] = value;
}
}
void add_objectmap(int cache_index, int mx, int mz, int px, int pz, const ItemMapDescriptor& itemMap, const World& w)
{
assert( cache_index < s_cacheSize && m_active.test(cache_index) == true );
Cache& map = m_map[cache_index];
// x,z location of the plant in the map sq
// cx,xz is the index of the collsion map the object is in
// x2,z2 is the location of the object in the collision map
// for each object in map, if within CM, add to CM
for( int a=0; a!=itemMap.numItems; ++a )
{
// skip every other rock
if (a%itemMap.rarity != 0)
continue;
// calc item's x z
int x = px*itemMap.size + itemMap.items[a].x;
int z = pz*itemMap.size + itemMap.items[a].z;
// calc CM index cx,cz
// if its not in this CM, skip the item
int cx = x/s_size;
int cz = z/s_size;
if( cx != map.cx ||
cz != map.cz )
continue;
// if violates collision rules for this object type, skip the item
if( (itemMap.checkForWaterCollision && w.water_in_way( mx,mz,x,z)) ||
(itemMap.checkForStorepitCollision && w.storepit_in_way(mx,mz,x,z)) ||
(itemMap.checkForTerrainCollision && w.terrain_in_way( mx,mz,x,z)) )
continue;
// ok, item is in CM index cx,cz
// add it to the CM
// calc x,z coords in the CM (x2,z2)
int x2 = x - cx*s_size;
int z2 = z - cz*s_size;
int radius = itemMap.items[a].radius;
add_(cache_index, x2, z2, radius, itemMap.type);
}
}
void add_objects(int cache_index, int mx, int mz, int cx, int cz, const ItemMapDescriptor& itemMap, const World& w)
{
assert( cache_index < s_cacheSize && m_active.test(cache_index) == true );
int px1,pz1,px2,pz2,px3,pz3;
// px1 etc are begin and end of loops for object maps
int px1 = cx *s_size/itemMap.size;
int px2 = (cx+1)*s_size/itemMap.size;
int pz1 = cz *s_size/itemMap.size;
int pz2 = (cz+1)*s_size/itemMap.size;
for( int pz=pz1; pz<=pz2; ++pz )
{
for( int px=px1; px<=px2; ++px )
{
add_objectmap(cache_index, mx, mz, px, pz, itemMap, w);
}
}
}
void age_all_except(int cache_index)
{
for( int a=0; a!=s_cacheSize; ++a )
{
Cache& map = m_map[a];
if(!m_active.test(a))
continue;
if( a == cache_index )
map.age=0;
else
map.age++;
}
}
typedef void (*CollisionGeneratorFnPtr)(CollisionMap&, int, int, int, int, int, const World&);
int get_cache_index(int mx, int mz, int cx, int cz, CollisionGeneratorFnPtr fnGenerate, const World& w)
{
// first: see if it exists, if so return its index...
for(int a=0; a!=s_cacheSize; ++a)
{
if( !m_active.test(a) ||
m_map[a].mx != mx ||
m_map[a].mz != mz ||
m_map[a].cx != cx ||
m_map[a].cz != cz )
continue;
return a;
}
// doesnt exist. need to make one.
// serch for inactive, if found, use it to generate a CM and return its index...
for(int a=0; a!=s_cacheSize; ++a)
{
if (!m_active.test(a))
{
fnGenerate(*this, a, mx, mz, cx, cz, w);
return a;
}
}
// didnt find inactive CM. must use LRU one.
// find LRU CM, use it to gen new CM, and return its index...
int best=0;
int bestval=0;
for(int a=0; a!=s_cacheSize; ++a)
{
if (m_map[a].age > bestval)
{
best = a;
bestval = m_map[a].age;
}
}
fnGenerate(*this, best, mx, mz, cx, cz, w);
return best;
}
}
void gen_CM( CollisionMap& cm, int cache_index, int mx, int mz, int cx, int cz, const World& w )
{
// clear the collision map
cm.clear_cache(cache_index);
// helps if you make it active, etc! also - cx,cz must be set to add plantmaps (trees, rocks, fruit trees)!
cm.activate(cache_index, mx, mz, cx, cz);
// add trees as required...
switch (g_map[mx][mz].coverage)
{
case WOODS:
case JUNGLE:
cm.add_objects(cache_index, mx, mz, cx, cz, g_jungleTreeDesc, w);
break;
case SAVANNA:
cm.add_objects(cache_index, mx, mz, cx, cz, g_savannaTreeDesc, w);
break;
}
cm.add_objects(cache_index, mx, mz, cx, cz, g_caveDesc, w);
if (g_map[mx][mz].rocks)
cm.add_objects(cache_index, mx, mz, cx, cz, g_rockDesc, w);
if (g_map[mx][mz].fruittree)
cm.add_objects(cache_index, mx, mz, cx, cz, g_fruitTreeDesc, w);
if (g_map[mx][mz].cavern)
cm.add_objects(cache_index, mx, mz, cx, cz, g_cavernDesc, w);
cm.add_objects(cache_index, mx, mz, cx, cz, g_rocksheltersDesc, w);
if (map[mx][mz].volcano)
cm.add_objects(cache_index, mx, mz, cx, cz, g_volcanoDesc, w);
add_volcano_to_CM(i,cx,cz);
cm.add_objects(cache_index, mx, mz, cx, cz, g_hutsDesc, w);
cm.add_objects(cache_index, mx, mz, cx, cz, g_permsheltersDesc, w);
cm.add_objects(cache_index, mx, mz, cx, cz, g_tempshelters_Desc, w);
}//todo - fix all those globals^
.......
CollisionMap cm;
int mx=0, mz=0, cx=0, cz=0;
int idx = cm.get_cache_index(mx, mz, cx, cz, gen_CM, g_world);
Author
Here's a quick effort at rewriting it to be a bit more 'OO', turning some globals into arguments and killing off pages of code duplication:
i like the way you rolled the three versions of "add_some_plantmap_to_collision_map" into one. i just did a quick and dirty copy/paste/edit to create the three routines quickly.
i notice that you put it all (the cache and maps) in one big class.
before checking the latest responses to this post, i was thinking that one would have 4 classes: a collision map class, a cache class (IE data type objects), and two type of "controller" objects (objects that contain primarily controller code and use two or more different data objects): a "collision checker" and a "collision map generator". the collision checker would use the cache and maps. the collision map generator would use a collision map, and the world map data structures.
That code could be a lot simpler (and more readable) if you took just a bit more care when writing it.
no doubt! note that the posted code is output from a code generator - the source script is not nearly as long (or as ugly).
As one who survived the "everything needs to become an object!" era, I've got somewhat of a different view of what "object oriented" means.
much of what you say brings back fond (and not so) memories for me! <g>.
usage pattern would indicate that an array of objects is desired. most usage is on an individual map. only searches and aging operate on the entire cache.
And as for the OP, where you are just trying to put things into little buckets because you think that is how others do it? That's an unnecessary modification to the code by itself. Make changes to add a feature or to remove a feature, or in modifications you do both, but don't just change things for the purpose of changing them alone. If it works, leave it alone.
as i said, design exercise only. the code in the game is still as originally posted. about the only thing i did was #define the magic numbers used to represent tree, rock etc. Note that in the original code posted, the value 7 is used for both volcano and something else (by accident - a BUG!). however it has no effect, as collision checks are simply go/no go, and don't care what value is at the collision point (only that its non-zero).
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
