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

Design and implement a mechanism for discrete game states #46

Closed
bkloster opened this issue Oct 16, 2013 · 25 comments
Closed

Design and implement a mechanism for discrete game states #46

bkloster opened this issue Oct 16, 2013 · 25 comments
Assignees

Comments

@bkloster
Copy link
Contributor

Eventually, the game will need to switch between different states. Examples for game states are "main menu", "microbe gameplay" or "microbe editor".

Each game state is associated with a set of active systems and active entities. Transitioning from state A to state B should follow this rough procedure:

  1. Pause all systems associated with state A
  2. Switch the set of active entities (?)
  3. Resume all systems associated with state B

Implementing the entity switch could be achieved in multiple ways.

  1. Let the EntityManager hold a flagset per entity indicating whether the entity is active in a particular state. When queried for a component, the manager first checks if the entity is supposed to be active. If not, it returns a nullptr.
  2. Instead of filtering on queries, the EntityManager actually reorganizes its internal data on each state change.
  3. Make the EntityFilter accept state flags in its constructor (or as template parameter, but that could get ugly real quick). The filter only keeps those entities that are active in the states it filters for. This would obviously require the EntityManager to keep state flags for each entity around as well.
@jjonj
Copy link
Contributor

jjonj commented Oct 24, 2013

I've been fiddling a bit with a solution to this and i have a question: Why does the gamestate need to keep a set of "active" entities?

@bkloster
Copy link
Contributor Author

It may not have to, but that depends on how the switching mechanism is implemented. We definitely need some way to separate entities according to game state. For example, while the microbe editor state is active, the graphics engine should render a very different set of entities compared to when the microbe gameplay state is active.

After thinking a bit more about it, it looks promising to have completely separate EntityManager instances per game state, along with separate instances of the systems (i.e. no sharing of systems or entities between states). We'd still need some interface to access entities from another state, so that the microbe editor can access the player microbe structure from the gameplay state.

If we go that route, we'll also need to take a careful look at how scripts (and some C++ systems) interact with the entity manager and some of the systems (KeyboardSystem and MouseSystem come to mind). When the engine switches game state (and thus, the active EntityManager instance and the systems), we may end up with stale references in some scripts, still pointing at the now inactive previous entity manager and sytems.

@jjonj
Copy link
Contributor

jjonj commented Oct 25, 2013

Yeah it's a tricky problem. I also initially just thought it would be easy to keep seperate EntityManagers, but came to the issues with systems after i finally managed to understand EntityFilter somewhat (any reason EntityFilter.cpp isn't included in the cmakelists for files to include, confused me?).
Another idea is to change the way EntityFilter works so they don't keep their own collections of entities, which doesn't seem too intuitive to me, but obviously the system was made like it was for a reason, so it must be problematic without their own collections.

I'll try and give the issue some thought.

@bkloster
Copy link
Contributor Author

entity_filter.cpp is included by entity_filter.h, which seems backwards, but is necessary because EntityFilter is a template and the compiler needs to see the whole code wherever the template is instantiated. Originally, it was completely in one (huge) header file, but I separated it out into two separate ones so that the header pretty much only contains the interface while the implementation resides in the cpp file. That makes it easier to quickly look over EntityFilter's API without all that noise from the implementation.

The EntityFilter keeps a cache of matching entities around, I think that's what you mean by "their own collections". Without that cache, the filters would have to iterate over all entities each frame, which seems a little wasteful.

@jjonj
Copy link
Contributor

jjonj commented Oct 25, 2013

Yeah i know about the requirement for templates and header files, the reason i ask is that i have chosen the blessings and curses of staying a windows user, and the entity_filter.cpp didn't appear in codeblocks (to some confusion).
To my understanding this is because it isn't listed in srs/engine/CMakeList.txt, which is what i was curious about, is it intentional and necessary, or by accident that it isn't listed there?

@jjonj
Copy link
Contributor

jjonj commented Oct 26, 2013

