Adding a new scripting command

[Juutis]

I'm trying to add a new scripting command that returns the upper limit of an attribute. Here's what I did:
In CPawnLow.cpp, ScriptedObject::lowmethod, added:

Code: Select all

	case RGF_SM_GETATTRIBUTEHIGH:
		{
			// USAGE:	GetAttributeHigh(char* Attribute), 
			//			GetAttributeHigh(char* Attribute, char* EntityName)
			
			CPersistentAttributes *theInv;

			if(arguments.entries() > 1)
			{
				strcpy(param0, arguments[1].str());

				if(!stricmp(param0, "Player"))
					theInv = CCD->ActorManager()->Inventory(CCD->Player()->GetActor());
				else
					theInv = CCD->ActorManager()->Inventory(CCD->ActorManager()->GetByEntityName(param0));
			}
			else
				theInv = CCD->ActorManager()->Inventory(Actor);
		
			returnValue = theInv->High(arguments[0].str().c_str());
			return true;

		}

In CCommonData::FillHashMethods, added:

Code: Select all

	AddHashMethod("GetAttributeHigh",		RGF_SM_GETATTRIBUTEHIGH);

And in RFGScriptMethods added:

Code: Select all

       ...
	//RGF_SM_PITCH_SPEED,				// same as readable
	//RGF_SM_IDEAL_PITCH,				// same as readable
	RGF_SM_GETATTRIBUTEHIGH,
	
	RGF_SM_MAXMETHODS

It compiles fine but it seems unstable. When I run the program it often crashes either after the Genesis3D logo or after I click 'New Game' leaving nothing special in the logs. It usually works if I run it as a preview from the editor. I'm not very familiar with the RF source code, and my C++ ain't the best either. So is there anything special I should know when adding scripting commands?

I tried debugging it with VC++ 2008 Express (Debug -> Start Debugging) and it couldn't find any of the .ini files although there was a fresh RF install in my working directory.

I'm also using Federico's hacked NearestPoint. The original RF076 and the version with Federico's hack but without my addition both work fine.

Any help is greatly appreciated.

[bernie]

case RGF_SM_GETATTRIBUTEHIGH:
{
// USAGE: GetAttributeHigh(char* Attribute),
// GetAttributeHigh(char* Attribute, char* EntityName)

CPersistentAttributes *theInv;

if(arguments.entries() > 1)
{
strcpy(param0, arguments[1].str());

if(!stricmp(param0, "Player"))
theInv = CCD->ActorManager()->Inventory(CCD->Player()->GetActor());
else
theInv = CCD->ActorManager()->Inventory(CCD->ActorManager()->GetByEntityName(param0));
}
else
theInv = CCD->ActorManager()->Inventory(Actor);

returnValue = theInv->High(arguments[0].str().c_str());
return true;

}

you are missing chain brackets in the if and else statements;

Code: Select all

   case RGF_SM_GETATTRIBUTEHIGH:
      {
         // USAGE:   GetAttributeHigh(char* Attribute), 
         //         GetAttributeHigh(char* Attribute, char* EntityName)
         
         CPersistentAttributes *theInv;

         if(arguments.entries() > 1)
         {
            strcpy(param0, arguments[1].str());

            if(!stricmp(param0, "Player"))
      {
               theInv = CCD->ActorManager()->Inventory(CCD->Player()->GetActor());
      }
            else
     {
               theInv = CCD->ActorManager()->Inventory(CCD->ActorManager()->GetByEntityName(param0));
    }
         }
         else
    {
            theInv = CCD->ActorManager()->Inventory(Actor);
    }
      
         returnValue = theInv->High(arguments[0].str().c_str());
         return true;

      }

Not sure about the last bracket or whether it should come just before the return true;

[Juutis]

The brackets are fine. If there's only one expression after an if or else statement there's no need for brackets. Anyway, the code compiles fine so it's not the syntax.

[Jay]

