Potential early dismissal of physic task in jBullet

I might misunderstand that code, but this is most likely wrong.

[java]
private void setTickCallback() {
final PhysicsSpace space = this;
InternalTickCallback callback2 = new InternalTickCallback() {

        @Override
        public void internalTick(DynamicsWorld dw, float f) {
            //execute task list
            /* -------------------------- POLLED ONCE */
            AppTask task = pQueue.poll();
            /* -------------------------- POLLED A SECOND TIME */
            task = pQueue.poll();
            /* -------------------------- FIRST TASK WOULD NEVER BE RUN */
            while (task != null) {
                while (task.isCancelled()) {
                    task = pQueue.poll();
                }
                try {
                    task.invoke();
                } catch (Exception ex) {
                    logger.log(Level.SEVERE, null, ex);
                }
                task = pQueue.poll();
            }
            for (Iterator<PhysicsTickListener> it = tickListeners.iterator(); it.hasNext();) {
                PhysicsTickListener physicsTickCallback = it.next();
                physicsTickCallback.prePhysicsTick(space, f);
            }
        }
    };
    dynamicsWorld.setPreTickCallback(callback2);
    InternalTickCallback callback = new InternalTickCallback() {

        @Override
        public void internalTick(DynamicsWorld dw, float f) {
            for (Iterator<PhysicsTickListener> it = tickListeners.iterator(); it.hasNext();) {
                PhysicsTickListener physicsTickCallback = it.next();
                physicsTickCallback.physicsTick(space, f);
            }
        }
    };
    dynamicsWorld.setInternalTickCallback(callback, this);
}

[/java]

Why would the first task not be run? Its not cancelled so invoke() will be called? Or do I misunderstand something? Do you have some test case?

Edit: Oh, I see now, you mean the variable initialization already pulls one task, right? Yeah, that doesn’t seem right.

@normen said: Edit: Oh, I see now, you mean the variable initialization already pulls one task, right? Yeah, that doesn't seem right.

Yeah, popping it removes it from the stack and it’s replaced right away, so it’s lost. :confused:

I fixed this in the current gradle branch (which is our temporary trunk until the move to gradle is done), thanks!

2 Likes

Maybe there is some condition that prevents this from happening but what happens if the last task is cancelled? NPE?

I often see these loops that sprawl a little that probably evolved over time and then wonder why not:

[java]
AppTask task;
while ((task = pQueue.poll()) != null) {
if (task.isCancelled()) {
continue;
}
try {
task.invoke();
} catch (Exception ex) {
logger.log(Level.SEVERE, null, ex);
}
}
[/java]

It is generally better if a polling loop has only one poll… then you don’t have to worry about conditions slipping by one of them somehow… or if you see another extra one you know immediately that it’s wrong (as in the original unfixed code).

But I’m looking at it from the “never used it” perspective so maybe there is something I miss. Otherwise the tighter code should be the same result with one less potential bug.