TGE 1.3 errors...

#21 BTW, neat fix. I'm putting it on my list.

#22 Can you explain this ? Is it just me or these 2 lines (bold & italic) should be inverted ?

void ParticleEmitterNode::setEmitterDataBlock(ParticleEmitterData* data)
{
   if (!data)
      return;

   if (mEmitter)
   {
      mEmitterDatablock = data;
      ParticleEmitter* pEmitter = new ParticleEmitter;
      pEmitter->onNewDataBlock(mEmitterDatablock);
      if (pEmitter->registerObject() == false) {
         Con::warnf(ConsoleLogEntry::General, "Could not register base emitter for particle of class: %s", mDataBlock->getName());
         delete pEmitter;
         pEmitter = NULL;
      }
      if (pEmitter)
      {
[b][i]         mEmitter->deleteWhenEmpty();
         mEmitter = pEmitter;[/i][/b]
      }
   }
}

#23 In our project we use also emitter to simulate waterfall. So when I quit the application I still get the "out of the bins" error message. But if I wait a while before closing the game after being out of the mission, I don't get any message. So my bet is that the particles are still live in the system and continue to be until they die naturally. It's just the matter to wait a few seconds and miracle no warning messages.

So I am wondering if this it the missing "DeleteWhenEmpty" problem or simply that the system miss to properly kill the particle objects properly when closing mission.

Edited:

Ok, I did some debugging, the particleEmitter object is removed only few seconds after I quit the mission and am back to main menu. So if I close the game before I get these message.

#24 The particleEmitter is killed only few seconds after you end the mission (actual ParticleEmitter desctructor call). So the fact to wait 5+ secs before hitting "quit" may help. The particleEmitter is killed at processTick only after they reach the state where the particleList is emtpy and the mDeleteWhenEmpty flag is true (gg fix). So unless we have a way to force the emitter to kill all particles we have no other solutions than to wait a few secs before actually closing the game. Anyone has better idea ?

#25 You could always just suppress the out of bins error in release builds... :)

