Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensured that componets added after initialization are handled correctly. #258

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SleepProgger
Copy link
Contributor

There are multiple problems regarding components added by create_component / removed by remove_component.

Components created via create_component aren't removed at the gameworld.remove_entity call.

        gameworld.clear_entities()
        system = gameworld.system_manager['position']
        ent = gameworld.init_entity({'rotate':0}, ['rotate'])
        system.create_component(ent, 'general', (0,0))
        self.gameworld.remove_entity(ent)
        assert system.get_active_component_count() == 0

Initial components removed by remove_component lead to a crash at gameworld.remove_entity.

        gameworld.clear_entities()
        system = gameworld.system_manager['position']
        ent = gameworld.init_entity({'rotate':0, 'position':(0,0)}, ['rotate','position'])
        cind = gameworld.entities[ent].get_component_index('position')
        system.remove_component(cind)
        self.gameworld.remove_entity(ent)

Duplicate components lead to memory leaks as the second component will overwrite the first one without any warning whatsoever.

    gameworld.clear_entities()
    system = gameworld.system_manager['position']
    ent = gameworld.init_entity({'rotate':0, 'position':(0,0)}, ['rotate','position'])
    system.create_component(ent, 'general', (0,0))
    self.gameworld.remove_entity(ent)
    assert system.get_active_component_count() == 0

This PR fixes/prevents all the mentioned cases.

**Changes:**
- Changed the `EntityManager.set_component` method to call `Entity.set_component` so the `load_order` can be updated. Not super happy about this but i think its the best backwards compatible way to do this.
- The `Entity.set_component` function checks if the system is already in the `load_order` and otherwise adds it. This makes component initialization a little bit slower because we need to search in the `load_order` list. This could be optimized, but according to my benchmarks it doesn't really matter.
- The `Entity.set_component` function will now raise a `DuplicateComponentError` if there is already a component from the system active.
- The Entities `load_order` now saves the system indices instead of the names as all the places using them need the system index anyway.
- The Entities `load_order` property still returns the system_names but i removed the setter as changing them directly is just asking for trouble IMHO.


@SleepProgger
Copy link
Contributor Author

Unrelated, but can we get the travis build fixed please ? Seem to be a problem with opengl libs ?

@Kovak
Copy link
Contributor

Kovak commented Sep 14, 2017

👍 awesome work here, this is an area that really needed to be fleshed out. I'll look into fixing the travis build. It seems to break reaaally frequently and then fix itself every now and then. Don't know that much about travis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants