Idea for runtime configuration

[antsouchlos]

I have an idea for the runtime configuration of the engine. I have not decided yet, whether it would make our life easier or be utterly horrible.

We have so far decided, that we want to have a singleton class at the center of everything, managing all runtime configuration. We could combine this with a distributed approach, i.e. define configuration options for each part of the engine at will in this part of the engine.

Imagine a class similar to the following:

template<ParamType_>
class config_option_t {
public:
    config_option_t(std::string name) {
        auto& ref = global_system_singleton_t::get_instance();
        ref.register_option<ParamType_>(name);
    }

    void set_value() { /* ... */}
    ParamType_ get_value() { /* ... */}
private:
     // ...
};

This way, the physics engine for example could define all the config options it requires without altering any common code:

class physics_engine_t {
public:
    void do_smth() {
        float m = 1;
        float h = 20;
        float g = gravity_.get_value();

        float pot_energy = m*g*h;
    }
    // ...
private:
    config_option_t<bool>  enable_collisions_{"enable_collisions"};
    config_option_t<float> gravity_{"gravity"};
};

The advantage of this approach is of course also its big drawback: distributing the configuration code. On one hand it makes everything a lot easier to extend, on the other the configuration code gets spread out all across the codebase

[philsegeler]

OK, let me start by dissing your idea :)

THE UGLY

Since all subsystems including the physics engine are base classes that have to be subclassed, your proposal would look more like this in reality in dummy_classes.h:

 class physics_base_t : public OE_THREAD_SAFETY_OBJECT {
    public:
        physics_base_t() = delete;
        physics_base_t(std::string);
        virtual ~physics_base_t();

        virtual bool init(physics_init_info);

        // The below function is useless with your proposed approach
        //virtual void update_info(physics_update_info) noexcept;

        virtual bool update_multi_thread(const task_info_t, int);
        virtual void destroy();

        const std::string get_name();

    protected:
        const std::string name_;
        config_option_t<bool>  enable_collisions_{"enable_collisions"};
        config_option_t<float> gravity_{"gravity"};
    };

This is very similar to the previous solution. The only advantages are a nicer but more annoying interface and the ability to survive physics engine restarts.

THE BAD

1. No thread safety

Sure you could insert a mutex inside config_option_t. But that would not solve the cases of someone trying to change gravity while you are in the middle of a collision detection computation. In the current approach runtime configuration is updated once with update_info and you are supposed to copy all the configuration internally yourself.

In order to fix the above issue you would surely have to copy the config variables at the start of update_multi_thread. That would also mean locking and unlocking a lot more mutexes for each variables, instead of once as it is done in task_manager.cpp line 385 inside OE_TaskManager::update_thread:

        // SYNCHRONIZE PHYSICS
        lockMutex();
        physics_threads++;
        comp_threads_copy = physics_threads;
        if (physics_threads > (get_ready_threads() - 1)) {
            physics_threads = 0;
            this->physics_mutex.lockMutex();
            if (not this->physics_init_errors) this->physics->update_info(this->physics_info); // HERE IS YOUR CONFIGURATION UPDATED
            this->physics_mutex.unlockMutex();
            condBroadcast(3);
        }
        else {

            condWait(3);
        }
        unlockMutex();

        /**************************/
        // This is where physics are run
        this->threads[thread_id]->physics_task.update();
        if (not this->physics_init_errors)
            done_ = done_ or this->try_run_physics_update_multi_thread(thread_id, comp_threads_copy);
        /**************************/

        this->sync_end_frame();

2. Decentralisation.

In the current approach the task manager handles all the configuration whether runtime or initial. The initial configurations need global variables as well, but runtime configuration is only stored inside the task manager itself in task_manager.h line 71:

    /**********************************/
    // public member variables

    OE_THREAD_SAFETY_OBJECT  renderer_mutex;
    oe::renderer_base_t*     renderer{nullptr};
    oe::renderer_init_info   renderer_init_info;
    oe::renderer_update_info renderer_info; // RUNTIME CONFIGURATION

    OE_THREAD_SAFETY_OBJECT physics_mutex;
    oe::physics_base_t*     physics{nullptr};
    oe::physics_update_info physics_info; // RUNTIME CONFIGURATION
    oe::physics_init_info   physics_init_info;

    OE_THREAD_SAFETY_OBJECT window_mutex;
    oe::winsys_base_t*      window{nullptr};
    oe::winsys_update_info  window_info; // RUNTIME CONFIGURATION
    oe::winsys_init_info    window_init_info;
    oe::winsys_output       window_output;

    oe::networking_base_t*   network{nullptr}; //TODO: RUNTIME CONFIGURATION
    oe::networking_init_info network_init_info;

These are also the only public member variables of the task manager. Sure I might have to make the actual winsys/renderer/physics/network private, which I just discovered might make sense, but this is unrelated and for another issue ;)

3. The worst case scenario in your approach assuming you solve (1.) would amount to locking/unlocking the variable mutexes twice instead of only once for the struct with the current approach + 1 for the actual copying.

4. The overhead of copying a few trivially copyable variables in negligible to computing collision detection for thousands or vertices or rendering hundreds of objects. :)

5. Your approach is worse than the current one performance wise not only for the worst case, but in every way imaginable assuming you fix (.1) In a game engine it is more important to optimize for the worst case scenario, since we want consistent framerates anyways. Actually your best case scenario would be exactly like the worst case scenario with the current approach. :P

6. With your current approach the physics engine configuration options would only initialize when the physics engine initializes which would be problematic if we have runtime and initial config options on the same file. This more relevant for other subsystems but you get the point I think.

THE GOOD

Your idea of a configuration class config_option_t is not bad at all. This could in theory simplify reading/writing configuration from/on files. Although I prefer the current approach, we could put those variables in the structs themselves: (dummy_classes.h line 145)

    struct physics_update_info { // This should be what the physics engine sees in update_info
        float gravity{-9.81f};
    };
    struct physics_update_info_master { // This should be stored in the task manager
        physics_runtime_config_option_t<float> gravity{"gravity", -9.81f};
        physics_update_info get_data():
    };    

The actual physics engine would use physics_update_info just like it used to and the variables will still be kept inside the task manager. The global configuration class would then be useful for reading/writing from files.

We could make child classes subclassing config_option_t for the different subsystems like physics_runtime_config_option_t.

Drawback would be 2x more code changes if one wants to change or add/remove configuration options and of course the overhead of accessing a map of strings for the actual value. But then again with the current approach it would also be 2x more code changes to implement read/write from files in theory.

Is there maybe a way for any C++20 meta-programming feature to automatically produce physics_update_info_master ?