11
\$\begingroup\$

I'm working on a new prototype that is a simple drag and shoot arcade basketball game. For this project I am working with LibGDX and the Box2d physics library.

You can play the game here: Play arcade basketball!

This here is the custom collision listener for the game. One quirk that I have not been able to resolve is that there is no guarantee whether fixtureA or fixtureB will be first, and no way to guarantee which of them is which object, so in each of the methods I need to test against both of the fixtures to find the correct one.

Another problem is that I need to do frequent casting to access the properties of my classes, and I am frequently using the user data of the box2d bodies in order to store a reference to those classes. It makes the code hard to read and understand, but as long as I am careful it has not caused any crashes.

Here is the collision class:

public class BasketballListener implements ContactListener {

    private WormyBasketballWorld basketballWorld;

    public BasketballListener(WormyBasketballWorld basketballWorld) {
        this.basketballWorld = basketballWorld;
    }

    @Override
    public void preSolve(Contact contact, Manifold oldManifold) {   
    }
    @Override
    public void postSolve(Contact contact, ContactImpulse impulse) {
    }
    @Override
    public void endContact(Contact contact) {
    }
    @Override
    public void beginContact(Contact contact) {
        Fixture fixtureA = contact.getFixtureA();
        Fixture fixtureB = contact.getFixtureB();

        this.ballAndHoopCollision(fixtureA, fixtureB);
        this.ballAndTargetCollision(fixtureA, fixtureB);
        this.scoreDetection(fixtureA, fixtureB);
    }

    private void ballAndHoopCollision(Fixture fixtureA, Fixture fixtureB) {
        if (fixtureA.getBody().getFixtureList().get(0).getFilterData().categoryBits == CollisionFilter.HOOP) {
            if (fixtureB.getBody().getUserData() instanceof SimpleWeaponOrb) {

                //System.out.println("hoop collision with ball");

                this.basketballWorld.ballCollidedWithHoop(fixtureB.getBody().getAngle(), fixtureB.getBody().getLinearVelocity());
            }
        }
        if (fixtureB.getBody().getFixtureList().get(0).getFilterData().categoryBits == CollisionFilter.HOOP) {
            if (fixtureA.getBody().getUserData() instanceof SimpleWeaponOrb) {

                //System.out.println("hoop collision with ball");

                this.basketballWorld.ballCollidedWithHoop(fixtureA.getBody().getAngle(), fixtureA.getBody().getLinearVelocity());
            }
        }
    }

    private void ballAndTargetCollision(Fixture fixtureA, Fixture fixtureB) {
        if (fixtureA.getBody().getUserData() instanceof TargetPlatform) {
            ((TargetPlatform)fixtureA.getBody().getUserData()).markDestroyed();
        }
        if (fixtureB.getBody().getUserData() instanceof TargetPlatform) {
            ((TargetPlatform)fixtureB.getBody().getUserData()).markDestroyed();
        }
    }

    private void scoreDetection(Fixture fixtureA, Fixture fixtureB) {
        if (fixtureA.isSensor()) {
            if (fixtureB.getBody().getUserData() instanceof SimpleWeaponOrb) {
                if ((HoopSensor)fixtureA.getBody().getUserData() == HoopSensor.TOP_SENSOR) {
                    ((SimpleWeaponOrb)fixtureB.getBody().getUserData()).hasHitFirstTarget = true;

                    //System.out.println("orb has hit first sensor");

                } else if ((HoopSensor)fixtureA.getBody().getUserData() == HoopSensor.BOTTOM_SENSOR) {

                    //System.out.println("orb has hit second sensor");

                    if (((SimpleWeaponOrb)fixtureB.getBody().getUserData()).isValidForScore()) {
                        ((SimpleWeaponOrb)fixtureB.getBody().getUserData()).hasAlreadyScored = true;

                        //System.out.println("orb has scored");

                        this.basketballWorld.scored();
                    } else {
                        //invalidate balls that hit the second fixture before the first
                        ((SimpleWeaponOrb)fixtureB.getBody().getUserData()).hasAlreadyScored = true;
                    }
                }
            }
        }
        if (fixtureB.isSensor()) {
            if (fixtureA.getBody().getUserData() instanceof SimpleWeaponOrb) {
                if ((HoopSensor)fixtureB.getBody().getUserData() == HoopSensor.TOP_SENSOR) {
                    ((SimpleWeaponOrb)fixtureA.getBody().getUserData()).hasHitFirstTarget = true;

                    //System.out.println("orb has hit first sensor");

                } else if ((HoopSensor)fixtureB.getBody().getUserData() == HoopSensor.BOTTOM_SENSOR) {

                    //System.out.println("orb has hit second sensor");

                    if (((SimpleWeaponOrb)fixtureA.getBody().getUserData()).isValidForScore()) {
                        ((SimpleWeaponOrb)fixtureA.getBody().getUserData()).hasAlreadyScored = true;

                        //System.out.println("orb has scored");

                        this.basketballWorld.scored();
                    } else {
                        //invalidate balls that hit the second fixture before the first
                        ((SimpleWeaponOrb)fixtureA.getBody().getUserData()).hasAlreadyScored = true;
                    }
                }
            }
        }
    }
}

These two lines are all that is required to make the physics world use my custom listener:

    BasketballListener listener = new BasketballListener(this);
    this.world.setContactListener(listener);

Additionally, I would love any feedback you can provide about the way the physics feel.

\$\endgroup\$
4
\$\begingroup\$

Code Duplication

Each of ball*Collision() and scoreDetection() methods contain too much duplicated code. In each of them, the second outer if block can be simply cut out. An additional call of the method with inversed arguments will produce the same effect:

this.ballAndHoopCollision(fixtureA, fixtureB);
this.ballAndHoopCollision(fixtureB, fixtureA);

...

private void ballAndHoopCollision(Fixture fixtureA, Fixture fixtureB) {
    if (fixtureA.getBody().getFixtureList().get(0).getFilterData().categoryBits == CollisionFilter.HOOP) {
        if (fixtureB.getBody().getUserData() instanceof SimpleWeaponOrb) {
           this.basketballWorld.ballCollidedWithHoop(fixtureB.getBody().getAngle(), fixtureB.getBody().getLinearVelocity());
        }
    }
}

Law of Demeter

This code spits abundantly on the Law of Demeter. Calls like

fixtureA.getBody().getFixtureList().get(0).getFilterData().categoryBits

are simply awful and indicate that there is a deep design issue with the definition of the entities used throughout the code.

The worst case is when such calls modify the state of the deeply dependent objects, e.g. markDestroyed() or hasHitFirstTarget. It may be a real hell for debugging.

Encapsulation

categoryBits, hasAlreadyScored and hasHitFirstTarget should not be directly exposed and should be accessed/modified with getters/setters.

Missing Abstractions

There are many class casts like (SimpleWeaponOrb) or (HoopSensor) and checks with instanceof. It looks like the hierarchy of objects that may be returned by getUserData() is chaotic or there are some abstractions missing. I cannot say more about it, because the example is not enough to conclude about.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer! I never thought about just calling the method again with reversed arguments, very nice. Regarding the Law of Demeter, are you very familiar with box2d? I ask because the deeply nested categoryBits variable and all those other methods to access it are part of the box2d API. I'm not sure how I would implement the ContactListener class without referencing the Fixture and Body variables provided by box2d. \$\endgroup\$ – bazola Dec 17 '15 at 20:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.