Getting component from removed entity returns null


#1

Hi
Sorry about this noob question, I already searched forum but couldn’t find an answer. Sorry if it is asked before.

My question is, when an entity removed from an EntitySet (using ed.removeEntity ), I try to do some operations based on components on the removed entity, but I am getting null when trying to get a component.

Here is my code:

terrainBlocks = ed.getEntities(Filters.fieldEquals(PersistentEntityBuff.class, "target", EntityId.NULL_ID),
                PersistentEntityBuff.class, TerrainBlock.class, BlockInfo.class, BlockJoint.class); 

Note, filter will be updated later in game , I am using EntityId.NULL_ID just for initialization.

then in update()

if (applyTBChanges) {
            addBlocks(terrainBlocks.getAddedEntities());
            updateBlocks(terrainBlocks.getChangedEntities());
            removeBlocks(terrainBlocks.getRemovedEntities());
        }

and in removeBlocks method :

private void removeBlocks(Set<Entity> removedEntities) {
        removedEntities.forEach(block -> {
            if(block.get(BlockInfo.class) == null)
                System.out.println("BlockInfo is null");
            if(block.get(PersistentEntityBuff.class) == null)
                System.out.println("PersistentEntityBuff is null");
            
            EntityId terrian = block.get(PersistentEntityBuff.class).getTarget();
            terrains.getObject(terrian).removeBlock(block.get(BlockInfo.class).getNumber());
        });
    }

both of the BlockInfo and PersistentEntityBuff are returned null.

Is this the default behaviour and why ? and what is the proper solution ?


#2

Yes, this is the default behavior. After calling applyChanges() your set is updated. So if your entities don’t have the required components anymore it doesn’t make sense to try getting them.

If you really need the old component value from the entity then you need to save it before applying the changes. However, this rather alludes to a design issue.

What are you trying to do exactly?


#3

“Removes all of the components from an entity… surprised when it has no components.” :slight_smile:

The only thing you really have guaranteed access to for an entity removed from an EntitySet is its EntityId. Any of the components that were removed that cause it to go out of the set will be null now.

I’m not entirely sure what you are trying to do in your remove blocks so I can’t comment on a better way.

An aside: is it weird that you are building an entity set around entities with Filters.fieldEquals(PersistentEntityBuff.class, “target”, EntityId.NULL_ID) and then trying to ask for the target? Won’t it always be EntityId.NULL_ID?


#4

Thanks guys for your comments. I figured it out.

The filter will get update when a particular Scene gets enabled (when players from a particular Party are connected to server) while the players from a particular Party are not connected then no scene data will be loaded and processed.

private class TerrainContainer extends EntityContainer<TerrainModel> {

        Map<EntityId, ComponentFilter<PersistentEntityBuff>> filters = new HashMap<>();

        public TerrainContainer(EntityData ed) {
            super(ed, Filters.or(SceneState.class,
                    Filters.fieldEquals(SceneState.class, "state", SceneStates.ENABLED),
                    Filters.fieldEquals(SceneState.class, "state", SceneStates.REMOVED)),
                    Scene.class, SceneState.class, Terrain.class);
        }

        @Override
        protected TerrainModel addObject(Entity e) {
            log.trace("Add Terrain [terrainId:{}]", e.getId());
            
            filters.put(e.getId(), Filters.fieldEquals(PersistentEntityBuff.class, "target", e.getId()));
            resetFilters();
            return new TerrainModel(e.getId());
        }

        @Override
        protected void updateObject(TerrainModel object, Entity e) {
            
        }

        @Override
        protected void removeObject(TerrainModel object, Entity e) {
            log.trace("Remove Terrain [terrainId:{}]", e.getId());
            
            filters.remove(e.getId());
            resetFilters();
        }

        private void resetFilters() {
            if (!filters.isEmpty()) {
                terrainBlocks.setFilter(Filters.or(PersistentEntityBuff.class, filters.values().toArray(new ComponentFilter[filters.size()])));
                bridgeBlocks.setFilter(Filters.or(PersistentEntityBuff.class, filters.values().toArray(new ComponentFilter[filters.size()])));
            }
        }
    }

and this is how my TerrainBlockContainer looks like.
Note I changed it from an EntitySet (as you see in my first post) to an EntityContainer now.

private class TerrainBlockContainer extends EntityContainer<TerrainBlockModel> {