ParticleEmitters are on my list of things to visit, but I'm unlikely to get to them by the time you need 'em fixed. :(

#26 Regarding the question, should these lines be inverted:

mEmitter->deleteWhenEmpty();
         mEmitter = pEmitter;

It looks like they should, doesn't it? But the answer is no. It's correct the way it is. Because.. This function is replacing the current emitter with a new emitter. There is already an emitter at mEmitter. This function attempts to create a new emitter (pEmitter), and if successful, it changes the mEmitter pointer to the new emitter. But the old emitter still exists, so before changing mEmitter, it makes a call to the current mEmitter->deleteWhenEmpty() so that the current (soon to be old) emitter will clean itself up as soon as it runs out of particles.

Hope that makes sense.

#27 Regarding the bins errors:

EmitterNodes (as well as every other game object class that uses particle emitters) clean up their emitter(s) by simply calling their deleteWhenEmpty() function. This doesn't immediately delete the emitter object, but rather allows it to delete itself when it runs out of particles. In an in-game situation this is good, because it would look rather strange if, for example, the smoke trail from a missile were to vanish as soon as the missile hit something. And that is what would happen if the emitters were deleted with the projectile, since in the current Torque particle system each emitter is responsible for tracking and rendering its own particles. If an emitter were deleted before the particles it emitted had all run their course, all the particles that it had generated would simply vanish.

So this deleteWhenEmpty approach works fine when objects are added and removed in the normal course of the game. The problem is when the mission ends. Now we really do want all the emitters deleted immediately. But they aren't. The emitters are cleaned up the same way they normally would be, with deleteWhenEmpty().

One solution is Groups. Objects are added to mission groups so that they will be deleted when the mission ends. Look at the top of any .mis (mission) file. Notice how it starts with a new SimGroup(MissionGroup). All the objects in this .mis are part of that group. Look at player.cs, when a player is spawned, you'll see a call to MissionCleanup.add(%player) adding the new player to the MissionCleanup group.

The problem is, while your ParticleEmitterNode is probably already being added to some mission group, the emitter object it creates is not. But, with a few simple additions to particleEmitter.cc, it can be.

Add:

void ParticleEmitterNode::onGroupAdd() {
	Parent::onGroupAdd();
	
	SimGroup* myGroup = getGroup();
	if (myGroup && mEmitter) myGroup->addObject(mEmitter);
}

And (new code in bold):

void ParticleEmitterNode::setEmitterDataBlock(ParticleEmitterData* data)
{
   if (!data)
      return;

   if (mEmitter)
   {
      mEmitterDatablock = data;
      ParticleEmitter* pEmitter = new ParticleEmitter;
      pEmitter->onNewDataBlock(mEmitterDatablock);
      if (pEmitter->registerObject() == false) {
         Con::warnf(ConsoleLogEntry::General, "Could not register base emitter for particle of class: %s", mDataBlock->getName());
         delete pEmitter;
         pEmitter = NULL;
      }
      if (pEmitter)
      {
         mEmitter->deleteWhenEmpty();
         mEmitter = pEmitter;
         [b]SimGroup* myGroup = getGroup();
         if (myGroup) myGroup->addObject(mEmitter);[/b]
      }
   }
}

Now the emitters created by emitter nodes will be deleted as part of the standard mission cleanup process.

Note that ideally this should be done for all objects which create their own particle emitters. Including Projectile, Debris, Explosion, Vehicle, Player, Splash, and ShapeImage classes.

I haven't actually tested the above code in the ParticleEmitterNode class. I have made similar additions to Projectiles, Explosions, and Vehicles. All of which have worked properly and solved "bins" errors. I haven't gotten around to testing the fix for EmitterNodes simply because I'm not using them in my game yet, but everything I've seen so far suggests it should work fine.

#28 There is to much "if'ing" and result comparing going on in a class that can be very CPU intensive. This could be cleaned up quite a bit by doing this instead...

void ParticleEmitterNode::setEmitterDataBlock(ParticleEmitterData* data)
{
	if(mEmitter)
	{
		ParticleEmitter* pEmitter = new ParticleEmitter;
		pEmitter->onNewDataBlock(data);
      
		if(pEmitter->registerObject())
		{
			mEmitterDatablock = data;
			mEmitter->deleteWhenEmpty();
			mEmitter = pEmitter;
			
			SimGroup* MissionCleanup = getGroup();
			
			if(MissionCleanup){ MissionCleanup->addObject(mEmitter); }
		}
		else
		{
			Con::warnf(ConsoleLogEntry::General, "Could not register emitter for class: %s", mDataBlock->getName());

			delete pEmitter;
			pEmitter = NULL;
		}
	}
}

I'm actually in the process of cleaning up quite a bit of the particle stuff. Like moving the "registerObject();" into the "onNewDataBlock()" function since they both have a True/False return and since the "onNewDatablock()" return is a wasted function because nothing happens either way true or false. It also prevents the setting of mEmitterDatablock to a bad setting which could currently happen. You can also move the group add to the "onNewDatablock()" function to wrap it all up in a neat package. That way the "onNewDatablock()" function can verify and register everything all at once and we can move on or exit based on the results obtained from it. This has to be done at all emitter creation points but it's no biggie. Doing so makes things quite a bit more efficient. The result looks like this...

bool ParticleEmitter::onNewDataBlock(GameBaseData* dptr)
{
	mDataBlock = dynamic_cast<ParticleEmitterData*>(dptr);
	
	if(!mDataBlock || !Parent::onNewDataBlock(dptr)){ return false; }

	if(this->registerObject())
	{
		scriptOnNewDataBlock();

		SimGroup* MissionCleanup = getGroup();
			
		if(MissionCleanup){ MissionCleanup->addObject(this); }
			
		return true;
	}
	
	return false;
}
 
     
  
void ParticleEmitterNode::setEmitterDataBlock(ParticleEmitterData* data)
{
	if(mEmitter)
	{
		ParticleEmitter* pEmitter = new ParticleEmitter;
      
		if(pEmitter->onNewDataBlock(data))
		{
			mEmitterDatablock = data;
			mEmitter->deleteWhenEmpty();
			mEmitter = pEmitter;
		}
		else
		{
			delete pEmitter;
			pEmitter = NULL;
		}
	}
}

#29 @Gonzo: The group functions should not be moved to onNewDataBlock(), because onNewDataBlock() may be called before the object has been added to a group. Leaving the group functions in onGroupAdd() assures that the object will have been added to a group before attempting to add the emitters.

Also, I don't think MissionCleanup is the best choice for a variable name in this case, since it may be misleading. The group in question may be the MissionCleanup group, or it may not. There are several other groups to which the mission objects may belong.

I like the idea of calling onNewDataBlock from setEmitterDataBlock(). It may be worth noting however, that setEmitterDataBlock appears to be used exclusively by the Particle Editor. It may not be worth spending much time optimizing, since it won't be used in the course of normal game play.

#30

Quote:The group functions should not be moved to onNewDataBlock(),

Why not?

Quote:because onNewDataBlock() may be called before the object has been added to a group

That's precisely why I moved the group.add() into onNewDataBlock().

When you call registerObject(), the True/False bool you get back is the result of SimObject::registerObject()'s call to "ParticleEmitter::onAdd()" which then calls back to "GameBase::onAdd()" which then calls back to "SceneObject::onAdd()" which then calls to "NetObject::onAdd()" which then calls back to "SimObject::onAdd()" which is where this all started with the "registerObject()" call. If you made it to here, then the object has completely established itself into the game, it's flagged as "Added" and then "True" is returned to "NetObject::onAdd()" which processes and returns the result to "SceneObject::onAdd()" which processes and returns the result to "GameBase::onAdd()" which processes and returns the result to "ParticleEmitter::onAdd()" which processes and returns the result to "SimObject::registerObject()" which then processes and returns the result to "ParticleEmitter::onNewDataBlock()'s" original query. So if we have a true result here, it's in the game by god, lol. This is a very complex process for the engine to handle for every emitter that is created. And when you consider that this process is called for any Weapon emitters, projectile emitter(s), explosion emitter(s), Debri emitter(s), Splash emitter(s), Dust emitter(s), Player emitter(s), vehicle emitter(s), every emitter, you can see exactly why it needs to be as fast, efficient, and precise as possible. The further down the line you are, the further back you have to call, and as a result more and more has to be processed. If you look at what I've done and the original code and consider every detail of how I rearranged the execution you'll see that billions upon billions of calculations per hour(maybe even per mission) can be saved if you are running a multiplayer network game that has 20 players firing weapons and flying vehicles that emit smoke and flames while firing weapons with explosions going off every other second and every other way that emitters get created going full steam as well.

As I stated earlier, onNewDataBlock is actually a bool function that is returning a condition to a function that didn't ask for it. In this case we are talking about "ParticleEmitterNode::setEmitterDataBlock". And also note that in the original "ParticleEmitter::onNewDataBlock" the function "scriptOnNewDataBlock();" will be fired off which then calls(Check the comments)...

void GameBase::scriptOnNewDataBlock()
{
	// Script onNewDataBlock() must be called by the leaf class
	// after everything is loaded.
	if(!isGhost()){ Con::executef(mDataBlock,2,"onNewDataBlock",scriptThis()); }
}

which we can see left the possibility of a bad call at the wrong time due to the fact that the original code hasn't even called to registerObject() yet. The way I've arranged it the only way that this function is getting called is if the object has actually been registered. And then after any scripted functions have been run(which is rare for the actual emitter) the object is moved into the MissionCleanup group which is almost certain to get deleted at mission end or engine shutdown if the code is not borked by someone getting rid of the MissionCleanup group or taking out the MissionGroup.delete(); calls. Keep in mind also that NONE of that will even happen until we have processed through...

bool GameBase::onNewDataBlock(GameBaseData* dptr)
{
	mDataBlock = dptr;

	if(!mDataBlock){ return false; }

	setMaskBits(DataBlockMask);
	return true;
}

So IMO it makes more sense to take advantage of the situation to centralize the focus point of every emitter creation at this location(ParticleEmitter::onNewDataBlock) and eliminate all those wasteful calls forever.

#31

Quote:I don't think MissionCleanup is the best choice for a variable name in this case, since it may be misleading.

How so? MissionCleanup is pretty much where every temp object ends up being put. The rest are usually deleted immediately within the function they originate in.

Quote:There are several other groups to which the mission objects may belong.

This is true but players, projectiles, explosions, and most of the other dynamic mission objects DO get put into MissionCleanup, which even so is still meaning less overall because the emitters are not children of their originating objects so where they are put is just a matter of preference, and MissionCleanup is the standard dumping ground for TGE, TSE.

Quote:It may be worth noting however, that setEmitterDataBlock appears to be used exclusively by the Particle Editor.

That's true, but as I stated earlier, if you make this change it should be done at every emitter creation point of which there are 20. Also I think you might want to take a good hard look at the emitters being created in Explosion.cc. Tell me if something there strikes you as odd.

Quote:It may not be worth spending much time optimizing, since it won't be used in the course of normal game play.

No offense intended at all but that's the exact same thinking that got Torque into the ragtag shape it's in, lol. Think about this, you get 1000 game ticks per second. And how much you can do in those 1000 ticks is directly related to how fast your CPU is. And the more CPU you eat up with spectacular graphics the less CPU you have left for game ticks. But the engine gets priority over your graphics, so the more your engine needs, the less your attention your graphics get, and down goes your FPS. Programmers today are locked into archaeic habits and misconceptions about the power of a processor. Most programmers don't give a little waste a second thought or even know exactly what is wasteful. And in the end, it really adds up. TGE has over half a million lines of code in it, if even one out of every 100 lines are wasteful, then what you get is 5000 wasteful lines of code that could be calling up to 5000 other functions which may have anything for 2 to 100 lines of code of their own to execute, or worse still, they might have to traverase an entire family like I demonstrated above. Theoretically you could be executing hundreds upon hundreds of thousands of wasteful functions, statements, or calculations, for every second Torque runs. What if half those lines were not only uneccesary but were also wasteful themselves? And what happens when all the lines you do need have waste in them that has to be added in? And the less speed the CPU has, the worse the problem compounds overall. Worse still, bad logic and waste in your scripts. Don't make me go there, lol.

Anything I touch gets optimized, period. I never compromise on that. Wasted CPU cycles get on my nerves.

#32 I don't have much to add to this thread, other than Gonzo's thoughts about optimization is a really valid point. I waste a lot of cycles in my code myself, and invariably wind up having to go back and find out "what functions are wasting so many cycles!"--better to just design and implement with that in mind from the beginning.

#33 Thanks Stephen

Quote:better to just design and implement with that in mind from the beginning.

Excellent, excellent point. LOL, after all my rant, I should have been the one to say that. DOH!!! That's exactly what it all boils down to. If you take those extra few minutes in each function to make sure it's clean and makes sense, you don't have to come back and screw with it later(time saved). As a bonus, it's easier for everyone to follow(time saved), easier to learn from(time saved), and easier to modify CORRECTLY, if you need to at a later date(time saved). The bottom line is, you are either going to clean it, or you're not, and if you are, then clean it to begin with and move on with confidence knowing it was done right and done well.

While we are on the subject, I'd like to point out a perfect example of total waste...

void Projectile::onCollision(.....)
{
	// No client specific code should be placed or branched from this function
	if(isClientObject()){ return; }

	if(hitObject != NULL && isServerObject())
	{

The commented line tell's you everything you need to know to see wasted CPU time in action. In TGE, if you are a projectile, you are either a server object, or you are a client object. That's it. Those are a projectiles only choices. So the first thing that gets done, is a check "isClientObject()" which has to traverse a few classes to find out which it is. If it's a client object, we exit(return) immediately. Fantastic! Just as it should be. So why is it that two lines down we run another check "isServerObject()" which has to traverse a few classes to find out which one it is? If we are here, we already know it's a server object. Worse still, it's being checked AFTER another wasteful check. First let's clean the object issue. Since nothing at all in th code block pertains to the client lets swap out the calls so the result would be this...

void Projectile::onCollision(....)
{
	// No client specific code should be placed or branched from this function
	if(!isServerObject()){ return; }
	
	if(hitObject != NULL)
	{

Much much better, but can still be improved. Now unless you know for a fact that your compiler is as smart as you are, you have to assume it's not and it will do what it's told to do in the most basic of operations. This line when processed will operate in the following fashion...

if(hitObject != NULL)

will set up a bool comparison by referencing the hitObject arg that was passed to us(if any). It will use the value of that arg for it's comparison. Let's say on this run we hit 1970. The result of the arg reference will set us up with this...

if(1970 != NULL)


which when processed will clearly compute as true because 1970 is not equal to NULL, and the rest of the code behind the condition will be executed. At this point, it is clear that we expect only two conditions, it's either NULL, or it's not. Nothing else matters. So the engine can correctly compute this state with...

if(hitObject)

but it can do it even faster because..

if(NULL) will always execute as false inside the engine and...

if(1970) will always execute as true inside the engine.


but we are only checking to see if a value exists instead of getting the value and then comparing it to another value. Since 1970 will read as true, 0 will be read as false which is the same as NULL. So to use any of these would be a waste...

if(hitObject != NULL)

if(hitObject != 0)

if(hitObject != false)

CONT:

#34 Care to guess how many hundreds of those little grinders are eating at nearly every module in the engine? There are about 300 places in the engine that use this type of conditional statement..

if(someValue == true)

and in those particular places(in all that I've found so far) the function could do the exact same job with..

if(someValue)

because in those places the only values it cares about are "true" or "not". And "NULL", "0", "", or "false", do not equal "true" and pretty much anything else does. All of this does make a difference. And after a while, it adds up into a BIG difference. I can't name names at this time because of NDA's, but maybe one of my customers will chime in here with thier opinions and observations of running one of my executables. Not as a bragging thing or ego stroaking, lol. If they have compared their personal .exe to mine and they know for a fact that my efforts resulted in a performance that is "useless", "worse", "no different", "better", whatever they feel they believe, I would like them to share it. Because I'd like them to reinforce in your mind what the extra effort is worth to them and why it matters to them in thier own words.

#35 The only caveat I would make to the above posts:

1) Code has to be readable as well as "fast". Based on context (C++, etc.), I'm assuming here that hitObject is actually a pointer, and therefore can never be "1970" (again, a contextual assumption that you mean an objectID of 1970)...and I'm also using the fact that you do check against NULL, which is an extremely common syntax to use (and I agree, simply understanding C++'s logic evaluations makes your technique a very popular one as well).

2) It doesn't apply in the particular examples you gave, but C++ compilers use "short circuit boolean evaluation", and have for quite a long time, so it's pretty fast as well. I will routinely do things like:

if ( (pointerToMyObject) && 
     (pointerToMyObject->publicValue) &&
     (pointerToMyObject->accessMethodThatUsesPublicValue())  )
{
...
}

because it makes my code extremely "safe" (robust), keeps people from breaking it by doing things I don't expect them to do, and is very fast due to short circuited boolean logic--the accessMethod will never be called, or even evaluated as needed to be called if either of the first two logic checks fail.

3) I think that while it illustrates your immediate point, use of

if (someValue)

can be very dangerous until the code is completely finished. Reason being that if a follow on coder doesn't realize that "someValue" is designed to be a bool, and you implemented it as say an S32, they could set it to something like someValue = 345;...and that will eval as true, even though it may not be the actual logic check "protection" you wanted when you originally wrote the code.

For community resources, I tend to not worry quite so much about processing optimization, because it's critical (at least to me) that your code be as robust as possible, and as obvious as possible. Folks are going to do things you didn't expect (like 3) above), and then wonder why your resource is "broken".


On the scale of robustness, performance optimization, and readability, each and every project (hell, each and every function) has different values along the scale...

#36 As a C and C++ developer for 20 years, I can tell you this:

the code:

if (someValue)

and

if (someValue == true)

Are exactly the same. It's not an optimization to take it out. Matter of fact, the optimizer will take care of most things that people consider optimizations (loop unrolling, redundant assignments, moving things out of loops, etc). While it is always true to have good coding practices and not be wasteful, (and I commend you on this), true optimization is done at the algorithm level - not at the single statement level. The golden rule of programming is: "Premature optimization is the root of all evil".

What that means is multifold: It is easier to cause errors, it is harder to read and debug, and it just plain wastes time.

Remember the (possibly apocraphyl) story about the first unix developers -- In an effort to increase the speed, they found the function that took up the most time. Spent days optimizing it, and made it much faster. To their chagrin, it didn't speed anything up at all. Then they realized it was the system idle process.

If you want to optimize TGE, there are other things that can be done - for one, rewriting the rendering pipeline so that, as much as possible, items are drawn in material & renderstate order. (this is obviously not always possible).

#37 Wish I had time to read all of that but I don't. So forgive me if I'm repeating something that's been said...

@Gonzo:

In many cases, an object won't have a group when it's onAdd() function is called. Believe me here. The first thing I tried when attempting to fix these bins errors was to place code in object's onAdd() functions to add their emitters to groups. It didn't work. Printing the results of getGroup() at this point revealed that frequently there was no group at that point. The problems were only solved by moving that code into the objects onGroupAdd() function. Which frankly makes more sense anyway.

A call to registerObject() is the first step in adding a new object to the simulation. registerObject() calls onAdd(). As yet, the object is part of no group. For GameBase derived objects on the server, onNewDataBlock() is then called immediately from onAdd(). Still, no groups.

Take a look at compliedEval.cc, line 596, case OP_ADD_OBJECT. AFAIK this is where new objects are added through script. Note that the first step is a call to registerObject() on line 600. This will initiate a chain resulting in onAdd() and onNewDataBlock(). Then, later in the code at line 638 we are added to our first group.

Does that make sense? Do you see why trying to call getGroup() in onNewDataBlock() would be a bad idea? The function onGroupAdd() exists for a good reason. ;)

#38 @Gonzo:

Quote:which even so is still meaning less overall because the emitters are not children of their originating objects so where they are put is just a matter of preference, and MissionCleanup is the standard

Whoa, wait. You don't think naming that variable "MissionCleanup" will put the emitter in the MissionCleanup group, do you? That's not how C++ works. It's just an arbitrary variable name. You could just as well say SimGroup* CasperTheFriendlyGhost = getGroup() and it woudn't matter. It won't affect what group getGroup() returns or what group the emitter will be added to.

Now, if you want to add the emitters to MissionCleanup, you could do this:

SimGroup* MissionCleanup = dynamic_cast<SimGroup*>(Sim::findObject("MissionCleanup"));
if (MissionCleanup) MissionCleanup->addObject(emitter);

But that assumes that there will be a MissionCleanup group. Currently MissionCleanup is purely a script-generated group, with no hard references to it in the engine. Personally, I think it best to keep it that way. I'd rather avoid hard coding names such as that into the engine itself, as it limits the engine's flexibility. And in this case, I see no real need to do it that way. Not to mention the added overhead of a dynamic cast and Sim::findObject() call.

#39 In case anyone is wondering Gonzo is correct in that it will generate different code.

To prove the point.

void func1()
{
        int i;
        if (i)
                printf ("foo");
}

void func2()
{
        int i;
        if (i == 1)
                printf ("foo");
}

With GCC 3.3.x will generate the following assembly:

.globl func1
        .type   func1, @function
func1:
        pushl   %ebp
        movl    %esp, %ebp
        subl    , %esp
        testl   %eax, %eax
        jne     .L4
.L2:
        movl    %ebp, %esp
        popl    %ebp
        ret
        .p2align 4,,7
.L4:
        movl    $.LC0, (%esp)
        call    printf
        jmp     .L2
        .size   func1, .-func1
        .p2align 4,,15

.globl func2
        .type   func2, @function
func2:
        pushl   %ebp
        movl    %esp, %ebp
        subl    , %esp
        decl    %eax
        je      .L7
.L5:
        movl    %ebp, %esp
        popl    %ebp
        ret
        .p2align 4,,7
.L7:
        movl    $.LC0, (%esp)
        call    printf
        jmp     .L5
        .size   func2, .-func2
        .section        .note.GNU-stack,"",@progbits
        .ident  "GCC: (GNU) 3.3.5  (Gentoo Linux 3.3.5-r1, ssp-3.3.2-3, pie-8.7.7.1)"

This was generated with:
gcc -O3 -S -c test.c

I'll leave it up to the assembly experts as to which chunk will be more effcient/faster.

#40 What you have there are two different comparisons:

if (i)

translates to:

if (i != 0)

which is not at all the same comparison as:

if (i == 1)

So of course the assembly is different.

This on the other hand:

if (bool)

does translate to:

if (bool == true)

That is the same comparison. Run those through your test and tell me if they don't come out the same.