Code Review et optimisation

[FloreauLuca]

Comme nous avons pas beaucoup eu le temps de revoir mon code en cours, pourrais-je avoir un rapide code review sur mon ResourceManager et mon Test, ainsi que quelque piste d’optimisation

Branche sprint3_physfs
resource.h

#pragma once

#include <map>
#include <future>
#include "sole.hpp"
#include "utilities/file_utility.h"

namespace neko
{

using ResourceId = sole::uuid;
using Path = std::string_view;
const ResourceId INVALID_RESOURCE_ID = sole::uuid();


/**
 * \brief Custom promise waiting for a resource
 */
struct LoadPromise {
    BufferFile resource;
    Path path = "";
    bool ready = false;
    LoadPromise(Path newPath) : path(newPath) {}
    LoadPromise() {}
};

class ResourceManager
{
public:
    ResourceManager();
    void Destroy();
    bool IsResourceReady(ResourceId resourceId);
    std::string GetResource(ResourceId resourceId);
    ResourceId LoadResource(Path assetPath);
private:
    void LoadingLoop();
    enum ResourceManagerStatus : std::uint8_t
    {
        IS_RUNNING = 1, //To check if the ResourceManager is running
        IS_NOT_EMPTY = 10, //To check if the ResourceManager has tasks
        IS_WAITING = 100, //To check if the ResourceManager is waiting for a task
    };
    std::unordered_map<ResourceId, LoadPromise> resourcePromises_;
    std::vector<ResourceId> idQueue_;
    std::thread loadingThread_;
    std::atomic<std::uint8_t> status_;
    std::condition_variable cv_;
    std::mutex loadingMutex_;
    
};
}

resource.cpp

#include <engine/resource.h>

#include "engine/assert.h"
#include "utilities/file_utility.h"
#include "utilities/json_utility.h"

neko::ResourceManager::ResourceManager()
{
    status_ = IS_RUNNING;
    loadingThread_ = std::thread([this]() { this->LoadingLoop(); });
}

void neko::ResourceManager::Destroy()
{
    status_ &= ~IS_RUNNING;
    cv_.notify_one();
    loadingThread_.join();
}

void neko::ResourceManager::LoadingLoop()
{
    while (status_ & IS_RUNNING) 
    {
        if (status_ & IS_NOT_EMPTY) 
        {
            std::lock_guard<std::mutex> lockGuard(loadingMutex_);
            Path path = resourcePromises_[idQueue_[0]].path;
            resourcePromises_[idQueue_[0]].resource.Load(path);
            resourcePromises_[idQueue_[0]].ready = true;

            const size_t extensionIndex = path.find_last_of('.');
            std::string metaFilePath = path.data();
            metaFilePath = metaFilePath.substr(0, extensionIndex).append(".meta");
            json metaJson = json::object();
            metaJson["resourceID"] = json::string_t();
            metaJson["resourceID"] = idQueue_[0].str();
            WriteStringToFile(metaFilePath, metaJson.dump(4));

            idQueue_.erase(idQueue_.begin());
            if (idQueue_.empty()) 
            {
                status_ &= ~IS_NOT_EMPTY;
            }
        } else 
        {
            std::unique_lock<std::mutex> lock(loadingMutex_);
            if (!(status_ & IS_NOT_EMPTY))
            {
                cv_.wait(lock);
            }
        }
    }
}

bool neko::ResourceManager::IsResourceReady(ResourceId resourceId)
{
    const std::lock_guard<std::mutex> lockGuard(loadingMutex_);
    return resourcePromises_[resourceId].ready;
}


std::string neko::ResourceManager::GetResource(ResourceId resourceId)
{
    const std::lock_guard<std::mutex> lockGuard(loadingMutex_);
    if (resourcePromises_[resourceId].ready)
    {
        return resourcePromises_[resourceId].resource.dataBuffer;
    } else
    {
        neko_assert(true, "Resource not ready");
    }
}

neko::ResourceId neko::ResourceManager::LoadResource(const Path assetPath)
{
    const std::lock_guard<std::mutex> lockGuard(loadingMutex_);
    const ResourceId resourceId = sole::uuid4();
    idQueue_.push_back(resourceId);
    resourcePromises_[resourceId] = LoadPromise(assetPath);
    status_ |= IS_NOT_EMPTY;
    cv_.notify_all();
    return resourceId;
}

test_resource.cpp

#include <gtest/gtest.h>
#include <engine/resource.h>

#include <sole.hpp>
#include <utilities/file_utility.h>

TEST(Engine, TestResource)
{
    std::string path = "./../../data/test/test.txt";
    std::string test = neko::LoadFile(path);
    neko::ResourceManager resourceManager;
    neko::ResourceId resourceId = resourceManager.LoadResource(path);
    int count = 0;
    while (!resourceManager.IsResourceReady(resourceId))
    {
        count++;
    }

    EXPECT_TRUE(resourceManager.GetResource(resourceId) == test);
    EXPECT_FALSE(resourceManager.IsResourceReady(sole::uuid0()));
    resourceManager.Destroy();
}

[EliasFarhan]

Avant tout je m'intéresse au test_resource.cpp, car c'est le fichier le plus important pour comprendre ce que fait votre ResourceManager et comment l'utiliser:

• std::string path = "./../../data/test/test.txt"; Il faut préférer utiliser config.dataRootPath (quitte à créer un object Configuration juste pour avoir le dataRootPath.
• test n'est pas un bon nom pour le contenu textuel du fichier.
• Plutôt que sole::uuid0(), ne serait-il pas judicieux de créer une constante random_uuid?
• Je ne comprends pas à quoi sert le count dans le while? Est-ce un reste de debug?
• Le Resource Manager lance son thread dans le constructeur, mais il faut appeler Destroy() pour l'arrêter. Ce n'est pas très RAII et pour Neko, la logique est d'utiliser plutôt un paradigme Init+Destroy pour contrôler quand le système s'initialise et quand il se détruit (ce qui peut être ensuite register dans l'engine s'il hérite de la classe neko::System).

Ensuite, pour resource.cpp:

• Ta section critique comprend le fait de loader tout le fichier et d'écrire un meta file. C'est BEAUCOUP trop. La section critique est dans le fait de prendre un élément de la queue est c'est TOUT. C'est le genre de chose dont on se rend compte quand on fait du benchmark ou qu'on stress teste son programme. Il faut donc mesurer combien de temps cela prend d'appeler chaque fonction (avec l'implémentation actuelle, LoadResource va prendre chère).
• Le .meta s'ajoute au nom de fichier complet (data/hello.jpg -> data/hello.jpg.meta), sinon tu risques d'avoir des suprises avec des noms de fichiers similaires, mais avec des extensions différentes (modèle 3d: data/wall.obj et texture: data/wall.jpg)
• Pour l'instant, laissez de côté les fichiers .meta, on mettra tout ça en place avec le neko editor.
• Il me faut un moyen de libérer une ressource (Destroy Resource?)
• Il faut retourner tout le BufferFile, pas juste le dataBuffer, parce qu'il faut le pointeur et la taille du fichier.
• neko_assert(true, "Resource not ready"); ne va jamais assert... il faut mettre false.

[FloreauLuca]

Est-ce que je suis obligé d'utiliser les sole::UUID ?

[FloreauLuca]

En fait c'est juste le premier appelle de l'uuid4() qui prend du temps les suivant sont très rapide

[EliasFarhan]

Bah tu peux l'appeler avant le while de ton Loading Loop ;)