I am not sure, but are you really sure that the entity from which you want to get the high amount of the attribute actually has the attribute BEFORE you use the command? (In earlier versions i think it could cause a crash if you wanted to get or set the value of an attribute if it didn't exist on that entity) Or don't you use the command at all?

Also, this line:

returnValue = theInv->High(arguments[0].str().c_str());

could be changed to

returnValue = (int)theInv->High(arguments[0].str().c_str());

to make sure the function returns the integer portion of the attribute (it could somehow confuse the compiler and it gives the return value a float for example, not sure how to explain this...)

In the official RF source code QoD does this with the SetAttribute and GetAttribute commands, so i guess it should be in your attribute command too, if only to be conform with the official source code.

[Juutis]

I am not sure, but are you really sure that the entity from which you want to get the high amount of the attribute actually has the attribute BEFORE you use the command? (In earlier versions i think it could cause a crash if you wanted to get or set the value of an attribute if it didn't exist on that entity) Or don't you use the command at all?

I don't think that's the problem since my code is almost identical to the code for GetAttribute. The only difference is I'm calling CPersistentAttributes::High and the GetAttribute code calls CPersistentAttributes::Value which again are identical methods, only that High returns the upper limit for the attribute and Value returns the value.

to make sure the function returns the integer portion of the attribute (it could somehow confuse the compiler and it gives the return value a float for example, not sure how to explain this...)

In the official RF source code QoD does this with the SetAttribute and GetAttribute commands, so i guess it should be in your attribute command too, if only to be conform with the official source code.

Didn't help, but you're right. It's good to have that even if it's only for the readability.


Did some more tests and I found out that it's the code coupled with my game content that's crashing it. I used the modified .exe with the default 076 content and it worked fine, even if I used the GetAttributeHigh() command in scripts. So:
1) The official 076 executable works perfectly.
2) The executable with the NearestPoint modification from Federico works perfectly too.
3) My executable crashes my game, but not the RF demo game... I really hope I don't have to go figure it out with trial and error. It would be hell of a job to test all the stuff that's in my game one by one.

O' well, I guess I'll head to bed. Maybe tomorrow is a better day.

[QuestOfDreams]

I haven't tested your code but it looks completely valid to me. For debugging you must set up the working directory correctly. Open the Property Pages (Project->RGF Properties...) and fill in the Working Directory in Configuration Properties->Debugging (at least that's where it is in VC++ 2005 but it should be the same in VC++ 2008 ...). As RF runs pretty slow in Debug mode, I usually use CCD->ReportError calls around the code in question for debugging.

[paradoxnj]

Also, this line:

returnValue = theInv->High(arguments[0].str().c_str());

could be changed to

returnValue = (int)theInv->High(arguments[0].str().c_str());

Jay, it should be:

Code: Select all

returnValue = atoi(theInv->High(arguments[0].str().c_str()));

A string does not cast to an int. It needs to be converted.

Juutis, are you running this using the debugger (the green arrow in the toolbar) or just double-clicking the EXE? If you are running using the debugger, you have to set the working directory in the project options. That may explain why it cannot find the INI files.

Bernie, one line if statements do not require brackets. You can use them if you want. It's a programmer's preference. Only if the conditions contain multiple lines do you require brackets.

Example:

Code: Select all

// Valid
if (!some_var)
    return;

// Also valid
if (!some_var)
{
    DoSomething();
    DoSomethingElse();
    return;
}

[Jay]

@paradoxnj:

CPersistentAttributes::High(const char *szTag) returns an int, not a char* (i was sure it returned a number, but not which one. I just looked at the source again.). I have no intention to use the address of a string in scripts. . The reason why i wrote this was mainly because QoD used the implicit int conversion to make it clear that the script command always returns an integer value. The new command would be conform with this.

[paradoxnj]

My apologies...I saw .c_str() at the end of that and was lost in the excess brackets. Even though...you shouldn't use C-style casts. Use static_cast<int> to cast the integer. It's more efficient.

The reason why i wrote this was mainly because QoD used the implicit int conversion to make it clear that the script command always returns an integer value.

No offense to QoD, but this is inefficient. It's an unnecessary cast. Casting takes away FPS. I can bet that if you change all the C-Style casts to static_cast<> you will gain FPS. In addition, you will also gain some FPS if you stop the unnecessary casts. You should run the code through a profiler tool to show you the difference. Here are some free ones:

http://developer.amd.com/cpu/CodeAnalys ... fault.aspx
http://sourceforge.net/projects/shinyprofiler
http://www.codersnotes.com/sleepy
http://www.codeproject.com/KB/cpp/Profi ... indow.aspx

The reason you give is exactly what Hungarian Notation is for. If the function was iHigh(), you would know it returns an integer.

[Juutis]

Juutis, are you running this using the debugger (the green arrow in the toolbar) or just double-clicking the EXE? If you are running using the debugger, you have to set the working directory in the project options. That may explain why it cannot find the INI files.

I used the debugger.

I set the working directory (thanks QoD) and it ran. I got the following errors:

Assertion failed!

Program: C:\Nirhis\RF\test\RealityFactoryD.exe
File: C:\RF_FINAL\Genesis_02052007\GenesisLib\A...\actor.c
Line: 1115

Expression: geExtBox_IsValid(ExtBox) != GE_FALSE

For information on how your program can cause an assertion failure, see the Visual C++ documentation on asserts.

(Press Retry to debug the application - JIT must be enabled)

Assertion failed!