Regarding scripts pointing to innactive systems, that could happen nomather how we do it, right? as we can be in a gamestate where some systems are innactive, while scripts still are trying to use them?

@bkloster
Copy link
Contributor Author

entity_filter.cpp didn't appear in codeblocks

Oh, that's right. I forgot that Code::Blocks only displays files that CMake knows about. I'm actually not sure if adding that file will cause any problems. It either doesn't, or the compiler will complain about duplicate stuff (stupid compiler). I'll try it out later.

Regarding scripts pointing to innactive systems, that could happen nomather how we do it, right?

That depends. The case I'm thinking about is when we have two identical systems in separate game states. Say, the (yet to be implemented) sound system. That sound system has a method setVolume which scripts can use to adjust the audio volume like this:

SoundSystem:setVolume(1.0)

with SoundSystem being a global, similar to Keyboard or Mouse right now. When we switch game states, we'd need to change that global to the SoundSystem of the newly active state. That's not quite trivial, but doable. Where stuff breaks is when a script author hears about local variables being faster in Lua and, meaning well, does this:

local soundSystem = SoundSystem
-- Later, in some update loop
soundSystem:setVolume(1.0)

Even if we switch out the SoundSystem global, the local soundSystem will remain the same. The call to setVolume on the inactive sound system will have no effect. It's easy to imagine other cases where something like this can happen. Many of them will result in bugs that will be easy to fix, but hard to find.

Now, if we don't use completely separate systems and share them among states, this problem isn't really there. The new game state uses the same SoundSystem instance, after all, so we don't need to switch out the global nor does the script author's local variable go stale. Everything works as expected. Worst case scenario: The new game state deactivates the SoundSystem because the state is not supposed to play any sound. No biggie, the call to setVolume still works, it just doesn't have any immediately obvious effect.

@jjonj
Copy link
Contributor

jjonj commented Oct 26, 2013

Ah thats what you meant, i can see the issue. but I don't see the need for keeping seperate system instances in the first place.

An idea i've been working on that i thought i should share:
I thought of moving the internal m_entities collections of EntityFilter into the EntityManager instead, they could be implemented as unordered_map<tuple<ComponentType1,..., ComponentTypeN>, EntityMap> with an entry for each used combination of component types. Then have the EntityManager handle changes in the collections with state changes (probably with another level of unordered_map with gamestate as key)
But i haven't thought it through completely yet with the existing code, so there might be issues i haven't spotted yet

Edit: Fixed formatting of proposed type

@bkloster
Copy link
Contributor Author

I don't see the need for keeping seperate system instances in the first place.

It all comes down to how we handle the entities during a state switch. We could notify systems (i.e. their entity filters) that some entities have been added or removed due to a state switch. But I'm pretty sure that only very few entities will actually have a place in more than one state. Most will be exclusive to a particular one. So, the OgreSceneNodeSystem, for example, will end up removing almost all scene nodes from the Ogre::SceneManager, then adding a whole bunch of new scene nodes from the new state.

Now, if we have separate system instances (and move the Ogre::SceneManager from the engine into an appropriate system), we can instead just switch out the scene manager together with the system. It would also avoid all the calls into the entity filters, because each system's entity filter would only listen to the respective entity manager. Since no entities are actually added or removed, the entity filters don't have to do anything.

I thought of moving the internal m_entities collections of EntityFilter into the EntityManager instead

I really don't see what that would accomplish. On the contrary, it would greatly complicate the entity manager. We'd probably need to change the EntityManager each and every time we need another component combination.

@jjonj
Copy link
Contributor

jjonj commented Oct 26, 2013

each system's entity filter would only listen to the respective entity manager.

Wouldn't it be simpler to have more entity filters in one system than to have seperate systems?

We'd probably need to change the EntityManager each and every time we need another component combination.