        public TerrainBlockContainer(EntityData ed) {
            super(ed, Filters.fieldEquals(PersistentEntityBuff.class, "target", EntityId.NULL_ID),
                    PersistentEntityBuff.class, TerrainBlock.class, BlockInfo.class, BlockJoint.class);
        }

        @Override
        protected void setFilter(ComponentFilter filter) {
            super.setFilter(filter);
        }

        @Override
        protected TerrainBlockModel addObject(Entity block) {
            EntityId terrainId = block.get(PersistentEntityBuff.class).getTarget();
            int number = block.get(BlockInfo.class).getNumber();

            log.trace("Add TerrainBlock [terrainId:{}, blockId:{}, blockNumber:{}]", terrainId, block.getId(), number);

            SpawnPosition position = ed.getComponent(block.getId(), SpawnPosition.class);
            Vector3f location = position != null ? position.getLocation() : new Vector3f();

            TerrainBlockModel blockModel = new TerrainBlockModel(terrainId, block, number, location);
            TerrainModel terrain = terrains.getObject(terrainId);
            terrain.addBlock(blockModel);

            if (blockModel.getParent() != null) {
                updateLocation(blockModel.getParent());
            }

            if (!toIntegrate.contains(terrain)) {
                toIntegrate.add(terrain);
            }

            addCollisionShape(blockModel);
            return blockModel;
        }

        @Override
        protected void updateObject(TerrainBlockModel blockModel, Entity block) {
            TerrainModel terrainModel = terrains.getObject(blockModel.getTerrainId());
            BlockJoint blockJoint = block.get(BlockJoint.class);

            blockModel.update(terrainModel, blockJoint.getParentBlockNumber(), blockJoint.getJointSide(), blockJoint.getJointPosition(), blockJoint.getOffset());

            if (blockModel.getParent() != null) {
                updateLocation(blockModel.getParent());
            } else {
                updateLocation(blockModel);
            }

            //Add terrain to queue, so it will integrate updates to
            //Physics component  and Navigation component next update
            if (!toIntegrate.contains(terrainModel)) {
                toIntegrate.add(terrainModel);
            }
        }

        @Override
        protected void removeObject(TerrainBlockModel blockModel, Entity block) {
            log.trace("Remove TerrainBlock [terrainId:{}, blockId:{}, blockNumber:{}]", blockModel.getTerrainId(), block.getId(), blockModel.getNumber());
            terrains.getObject(blockModel.getTerrainId()).removeBlock(blockModel.getNumber());
        }
    }

I reference this image so it would be more clear with what I mean by a TerrainBlock . (take out the trees and rocks they are just visuals and not handled by ES)

A Terrain consists of multiple TerrainBlock and BridgeBlock entities which are buffed to Terrain entity using a PersistentEntityBuff component.

@pspeed now I have a design question,
Which system is responsible for removing of entities. Should each system handle removal of it’s own entities ? I am particularly speaking about buffed entities. When a terrain is removed all the terrain blocks and bridge blocks buffed to that terrain should also get removed. Should TerrainSystem handle it or the DecaySystem ?
At the moment I am handling it with DecaySystem. So when an entity is removed I am finding all the entities which are buffed to it and remove them as well. I am strongly feeling it is wrong to do like that, what is your idea.

This is how my DecaySystem looks like which is a modified version of yours in SiO2.

/**
 * Tracks entities with Decay components and removes them once the decay has
 * expired.
 *
 * @author Paul Speed
 */
public class EntityDecaySystem extends AbstractGameSystem {
    
    static Logger log = LoggerFactory.getLogger(EntityDecaySystem.class);
    
    private EntityData ed;
    
    private DecayContainer entities;
    
    public EntityDecaySystem() {
    }
    
    @Override
    protected void initialize() {
        ed = getSystem(EntityData.class);        
        entities = new DecayContainer(ed);
    }
    
    @Override
    protected void terminate() {
    }
    
    @Override
    public void start() {
        super.start();
        entities.start();
    }

    /**
     * Called when a decaying entity has expired. Default implementation calls
     * EntityData.removeEntity().
     */
    protected void destroyEntity(Entity e) {
        if (log.isTraceEnabled()) {
            log.trace("Removing:" + e);
        }
        ed.removeEntity(e.getId());
    }
    