Program: C:\Nirhis\RF\test\RealityFactoryD.exe
File: C:\RF_FINAL\Genesis_02052007\GenesisLib\A...\actor.c
Line: 1159

Expression: Box->Max.X >= Box->Min.X

...

Assertion failed!

Program: C:\Nirhis\RF\test\RealityFactoryD.exe
File: C:\RF_FINAL\Genesis_02052007\GenesisLib\A...\ExtBox.c
Line: 186

Expression: geExtBox_IsValid(B) != GE_FALSE

...

And AFAIK, these have nothing to do with my code. So I guess I'll start looking for the problem in my levels and other content.

[paradoxnj]

It looks like you didn't set a bounding box to its minimum values.

[QuestOfDreams]

My apologies...I saw .c_str() at the end of that and was lost in the excess brackets. Even though...you shouldn't use C-style casts. Use static_cast<int> to cast the integer. It's more efficient.

The reason why i wrote this was mainly because QoD used the implicit int conversion to make it clear that the script command always returns an integer value.

No offense to QoD, but this is inefficient. It's an unnecessary cast. Casting takes away FPS. I can bet that if you change all the C-Style casts to static_cast<> you will gain FPS. In addition, you will also gain some FPS if you stop the unnecessary casts.

Casting is a compile time concept, the code produced should be the same, no matter if you use C or C++ casts. However, C-style casts can do different types of cast, i.e. static_cast, const_cast, reinterpret_cast, so it's safer to use C++ casts and they stand out more in your code (so casts are easier to find and debug).
Unneeded casts should be removed of course, but please remember that the RF code was written by several people, not just me. I try to improve the code with every new version (e.g., I've done major work on const-correctness in the last version) but that is a tedious task.

The reason you give is exactly what Hungarian Notation is for. If the function was iHigh(), you would know it returns an integer.

If you do it right, then I agree that Hungarian notation can be useful, but most people don't. Also IDE functions like IntelliSense make it less important.

@Juutis
For debugging purposes, the function call stack would be more interesting

[paradoxnj]

Don't get me wrong....I wasn't saying it was you. I said "no offense" because you are currently maintaining the code and I know that you did not wrote most of it. I guess I should have used better wording...

While I agree it should produce the same ASM...it also can improve your performance to use the C++ _cast<> templates. Reason being is that you can control the type of cast that you make. It also helps make code more readable.

Here's a little excerpt from AT&T about it.

What good is static_cast?
Casts are generally best avoided. With the exception of dynamic_cast, their use implies the possibility of a type error or the truncation of a numeric value. Even an innocent-looking cast can become a serious problem if, during development or maintenance, one of the types involved is changed. For example, what does this mean?:
x = (T)y;

We don't know. It depends on the type T and the types of x and y. T could be the name of a class, a typedef, or maybe a template parameter. Maybe x and y are scalar variables and (T) represents a value conversion. Maybe x is of a class derived from y's class and (T) is a downcast. Maybe x and y are unrelated pointer types. Because the C-style cast (T) can be used to express many logically different operations, the compiler has only the barest chance to catch misuses. For the same reason, a programmer may not know exactly what a cast does. This is sometimes considered an advantage by novice programmers and is a source of subtle errors when the novice guessed wrong.
The "new-style casts" were introduced to give programmers a chance to state their intentions more clearly and for the compiler to catch more errors. For example:

int a = 7;
double* p1 = (double*) &a; // ok (but a is not a double)
double* p2 = static_cast<double*>(&a); // error
double* p2 = reinterpret_cast<double*>(&a); // ok: I really mean it

const int c = 7;
int* q1 = &c; // error
int* q2 = (int*)&c; // ok (but *q2=2; is still invalid code and may fail)
int* q3 = static_cast<int*>(&c); // error: static_cast doesn't cast away const
int* q4 = const_cast<int*>(&c); // I really mean it

The idea is that conversions allowed by static_cast are somewhat less likely to lead to errors than those that require reinterpret_cast. In principle, it is possible to use the result of a static_cast without casting it back to its original type, whereas you should always cast the result of a reinterpret_cast back to its original type before using it to ensure portability.
A secondary reason for introducing the new-style cast was that C-style casts are very hard to spot in a program. For example, you can't conveniently search for casts using an ordinary editor or word processor. This near-invisibility of C-style casts is especially unfortunate because they are so potentially damaging. An ugly operation should have an ugly syntactic form. That observation was part of the reason for choosing the syntax for the new-style casts. A further reason was for the new-style casts to match the template notation, so that programmers can write their own casts, especially run-time checked casts.

Maybe, because static_cast is so ugly and so relatively hard to type, you're more likely to think twice before using one? That would be good, because casts really are mostly avoidable in modern C++.