You'd just insert a new element in the unordered_set i proposed, which i guess is indeed "changing the EntityManager" but i'm not sure thats what you meant. The advantage would be that the entity manager could keep all the state changing in its internal implementation, and when the entity filter is queried for its entities, it just forwards a constant time lookup to the entity manager.
Ultimately it's a bit of a restructuring and naturally not up to me, just an idea :)

EDIT: The tuples i'm talking about here would be what ComponentGroup is defined as, if i understand it correctly.
EDIT2: I made a mistake in the formatting of my previous comment so it didn't actually show the composite type that i was trying to illustrate: unordered_map<tuple<ComponentType1,..., ComponentTypeN>, EntityMap> which would probably become std::unordered_map<ComponentGroup, EntityMap>

@jjonj
Copy link
Contributor

jjonj commented Oct 26, 2013

I've done a poor job explaining my idea, i'll try like this:

Use case: System X wants to iterate over the entities and thus needs a collection (AKA EntityMap) of entities.
System X trusts that it only gets the entities that have the right components and only the entities relevant for the active game-state.

This happens through 3 levels.

  • Level 1: System X requests the entities from its EntityFilter (semantically same as before)
    • call EntityFilter.entities()
  • Level 2, EntityFilter now requests the correct EntityMap from EntityManager instead of having it's own collection:
    • call EntityManager.getEntitiesFiltered(componentGroup)
  • Level 3: EntityManager does a constant time lookup
    • return (m_filteredEntityMapsState[activeGameState])[filter]

The relevant implementations:

std::unordered_map<GameState, std::unordered_map<ComponentGroup, EntityMap>> filteredEntityMapsState;
template<typename... ComponentTypes>
const typename EntityFilter<ComponentTypes...>::EntityMap&
EntityFilter<ComponentTypes...>::entities() const {
    return entityManager.getEntitiesFiltered(magic_cast<ComponentGroup>(ComponentTypes));
}
EntityMap&
getEntitiesFiltered(
    ComponentGroup filter
){
    return (m_filteredEntityMapsState[activeGameState])[filter];
}

One minor advantage is that the need for callbacks will be gone, simplifying some of the code.

Things like iterators should also just be forwardable from EntityFilter to the collection in EntityManager

This idea should completely remove the need to have seperate systems, entity manager and entity filters!

I don't see any problems with this solution... but i don't understand all the affected classes completely either (perhaps the whole record-changes become problematic, i haven't looked through how that's used yet)

@bkloster
Copy link
Contributor Author

Ok, that makes a lot more sense than what I initially imagined you proposed. We'd need to find a good way to make ComponentGroup hashable, though. I don't see any obvious (and easy) way to do that. I'm also not quite convinced that it's better than separate systems, because it doesn't solve this problem I mentioned earlier:

the OgreSceneNodeSystem, for example, will end up removing almost all scene nodes from the Ogre::SceneManager, then adding a whole bunch of new scene nodes from the new state.

Anyway, for an example of how recordChanges is used, look at the RigidBodySystem, in particular this part. It's really just a convenient way for systems to react to new or removed entities.

@jjonj
Copy link
Contributor

jjonj commented Oct 27, 2013

We'd need to find a good way to make ComponentGroup hashable

With hashable i assume you mean usable as a key in a hashtable/map, which i actually thought was trivial, my lack of experience being at fault there.

this problem I mentioned earlier

I'm not sure i understand the problem, i'm not fully familiar with ogre yet. Wouldn't seperate systems also just be pointing to the same Ogre::SceneManager and having to do the same thing? or are you suggesting having seperate SceneManagers aswell?

@jjonj
Copy link
Contributor

jjonj commented Oct 27, 2013

Regarding hashing, ComponentGroup just ends up being a tuple of component types, right? There is a stack overflow post here that implements hashing of generic tuples, shouldn't that be able to work for us?
http://stackoverflow.com/questions/7110301/generic-hash-for-tuples-in-unordered-map-unordered-set

@bkloster
Copy link
Contributor Author