    @Override
    public void update(SimTime time) {
        super.update(time);
        entities.update();
        
        long current = time.getTime();

        // Check for expired entities
        for (Entity e : entities.getArray()) {
            EntityDecay d = e.get(EntityDecay.class);
            if (d.isDead(current)) {
                destroyEntity(e);
            }
        }
    }
    
    @Override
    public void stop() {
        entities.stop();
        super.stop();
    }

    /**
     * A simple EntityContainer that just tracks membership of the entity based
     * on the Decay component. Technically we're not much better here than a
     * straight EntitySet except we get the array access for free and convenient
     * start/stop behavior. (Could be a standard utility extension, I guess.)
     */    
    private class DecayContainer extends EntityContainer<Entity> {
        
        public DecayContainer(EntityData ed) {
            super(ed, EntityDecay.class);
        }
        
        @Override        
        public Entity[] getArray() {
            return super.getArray();
        }
        
        @Override
        protected Entity addObject(Entity e) {
            EntityDecay decay = e.get(EntityDecay.class);
            if (decay.isDecayBuffsEnabled()) {
                ed.findEntities(Filters.fieldEquals(PersistentEntityBuff.class, "target", e.getId()), PersistentEntityBuff.class)
                        .forEach(eId ->  ed.setComponent(eId, new EntityDecay(decay.getStartTime(), decay.getEndTime(), true));
            }
            return e;
        }
        
        @Override
        protected void updateObject(Entity object, Entity e) {            
        }
        
        @Override
        protected void removeObject(Entity object, Entity e) {
        }        
    }
    
}

#5

You are right that modifying the decay system to handle something new is a bad design. The whole idea behind the ES is that you can add new features without really affecting existing systems. So it’s a sign of a design problem when you have to hack an otherwise simple core system like this.

You call these buffs but I’m not sure they are? I will speak within the context of real buffs (like heatlh, magic, fire resistance, etc.) because it’s something I’ve actually thought about. It could be that it matches your use case.

Buffs are usually a case where in the end you want one value from a stack of values… like an attack bonus, health damage, or defense bonus. I would treat these all essentially the same but for every target value there would probably be a different system (at least conceptually).

An AttackBonusSystem would have an entity container that watched for AttackBuff components. In this case, it doesn’t really care about the adds/removes, only membership (you’ll see) so we can just return the entity directly from addObject.

Every update, we iterate over all of the entities in the buff set, and collect together all of the buffs for each target entity. (Probably in a map to make this simple.)

Map<EntityId, Attack> attackers = new HashMap<>();
for( Entity e : bonusContainer.getArray() ) {
    ...get existing Attack object for target...
    ...add or subtract e.get(AttackBonus)
}
...go through attackers map and apply final Attack values.

That’s the simplest way. Nothing to manage as the Attack value gets reset every update. It presumes things might have an Attack if there is still no bonus anymore.

Else we have to go the slightly more complicated way and keep our own buff structures inside the system. In this case, the AttackBonus component already has everything we need: target and bonus.

EntityContainer… addObject() just returns the component. When we see the addObject(), we look up the entity and add the bonus. When we see removeObject(), we look up the entity and remove the bonus.

The problem with “buffs” as relates to your not-really-a-buff (I think) thing is that buffs are inherently transient. We trust that they already have something to remove them at some point because they are usually time-based. We also think that objects that can have buffs in the first place will probably always have some target value… and the target is not likely to go away.

I think yours are more attachments. You are hooking something to a target entity and when that entity is removed then you want to remove the things that it points to. Don’t hack the decay system for this. Make a system that watches for the targets to disappear and removes their children. I’d base it on something you know might go away on the target. (Perhaps there are some things that really are permanent and don’t deserve this processing, eh?)

The problem with your current approach is that it hacks a core system that doesn’t care about your new components. It also doesn’t handle the case where the decay value is changed. (You’d have been better off cleaning things up in destroyEntity())

One of the ES mantras should be that if I’ve properly designed my main systems then if I want to add some new features, I should almost never have to modify those core systems. For example, I shouldn’t have to hack the decay system or damage system just because I’m adding fireballs now or whatever. Things should either fit logically into what I’ve already done or be a whole new system separate from them. (within reason)


#6

Thanks for your thorough explanation.

In my current iteration (it’s iteration 2) my design looks like below :

I am using term Buff as a generic name and distinguish it with a buff type.

public class BuffTypes {
    
