5
\$\begingroup\$

I am new person regarding LibGDX. I have written the below render method. I have a randomly generations oceans (where there are islands at random positions). Within the below code, focusedMapRectangle refers to the focused part of this map. The task is to draw the tiles of the islands that are within focusedMapRectangle.

I would like some feedback regarding:

  1. conversion from my own algorithms/code to LibGDX features (i.e. how can I use existing LibGDX features as opposed to doing the the calculations myself directly).

  2. code style / variable names (and whether others are preferred)

  3. inefficient calculations that I can do in more efficient ways (speed)

@Override
public void render(float delta) {
    Gdx.gl.glClearColor(0.5f, 0.5f, 1, 1);
    Gdx.gl.glClear(GL20.GL_COLOR_BUFFER_BIT);
    Gdx.gl.glClear(GL20.GL_ALPHA_BITS);
    getBatch().begin();

    Rectangle focusedMapRectangle = new Rectangle(
            getMatchMap().getBottomLeft().x,
            getMatchMap().getBottomLeft().y,
            Gdx.graphics.getWidth() / 16,
            Gdx.graphics.getHeight() / 16
    );

    getMatchMap().getIslands().forEach((island, point) -> {
        Rectangle islandRectangle = new Rectangle(
            point.x,
            point.y,
            island.getMaxWidth(),
            island.getMaxHeight()
        );

        // Signifies that the island is indeed present in the focused part of the map
        if (focusedMapRectangle.overlaps(islandRectangle)) {
            for (int islandTileX = 0; islandTileX < islandRectangle.getWidth(); islandTileX++) {
                for (int islandTileY = 0; islandTileY < islandRectangle.getHeight(); islandTileY++) {

                    // These refer to the absolute positions that each tile of the island has
                    // within the WHOLE map (not just focused)
                    int absoluteTileX = islandTileX + (int) point.x;
                    int absoluteTileY = islandTileY + (int) point.y;

                    // These refer to the position that each tile of the island has
                    // relative to the bottom left of the focused part of the map.
                    int relativeXToFocusPoint = absoluteTileX - (int) getMatchMap().getBottomLeft().x;
                    int relativeYToFocusPoint = absoluteTileY - (int) getMatchMap().getBottomLeft().y;

                    // Though we have verified that some part of the island is within the
                    // focused part of the map, it is is still possible that SOME of the island's tiles
                    // are NOT in focus.
                    boolean isXOutOfFocus = relativeXToFocusPoint < 0;
                    boolean isYOutOfFocus = relativeYToFocusPoint < 0;

                    // No need to render these said tiles of the island
                    if (isXOutOfFocus || isYOutOfFocus)
                        continue;

                    IslandTileType islandTileType = island.getRepr()[islandTileX][islandTileY];

                    // Do not go into statement if island tile type is water i.e null
                    if (islandTileType != null) {
                        Texture islandTexture = getIslandTextures().get(islandTileType);

                        int relativeCoordXToFocusPoint = relativeXToFocusPoint * 16;
                        int relativeCoordYToFocusPoint = relativeYToFocusPoint * 16;

                        getBatch().draw(islandTexture, relativeCoordXToFocusPoint, relativeCoordYToFocusPoint);
                    }
                }
            }
        }
    });

    getBatch().end();
}
\$\endgroup\$
3
\$\begingroup\$

Reduce Allocations

Creating objects while the game is running is not optimal. You are also creating those objects inside of the render method which will be called many thousands of times. The garbage collector is good, but you will still see performance spikes when it is doing too much work.

Luckily you can eliminate all of these allocations!

One approach that you will find useful is to use Pools. This is very easy when working with many libGDX classes. Setting it up is just one line where 100 below is the maximum number of objects you think you will need at one time:

private final Pool<Rectangle> recPool = Pools.get(Rectangle.class, 100);

Then when you want a rectangle object:

Rectangle focusedMapRectangle = recPool.obtain();

And when you are done using the object:

recPool.free(focusedMapRectangle);

The Rectangle class has a number of set methods in order to configure it after you obtain the object.

focusedMapRectangle.setX(getMatchMap().getBottomLeft().x);
focusedMapRectangle.setY(getMatchMap().getBottomLeft().y);
focusedMapRectangle.setWidth(Gdx.graphics.getWidth() / 16);
focusedMapRectangle.setHeight(Gdx.graphics.getHeight() / 16);

I mention Pools because I think you will find it useful in the future, however I actually think there is a better approach in this case.

You do need one new Rectangle at the start of the render method because it will match the current view of the world. However, rather than creating a new object each time, you could have one Rectangle and just use the set methods above to set it to the appropriate values.

That brings me to the next offender:

getMatchMap().getIslands().forEach((island, point) -> {
    Rectangle islandRectangle = new Rectangle(
        point.x,
        point.y,
        island.getMaxWidth(),
        island.getMaxHeight()
    );

What getIslands() returns is not totally clear to me, but I can tell that each island has a point associated with it. That point does not appear to be subject to change. I also assume that the maxWidth and maxHeight of the island are not subject to change. So why not pre-calculate the Rectangle for the island when first creating it? Then you can just do

if (focusedMapRectangle.overlaps(island.getRectangle())) { 

and completely avoid allocating any new objects.

Beware of iterators

I don't know if you are using the libGDX Array and Map classes, but you should consider doing so if not. forEach by itself is not a problem, but if you are using the regular Java collection classes, iterating will lead to lots and lots of allocations. The libGDX collections are designed to avoid these allocations when iterating.

Separation of concerns

Currently your render method does two main things. It determines whether an island is in view, and then it renders it. However, it does both of those things for each island at the same time. I think that it would be better to separate the sorting from the rendering. First you would call a sortIslands() method that would put all the islands that need to be drawn into an array. Then the render method would just iterate over the array and draw the necessary islands. Later if you decide to change the way the islands are sorted, it will be much easier. It will also be more clear and more self documenting to do the sorting outside of the render method.

Is it worth it?

Sorting through the tiles to decide what needs to be rendered and what doesn't will (in my experience) end up using more resources than it is worth. I recommend profiling with and without culling in order to see if it provides any performance gains for your game.

\$\endgroup\$
  • \$\begingroup\$ While some of this I have already fixed myself (the question is 3 weeks old), the pool and rectangle allocations points you brought up haven't been implemented within my game. I will apply those, thank you for your answer! \$\endgroup\$ – Mar Dev May 4 '18 at 22:49

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.