Hm, could work. But remember that we wouldn't be able to use std::tuple itself anymore because not all ComponentGroups have the same number of components. It would have to become kind of a "runtime tuple" that we'll need to write ourself.

By the way, are you actively working on this issue right now? Otherwise, I'll probably take a stab at it this week (albeit with the separate entity manager / system approach).

@jjonj
Copy link
Contributor

jjonj commented Oct 28, 2013

anymore because not all ComponentGroups have the same number of components.

I don't want to sound rude, but did you read the post? It should work for generic std::tuples, right?
I have actually been transforming some of the code in a branch, but some parts are a bit hard to understand right now, so it would take me quite a while.
It being an important thing to make progress it would probably be wiser if you took on the task.

Btw, if you want me to change anything regarding the agent-registry pull request let me know.

@bkloster
Copy link
Contributor Author

Sorry, I didn't make myself clear enough. I was more concerned about using the tuple as a key in the map inside the entity manager. All keys inside a map have to have the same type and std::tuple<OgreSceneNodeComponent, RigidBodyComponent> is a different type than std::tuple<AgentEmitterComponent, OgreSceneNodeComponent>. So if we use ComponentGroup as a key type, it can't be an std::tuple. We'd need to make it a class that holds a list of component type ids or similar. That said, the hash functions you linked should still work in principle.

Oh, and thanks for reminding me about the pull request. I must shamefully admit that I kind of forgot about it. I added some (really minor) notes.

@jjonj
Copy link
Contributor

jjonj commented Oct 28, 2013

Ah i understand now, alright. Well i'll leave you to implementing it and look at other issues then!

@ghost ghost assigned bkloster Oct 28, 2013
@bkloster
Copy link
Contributor Author