    public static final int OBJECT                = 1; 
    public static final int ITEM                  = 2; 
    public static final int STAT                  = 3;
    public static final int SPELL_ATTACK          = 4;
    public static final int MELEE_ATTACK          = 5;
    public static final int QUEST                 = 6;
}

type OBJECT is used for any physical object which is attached to a Scene/Terrain. For example in this case a TerrainBlock is of this type.

I am considering them of type STAT. I am using a general Stat entity.

/**
 *
 * @author ali
 */
public class Stat implements PersistentComponent {

    private int type;
    private int value;
    private int maxValue;

    public Stat() {
    }

    private Stat(int type, int value, int maxValue) {
         this.type = type;
         this.value = value;
         this.maxValue = maxValue;
    }

    public static Stat create(String type, int value, int maxValue, EntityData ed){
        Stat stat = new Stat();
        stat.type = ed.getStrings().getStringId(type, true);
        stat.value = value;
        stat.maxValue = maxValue;
        return stat;
    }
    public int getType() {
        return type;
    }
    
    public String getTypeName(EntityData ed){
        return ed.getStrings().getString(type);
    }

    public int getValue() {
        return value;
    }

    public int getMaxValue() {
        return maxValue;
    }

    public Stat newAdjusted(int delta) {
        return new Stat(type, value + delta, maxValue);
    }
}

Yes, maybe lets say player avatar which is also attached to scene should not be removed when a scene is disposed but should be attached to new scene that is unlocked by completion of previous scene ?

Okay, I removed my modified DecaySystem and returned back to the one in SiO2. Now this is what I am doing in TerrainSystem.

I added a new SceneState component to scene. it can be in these states : (a COMPLETED state will be added later)

  • ENABLED (when party is connected)
  • DISABLED (when party is disconnect)
  • DISPOSED (we have already completed all the quests in this scene and can remove it now)

And in TerrainSystem inside TerrainContainer update method I am checking if a scene is in DISPOSED state I remove all the TerrainBlocks and BridgeBlocks associated with that scene.

private class TerrainContainer extends EntityContainer<TerrainModel> {

        Map<EntityId, ComponentFilter<PersistentEntityBuff>> filters = new HashMap<>();

        public TerrainContainer(EntityData ed) {
            super(ed, Filters.or(SceneState.class,
                    Filters.fieldEquals(SceneState.class, "state", SceneStates.ENABLED),
                    Filters.fieldEquals(SceneState.class, "state", SceneStates.DISPOSED)),
                    Scene.class, SceneState.class, Terrain.class);
        }

        @Override
        protected TerrainModel addObject(Entity e) {
            log.trace("Add Terrain [terrainId:{}]", e.getId());

            filters.put(e.getId(), Filters.fieldEquals(PersistentEntityBuff.class, "target", e.getId()));
            resetFilters();
            return new TerrainModel(e.getId());
        }

        @Override
        protected void updateObject(TerrainModel terrain, Entity e) {
            log.trace("Update Terrain [terrainId:{}]", e.getId());
            
            SceneState sceneState = e.get(SceneState.class);
            if(sceneState.getState() == SceneStates.DISPOSED){
                terrain.getTerrainBlocks().values().forEach(block -> ed.removeEntity(block.getEntity().getId()));
                terrain.getBridgeBlocks().values().forEach(block -> ed.removeEntity(block.getEntity().getId()));
            }
        }

        @Override
        protected void removeObject(TerrainModel object, Entity e) {
            log.trace("Remove Terrain [terrainId:{}]", e.getId());

            filters.remove(e.getId());
            resetFilters();
        }

        private void resetFilters() {
            if (!filters.isEmpty()) {
                terrainBlocks.setFilter(Filters.or(PersistentEntityBuff.class, filters.values().toArray(new ComponentFilter[filters.size()])));
                bridgeBlocks.setFilter(Filters.or(PersistentEntityBuff.class, filters.values().toArray(new ComponentFilter[filters.size()])));
            }
        }
    }

Now for removing a scene :

ed.setComponent(sceneId, new SceneState(SceneStates.DISPOSED));
ed.setComponent(sceneId, Decay.duration(System.nanoTime(), 1000000000)); // using default decay component of  SiO2

@pspeed any thoughts ?