A simple java game engine [closed]

Question

I'm starting an object orientated game engine in Java. My plans for the engine is to be able to split things easily into multi-core loads. My idea is that a Unique (interface with a GetUID() method) gets passed in. The Unique can also implement any number of other interfaces. In this case, the unique can also implement an Entity (having the Compute() and Render() methods) or MouseInputter (having the UpdateMouse() method). In the future, any number of other interfaces can also be added and handled if needed. Then Tick() moves everything forward one step. So far I've not implemented the multi-core parts, but want early feedback to ensure I'm not going astray.

I'd like to know the common weaknesses or pitfalls of my approach. I'm also concerned about the performance impact of running instanceof X times on N objects per loop. Is there a benefit to caching the interfaces? I think I'd use a long with bitmasks, so the overhead is about 16 bytes per object. It's harder to compare the CPU load (fetching from cache vs instanceof). Is caching this value worth the tradeoff, generally? So what I'm getting at is; is my approach scalable or is a different one required?

package com.mygdx.engine;

import java.util.ArrayList;
import java.util.HashMap;

import com.badlogic.gdx.Gdx;
import com.mygdx.game.entity.Entity;
import com.mygdx.game.entity.Unique;
import com.mygdx.game.utils.MouseInputter;

public class Engine {

    private HashMap<String,Unique> things = new HashMap<String,Unique>();
    private long lastComputeMillis;
    private long lastMouseMillis;

    // TODO: implement multicore.
    public Engine(int cores) {

    }

    public void Tick() {
        this.UpdateMouse();
        this.Compute();
        this.Render();
    }
    
    public void AddObject(Unique o) {
        things.put(o.GetUID(), o);
    }
    
    public boolean Remove(String o) {
        if( this.things.containsKey(o) ) {
            things.remove(o);
            return true;
        }
        return false;
    }

    private void render() {
        Entity transientEntity;
        for (Object o : things.values()) {
            if (o instanceof Entity) {
                transientEntity = (Entity) o;
                transientEntity.Render();
            }
        }
        this.lastComputeMillis = System.currentTimeMillis();
    }

    private void compute() {
        Entity transientEntity;
        for (Object o : things.values()) {
            if (o instanceof Entity) {
                transientEntity = (Entity) o;
                transientEntity.Compute(System.currentTimeMillis() - this.lastComputeMillis);
            }
        }
        this.lastComputeMillis = System.currentTimeMillis();
    }

    private void updateMouse() {
        int mousex = Gdx.input.getX();
        int mousey = Gdx.input.getY();
        MouseInputter transientEntity;
        for (Object o : things.values()) {
            if (o instanceof MouseInputter) {
                transientEntity = (MouseInputter) o;
                transientEntity.UpdateMouse(mousex, mousey, System.currentTimeMillis() - this.lastMouseMillis);
            }
        }
        this.lastMouseMillis = System.currentTimeMillis();
    }

}

Answer 1

Overall review

I think it's too early to give a profound review of your architecture, so here's only what I found at a quick glance. It would be helpful to see the interface definitions besides the Engine class, at least (with Javadoc, please).

Naming conventions

With Java naming conventions, method names begin with lower case.

Javadoc

There are no Javadoc comments documenting the key methods.

Object orientation

private void Compute() {
    Entity transientEntity;
    for (Object o : things.values()) {
        if (o instanceof Entity) {
            transientEntity = (Entity) o;
            transientEntity.Compute(System.currentTimeMillis() - this.lastComputeMillis);
        }
    }
    this.lastComputeMillis = System.currentTimeMillis();
}

Using instanceof always is a hint at a flawed design. If you go for a one-for-all-purposes Engine class (instead of splitting that into sub-functionalities like computations and renderings), you should also go for a one-type-fits-all Thing interface having all the necessary methods for your Engine.

Bug

In the Compute() method, I think you want to call each Entity with the time span elapsed since its last Compute() invocation (having Javadoc would make that question clear), but in reality you use System.currentTimeMillis() on every individual call (changing from iteration to iteration), and record only one lastComputeMillis. This variable will receive an instant of time that lies after all System.currentTimeMillis() used in the Compute() calls, creating varying time gaps for the objects.

To clearly see the effect, implement some ten objects with slow Compute() methods, and have them log their time span parameter. Compare that with the total time elapsed. As a second effect of that flaw, after the loop the Entity instances will not reflect a consistent state in time but a blurred time range.

Answer 2

Types should be discriminated

This way of having one mega container that doesn't discriminate will make the code run through all objects multiple times. Let's imagine there are 10000 objects (because particles, for instance), but only 3 MouseInputter? Well, the CPU will do nothing but loop 99.997% of the time.

Just create several Maps: one for your entities and one for the mouseinputter objects.

private final Map<String, Entity> entities = new HashMap<>();
private final Map<String, MouseInputter> mouseInputters = new HashMap<>();

Then change your addObject method to something like this:

void addObject(Unique obj) {
  if (obj instanceof Entity entity) {
    entities.put(obj.getUid(), entity);
  }
  if (obj instanceof MouseInputter mouseInputter) {
    mouseInputters.put(obj.getUid(), mouseInputter);
  }
}

Then in your render method: (do the same for your compute and mouseInput methods)

void render() {
  var nanos = System.nanoTime() - this.lastRenderNanos;
  for (var entity: entities.values()) {
    entity.render(time);
  }
  this.lastRenderNanos = nanos;
}

What does it cost in terms of memory? Peanuts. What do you get in terms of performance? No more empty loops and costly reflection checks.

The remove method is too complex

The remove method should now delete from two maps, but the current implementation is way too complex. If something is in there, delete it, if not don't. That's exactly what the Map::remove method does, so just use it blindly. What happens in the case there is such element to remove? The method will have to find the appropriate element location twice, and you lose performance.

Okay, here I suggest to always have two lookups, but that's because I have two maps to look into. So two computations in either case means the better architecture wins, and that'd be this one.

boolean remove(String uid) {
  var changed =   entities.remove(uid);
                | mouseInputters.remove(uid);
  return changed;
}