Some notes (mostly for myself while I'm working on this, so feel free to ignore):

  • Savegames must include the current gamestate
  • Gamestates should probably be named, both for serialization purposes (see above) and to retrieve a specific gamestate other than the current one
  • Should gamestates be completely configured (i.e. including the list of systems belonging to that state) from Lua? That would require exposing all system classes with at least their constructor. The advantage is that systems could be selectively enabled / disabled by just editing a script as opposed to compiling.
  • For now, the transition between gamestates can be a cut (i.e. just set the new state's scene manager to be the main viewport's source). Later, it might be nice to smoothly fade between the scenes by rendering them to textures and blending them together.

@jjonj
Copy link
Contributor

jjonj commented Oct 31, 2013

Looks good

Should gamestates be completely configured

I don't see a problem with exposing the systems to lua, ofc it's a bit of extra work tho, which has low priority.

@patowen
Copy link
Contributor

patowen commented Oct 31, 2013

A little bit off topic, but I didn't want to create a new ticket for this:

Why is the prototype branch back?

@jjonj
Copy link
Contributor

jjonj commented Oct 31, 2013

I'm guessing it's the branch nimbal is using for this :P

@bkloster
Copy link
Contributor Author

Actually, no. I'm as surprised as you are that the prototype branch (and hud-agent-addition) is back on GitHub.

@jjonj
Copy link
Contributor

jjonj commented Oct 31, 2013

Actually i think it is my fault. My laptop had those branches locally, and when i pushed the cooldown code from it, it must have ressurected those branches. Sorry

@bkloster
Copy link
Contributor Author

Oh, right. In the push dialog of TortoiseGit, there's a checkbox for "Push all branches", which might be the culprit.

@bkloster bkloster mentioned this issue Nov 3, 2013
jjonj added a commit that referenced this issue Nov 13, 2013
Changed existing compounds energy to glucose in emitters and vacuoles.
Added processing organelle producing ATP from glucose and oxygen.

Squashed commits:
Added ProcessOrganelle class in process_organelle.lua. Not yet used in Microbe.

Fixed syntax in process_organelle.lua and added hasInputAgent method to ProcessOrganelle class

Added ProcessOrganelle support into Microbe class, and added process_organelle.lua to manifest

Added code to setup.lua to test ProcessOrganelle (currently not working)
Minor fixes to ProcessOrganelle related code in process_organelle.lua and microbe.lua

Minor formatting and reintroduced function missing in process_organelle.lua

Fix some minor errors in the ProcessOrganelle script

* Use table.insert instead of a[#a+1] to not repeat the table name
* Call superclass' constructor in ProcessOrganelle.__init
* Explicitly pass self to Organelle.update in ProcessOrganelle.update
* Lua has no -= (or += for that matter)
* Typos

Fixed error in loading a savegame with ProcessOrganelle.

Created cooldown system for Process organelles, limiting the frequency of agent absorbtion and production

Fixed missing local specifier on two variables in process_organelle.lua

Alligned the text on the hud.

Merged 54_coding_styleguide adding guide for coding style.

Add coding style guide

See #54

Fix typo

Implemeneted colours for process organelles to reflect how filled it is. Also minor bugfixes

Move "local" to where it belongs

Made distribution of agents, dependant on time elapsed instead of frames. (1 agent per agent type distributed per 100ms)

Add convenient random number generator

The RNG class defined in engine/rng.h offers a convenient API
to generate random numbers in specific intervals. The Engine
class holds an RNG object and offers it for clients to use.

See #55.

The following commit messages have been preserved from the
original feature branch for posterity.

Fixed compiler error for RNG manager. Still needs testing and integration

Renamed RNGManager to RNG

Fixed renaming of files: RNG to rng
Added unit tests for RNG.

Extra commit. Git is messing with me.

Added RNG object to the engine and refactored agent.cpp to use that object for random agent angles.

Added missing unique_ptr and destructor
added RNG.shuffle method with tests.

Rename RNG.* to rng.*

Update includes to use lower-case rng.h

Fix unit test for RNG::shuffle

Fixed proper exposing of RNG class to lua, and made a global rng object avaliable.
Changed fixed seed to random in engine rng object.

Updated interface for RNG

Removing obsolete random header

Move RNGSeed typedef into RNG class

Clean up RNG headers

Cleanup RNG formatting

Minor documentation changes to RNG

Remove constructors from RNG Lua bindings

For now, we don't want script authors to get any ideas about
creating their own RNGs instead of using the one provided by
Engine.

Update styleguide with new instructions for git merging

Release candidate 1 Done.

- Fixed compile error
- Removed debugging code
- Changed setup of compounds/agents to reflect mitochondrion

Added additional documentation for processing compounds.
Minor refactoring to compound distribution for more accuracy.

Replaced magic value with proper constant

Added the SpawnSystem and created a test factory function in setup.lua

Merge pull request #3 from Revolutionary-Games/master

Syncing

Added spawn_system.lua to create a simple SpawnSystem and created a test factory function in SpawnSystem's constructor.

Added a newline to the end of spawn_system.lua

Made a few fixes to spawn_system.lua and moved the test function to setup.lua.

Changed the spawning algorithm. It should now be good for all stationary entities.

Randomized the orientation of spawned emitters

Fixed issue with local/global variables and unhelpful variable names, and changed the spawning method to utilize the built in functions of Vector3

Merge branch 'master' of https://github.com/Revolutionary-Games/Thrive

Fixed some bugs, changed addSpawnType to use spawnDensity rather than spawnFrequency, and added several comments.

Fixed some bugs, changed addSpawnType to use spawnDensity rather than spawnFrequency, and added several comments.

Add GameState concept

Closes #46

* Systems are now initialized by their GameState instead of the
  Engine directly
* Mouse and keyboard are not handled by systems anymore, but are
  special objects updated by the Engine object directly
* Saving and loading are not handled by systems anymore, but
  by the Engine object directly
* Entity filters now take the game state instead of the
  entity manager in their init function. The rationale of
  this change is that an entity is better thought of as
  belonging to a game state than to an entity manager,
  which is really just a book keeper.

Fix crash when GameState has no viewport system

Fix ownership transfer of systems when adding new GameState from Lua

Add GameState bindings to Lua

They don't contain anything (yet), but are required because
System::init takes a GameState parameter.

Expose Engine::setCurrentGameState to Lua

Add GameState testing script

Add Lua bindings to all systems

Change MicrobeControlSystem to use Keyboard instead of KeyboardSystem

Fix MicrobeSystem::init() for game states

Switch microbe setup script to game states

Remove deprecated ADD_SYSTEM

GameStates should properly shut down their systems

Remove deprecated files

Re-enable saving / loading

Only allow the Engine to create GameStates

Update documentation

Rename System::active to System::enabled

To avoid confusion with the next commit

Enable systems to react to their GameStates activation / deactivation

This removes the ugly hack of the GameState explicitly asking
the OgreViewportSystem to remove / restore the viewports.

Remove GameState::setPhysicsDebugDrawingEnabled

Clients should just enable / disable the debug draw system

Add GameState::name()

Pass around GameState pointers instead of their names

Add Keyboard::wasKeyPressed and Keyboard::wasKeyReleased

These methods are more convenient than systems keeping track of
the key state themselves. Furthermore, they solve the problem of
systems not knowing the actual key state when activated by
a key stroke.

Make TextOverlaySystem handle state switch gracefully

All overlay elements need to be removed / restored on a state
switch.

Refactor OgreViewportSystem to be closer to TextOverlaySystem

Reduces code duplication

Make Entity class use GameState in its public API

Script authors shouldn't have to care about the EntityManager.
They *should* care about the game state an entity is living in.
This change reflects this.

Unfortunately, since GameState cannot exist without a
full-fledged Engine object, this change makes Entity pretty
awkward to test, so the tests have been removed for now.

Add initializer function to GameState

Update scripts to use GameState initializer

Also adds a SwitchGameStateSystem that toggles between two
alternate microbe universes on pressing F1. Purely for testing.

Reinstated entity test

Only switch game state at the beginning of a frame

Immediately activating a game state during setCurrentGameState
can potentially cause problems as the systems are only partially
updated during the current frame.

Make EntityFilter handle additions / removals in the same frame

If an entity is added to a filter and then removed during the
exact same frame (and before clearChanges() has been called on
the filter), remove that entity from the addedEntities collection
and *don't* add it to removedEntities. For an observer of the
filter, it's as if the entity never existed, after all.

Remove debug output

Safeguard some systems against loading, then switching game state

For RigidBodySystem, OgreCameraSystem and OgreLightSystem, there
is a situation where a new game is started, a savegame is loaded
and then the gamestate is switched. The systems try to remove
internal data structures because due to the game loading, some
entities were removed. Those entities were never initialized with
internal data, however, which can lead to crashes when trying
to remove that data.

The simple solution is to skip the removal for non-initialized
internal data.

Clear entity changes after restoring all entities

TextOverlaySystem and OgreViewportSystem reinitialize all their
entities on activation. Not only is it efficient to clear their
filters' changes after that, it also prevents a crash related to
loading a savegame.

The crash occurs when a savegame is loaded that contains
matching entities with the same id as the current game. In the
systems' activate functions, the newly loaded entities' internal
state is restored. However, the entity filter still has those
ids in its "removed" set as they were also present in the old game
world and have been replaced. Thus, the internal state of the
entities is cleared during update(). Unfortunately, this doesn't
clear the component's pointer to the internal state, which makes
it difficult to detect that situation and deal with it accordingly.

The easiest solution is the one implemented. Resetting the
entity filter will prevent the unwanted clearing of internal
state.

Add missing documentation

Remove unused code

Rename Engine::addGameState to Engine::createGameState

Add Engine::getGameState()

Update documentation

Fix spawn system initialization in game state

Merged with master. added random spawning for glucose and oxygen instead of energy
hhyyrylainen pushed a commit that referenced this issue Aug 30, 2019
* updating vector types

* doing the suggested changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants