Interface Redesign

[zenzike]

how should i convert float3 <-> SAIFloat?

You don't need to: inheritence means that you can make sure the type is an SAIFloat for any data you're exporting from the engine.

If you're pushing data from the AI to the engine that's an SAIFloat, then you can instantiate the float3 using the default constructor. Look at my Event code to see how that's done.

[hoijui]

ahh ok.. sorry, i did not even see that it is using function pointers :/
well.. that seems like an ok way for C and C++, but other languages have no support for function pointers (or it may get difficult to convert C function pointers to.. eg C# ones).

but we could do this:
The AI instantiates an SAICallback with default implementations for all function pointers (returning 0 for getUnitMorale() eg.). then the AI passes this SAICallback to the engine, which fills it with the real implementations.
this way we get the pro of your way and are friendly to other languages.

about SAIFloat3:
you mean, we should make float3 inherit from SAIFloat3? or what did you mean with the inheritance?
for AI->Engine: using the default constructor means instantiating a new object/memory. casting would prevent that, and possibly be more performant. it should work, as float3 is used as a value type everywhere, and usually not passed around with a pointer (except when it is const), and so we would not need a new instance. but then again, it won't make a big difference. i just though the plan was to cast, not to (re-)instantiate.

[hoijui]

thinking about it...
i think your approach with getFunction() is technically not possible. in the example with getMorale(int teamId, int unitId), the engine would have to return a function pointer like this:

Code: Select all

float (*someName)(int, int)

but the engine does not know what arguments or return value are needed, and even if, i think it is not possible to create a function at runtime, is it?

maybe i am wrong. this is the first time i am using function pointers in practice. sorry then

[zenzike]

thinking about it...
i think your approach with getFunction() is technically not possible.

Actually, it's used in the code I uploaded -- function pointers are required to expose the library interface.

Your argument about other languages isn't quite applicable; there would always need to be an interface wrapper written in C/C++ that exposes the interface to whatever language you want to export to. For example, JAI needed some C++ code to act as an adaptor between the interface and JAVA. The wrapper will always be able to understand function pointers since it's written in C/C++.

Your solution with a callback struct just introduces version dependencies -- it doesn't solve the problem that getFunction() is trying to solve.

[hoijui]

thinking about it...
i think your approach with getFunction() is technically not possible.

Actually, it's used in the code I uploaded -- function pointers are required to expose the library interface.

Could you please give some more information, where exactly is it used?

i was having lunch, and though about it again. it surely is possible (i don't know how you did it/how you want to do it, but the AI could set a default implementation before requesting the function from the engine, and if the engine does not change the pointer this one is used. that would be the same mechanism as i suggested to use for the function pointers (form the old idea).

Your argument about other languages isn't quite applicable; there would always need to be an interface wrapper written in C/C++ that exposes the interface to whatever language you want to export to. For example, JAI needed some C++ code to act as an adaptor between the interface and JAVA. The wrapper will always be able to understand function pointers since it's written in C/C++.

It is true that we always need a wrapper, an right.. it is not as bad as i though, but it is not true that my argument is not applicable at all.
with getFunction(), any non native language will need more native code to interface with the C interface, compared with when we use the Callback struct. The nice modularity we gain, is only available in native interfaces, and can not be used in Java for example, as we have to map each callback-"Event" to a single function in native code, before we can call it.

Your solution with a callback struct just introduces version dependencies -- it doesn't solve the problem that getFunction() is trying to solve.

Aehm.. could you explain that? why would it introduce version dependencies? It would solve the same problem with the same method -> the AI provides default implementations for the functions.

[AF]

Regarding SAIFloat3 or w/e its being called now:

• Theres talk of inheritance here yet C does not support inheritence because there are no classes in C only structs. Ideally we should be able to put this in a pure C AI.
• If class B is derived from class A, I can cast pointers of type A to B. But I dont think I could create an object of type A and cast it to type B because its not an object of type B so making float3 inherit from SAIFloat3 then converting by casting shouldnt work.
  Eitherway wether Im right or wrong its a hackish kludge
• Why dont we just pass it in as a constructor parameter?
  

  Code: Select all

  SAIFloat3 aifloat3;
  float3 float((float*)aifloat3);
  // etc...
  

Now I can see zenzikes method but let me just make it clear what I think your two methods are because its getting murky and all this discussion is compounding any confusion there already is.

hmmm I tried summarizing your method hoiju only to realize I don't actually know what it is =s. Can both of you summarize? State it without describing its advantages or disadvantages or reasoning, just nice and clear

[hoijui]

ok...

remember: float3 inherits from SFloat3 (both are classes). SFloat3 has the following data/variables:

Code: Select all

float x, y, z;

float3 its self has none (except those inherited ones).

my stuff is purely C.

we have

Code: Select all

struct SAIFloat3 {
   float x, y, z;
}

converting:
option 1:

Code: Select all

float3 f3;
SAIFloat3 aif3;
f3 = reinterpret_cast<float3>(aif3);
aif3 = reinterpret_cast<SAIFloat3>(f3);

short description of reinterpret_cast: http://www.cprogramming.com/reference/t ... tcast.html

option 2:

Code: Select all

float3 SAIFloat3_to_float3(SAIFloat3 aif3) {
   float3 f3;
   f3.x = aif3.x;
   f3.y = aif3.y;
   f3.z = aif3.z;
   return f3;
}
SAIFloat3 SAIFloat3_to_float3(float3 f3) {
   SAIFloat3 aif3;
   aif3.x = f3.x;
   aif3.y = f3.y;
   aif3.z = f3.z;
   return aif3;
}

side note (has nothing to do with my solution):
if we had SAIFloat3 as a pure C struct, and float3 (or SFloat3, which is the base class of float3) would inherit from SAIFloat3, we could safely cast a float3 to a SAIFloat3, which can be used in the C interface.

[AF]

Code: Select all

const char* myVeryImportantString = "super duper bannana lamb";
float3 f = reinterpret_cast<float3>(myVeryImportantString );

That method of casting sounds inherently dangerous, and I seriously doubt it will actually work as intended without dangerous consequences.

It should not be used to cast down a class hierarchy or to remove the const or volatile qualifiers.

For the reinterpret cast to work, the engine would need to create a float3, pass it as an SAIFlaot3 pointer to the AI and then the AI recast to float3. This would reintroduce C++ ABI compatibility, and it would require SAIFloat3 to be present in the engine and for the engine version of float3 to be identical to the AI version.

Basically:

• No inheritance between float3 or SAIFloat3. SAIFloat3 is a structure, it does not inherit from anything and it is not inherited by anything else. It is a C structure and C has no inheritance and classes.
• reinterpret casts should be used as a last resort, they can be dangerous. When used they should be used carefully in accordance with recommendations.
  
  I also doubt the need for inheritance of float3 from the float3 AI struct.
  

  Code: Select all

  float3::float3(SAIFloat3 f){
      x = f.x;
      y = f.y;
      z = f.z;
  }

  or even:
  

  Code: Select all

  float3* pos = new float3((float*)myaifloat3);

  The struct should be castable to a float* array, we cna just pass it as a constructor parameter.
  
  Also of note is that float3 is a value, and as such when creating the float3 we are copying the SAIFloat3s values. Changing the original struct should not change the float3 and vice versa.

[hoijui]

we had this discussion already. the only dangerous thing that could cause problems with this:
we have:

Code: Select all

struct SAIFloat3 {
   float x, y, z;
}

MinGW saves an instance of SAIFloat3 in memory like this 'xyz' and VC saves it like this 'zyx'. there is nothing else that could cause problems here, and btw:

float3* pos = new float3((float*)myaifloat3);

this solution would have the exact same problem.

if i remember right, tobi said that all compilers safe member values of structs in the same way as with classes, and he said it should be safe to cast because of that. it makes huge sense, if you think of it:
all compilers save struct variables in the same way, as structs are C.
a class can inherit from a struct, and therefore can not use an other scheme as the structs in the in the same compiler. therefore, classes in all C++ compilers have to safe their member variables in the same order, in memory.
as SAIFloat3, SFloat3 and float3 all have these, and only these member variables:

Code: Select all

float x, y, z;

it should be safe to cast SAIFloat3 to float3 to SFloat3. as they all look equal, memory wise. we know what we are doing. i see no problem in casting.

an other question:
as we usually pass float3's around as values, it means, a new copy is created, whenever we pass a float3 to a function?

[AF]

I still think we should not use inheritance and non C features inside a C API. Nor should we resort to reinterpret_Cast, it's kludgy.

Remember C has no reinterpret_cast or inheritance, those are C++ features. Its best we keep only C language stuff inside the C API for now. While the cast might 'work' its not good programming, and however correct our assumptions of compiler memory internal are, they're still assumptions, so lets do things the safer way.

tbh I still don't see why we don't just pass float* and pass it into a float3 structure on the other side. I'd have thought that would have made bindings to other languages easier too.

[hoijui]

both things, the inheritance and the reinterpret cast would not be part of the interface, they would be part of the internal implementation of the interface. the interface consists of the header files, and is pure C.
if we used inheritance eg, SAIFloat3 would be a pure C struct in its own header file, and float3 would inherit from it (-> C++), the implementation of the engine side C interface contains lots of C++ stuff. same reasoning applies for reinterpret cast.

but.. lets forget about it, i will just do it the way you want it, i already started doing it that way anyway.

[AF]

I say it partially because float3 is not in the C API domain but the C+ OO SDK, and as such it may change beyond what the engine has.

An example being listeners that fire events when the values change to listeners etc.

[hoijui]

i am running into ugly problems.

it is one kind of problem. i'll try to explain with an example. it is about passing a WeaponDef from the engine to the AI through the callback. the problem shows when recreating the C++ classes.

lets start at the engine side, where there is no problem:

Code: Select all

float WeaponDef_getAccuracy(int teamId, int weaponDefId) {
   IAICallback clb = team_callback[teamId];
   return clb->GetWeaponDef(weaponDefId)->accuracy;
}
float* WeaponDef_Damages_getDamages(int teamId, int weaponDefId) {
   IAICallback clb = team_callback[teamId];
   return clb->GetWeaponDef(weaponDefId)->damages->impulseFactor;
}
.
.
.

the problem comes on the AI side, in the C++ wrapper:

Code: Select all

WeaponDef* CAIAICallback::GetWeaponDef(int weaponDefId) {
   WeaponDef* weaponDef = new WeaponDef();
   weaponDef->range = sAICallback->WeaponDef_getRange(teamId, weaponDefId);
   weaponDef->damages = DamageArray();
   weaponDef->damages.damages = sAICallback->WeaponDef_Damages_getDamages(teamId, weaponDefId);
   return weaponDef;
}

As the AI has no access to DamageArray.o i get a linking error at these lines:

Code: Select all

   WeaponDef* weaponDef = new WeaponDef();
   weaponDef->damages = DamageArray();

and even if this problem were solved, the next line would fail, as damages has private access in DamageArray.

Code: Select all

   weaponDef->damages.damages = sAICallback->WeaponDef_Damages_getDamages(teamId, weaponDefId);

the only way i see to solve this, is:
creating an IDamageArray, which has functions to fetch the private members. using the current DamageArray as implementation everywhere in the engine, and IDamageArray as interface. creating an AIDamageArray implementation which has methods to set the private members. this implementation would be used in the C++ AI wrapper.

this solution requires using IDamageArray everywhere in the code, where we currently use only DamageArray.
i guess more of these sort of problems will come up, when i go on with FeatureDef and especially with UnitDef.

any other suggestions?

[hoijui]

an other option would be, to put everything into DamageArray.h, the implementation of the constructors, destructors, and the getters and setters for the private members.
maybe not so nice, but surely much less work, and much less invasive (less engine code changes).

[hoijui]

problem solved, WeaponDef is passes sucessfully throug the C interface now. working on FeatureDef, then going to UnitDef (largest).

[hoijui]

about half of callback is now ported to C. yet missing (done with the old C++ interface still): getUnitDef() & everything that changes game state.
i can let it run, but it crashes after about 15 minutes. maybe a pointer issue caused by me, or an other error with the current SVN.

somethign else:
there is a problem in the way the current C++ AIs (can) use the interface. let me show it on an example:
IAICallback:

Code: Select all

const CCommandQueue* GetCurrentUnitCommands(int unitid)

in some AIs code:

Code: Select all

for (int i=0; i < cb->GetCurrentUnitCommands(unitId)->size(); ++i) {
const Command* c = cb->GetCurrentUnitCommands(unitId)->at(i);
...

this was ok with the old C++ interface, but now: whenever the AI calls GetCurrentUnitCommands(unitId), the C++ interface wrapper has to recreate the CCommandQueue behind the scenes, over the C interface with lots of calls to the engine.

i see 3 options to solve these kind of problems:
* the AIs themself are held responsible to cash everything that can be cashes as long as it is valid. in the case of the example, the AI can be sure of that the CCommandQueue it fetches stays valid during a frame.
* the C++ wrapper is responsible for caching (big objects would be cached together with the frame they were created in), using lazy initialization.
* we use a push rather then a pull strategy for such objects, as zenzike once said. the engine knows best how long an object stays valid.

i am pro the third variant, because:
a lot of things may stay valid much longer then the AI or the wrapper can be sure of. eg, in the example used here: a units commands will usually not change every frame, and therefore we could save lots of calls from the AI to the engine.

[hoijui]

applies to:

Code: Select all

class IAICallback
{
public
	virtual const std::vector<CommandDescription>* GetGroupCommands(int unitid) = 0;
	virtual const std::vector<CommandDescription>* GetUnitCommands(int unitid) = 0;
	virtual const CCommandQueue* GetCurrentUnitCommands(int unitid) = 0;
	virtual const UnitDef* GetUnitDef(const char* unitName) = 0;
//	virtual void GetUnitDefList(const UnitDef** list) = 0;
	virtual const FeatureDef* GetFeatureDef(int feature) = 0;
	virtual const WeaponDef* GetWeapon(const char* weaponname) = 0;
};

plus the equivalen functions in IAICheats (not all are present there)

for pull style, we would need a ChangedEvent for each of thse functions, respectively for the values they return. the problem there woudl be, to ensure these events are thrown correctly, which is whenever something changes in those structs and classes.

[Hoi]

So I give you the current itnerface, BEHOLD!


(click to enlarge)

And here is what I propose:



This should outline a basic C++ Interface, highly generic, and easy to create bindings for. The matter of creating a C API is purely negotiating the communication between CAIObject instances across the library boundary. This can be done by changing the IAIObject class that the AIs inherit from into a basic wrapper class around which a C API can be built.

For backwards compatibility, the existing interface can be hoisted up as a wrapper using the old code to implement its functionality. As the new interface is built we can change the implementation of the compatibility layer and eventually remove the old interface completely. This way the process can be done gradually over a long period of time should there be periods where no developers are able to contribute to the effort.

While Ive started this thread I must stress that I do not have the time or means to do this single handedly, nor do I have the resources to spearhead as a leader by example. I will help out but I cant push this forward implementation-wise as I have my lobby project and other projects to think about.

Having said that, I've kept in mind that what I've proposed could be done piece by piece slowly over a very long period, possibly multiple spring releases. The advantages are that we could use a lot of new languages that compile into native code, and adding new language bindings would be far easier, and we could use any compiler we wanted, and there would only be one interface in the end to maintain and a set standard method of updating and adding to it.

i have no idea what this means, but looking at the first pic and the 2th i think that less boxes is better, so this would be great

[AF]

Currently this thread has moved towards defining and implementing the content of the green spring box.

[hoijui]

UnitDef now comes through the C interface as well. the legacy C++ interface wrapper just gets all the *Def structs through the C interface once and caches them. some bugs fixed. XTA and RAI do not crash (at least not for an hour). i am now working on the command stuff, which is everything in the AI callback that changes engine state. so far, commands were just commands for Units and Groups, now it will be more, eg: DrawLine on the battlefield, callLUARules, CreateGroup, ...

the question is, if we want one C interface Command for each unit command, like this:

Code: Select all

struct SMoveUnitCommand { int unitId, ...};
struct SBuildUnitCommand { int unitId, ...};
struct SAttackUnitCommand { int unitId, ...};
struct SCustomUnitCommand { int unitId, int cmdId, ...};

or have a single UnitCommand like this:

Code: Select all

struct SUnitCommand { int unitId, int cmdId, ...};

i would prefer the first, but it is not so nice because of custom commands. a custom command could be a very special command that exists only in one mod (eg. melt-together-with-other-unit). I will use the first way anyway, but if anyone thinks it is bad that way, give input, and we can have a look at it.