Optimization Handling (part 3) and Remove big use eVehicleTypes

[G-Moris]

Continuation of PR - #3580;
Removed the frequent useless use of eVehicleTypes.

[Fernando-A-Rocha]

Good initiative! 😀

[G-Moris]

@TracerDS Hello. Maybe I should add WriteDebugEvent to some events?

[TracerDS]

@TracerDS Hello. Maybe I should add WriteDebugEvent to some events?

what for

[G-Moris]

@TracerDS Hello. Maybe I should add WriteDebugEvent to some events?

what for

For try-catch

Example:

void CHandlingEntrySA::Recalculate() noexcept
{
    // Real GTA class?
    if (!m_HandlingSA)
        return;

    try
    {
        // Copy our stored field to GTA's
        memcpy(m_HandlingSA.get(), &m_Handling, sizeof(m_Handling));
        ((void(_stdcall*)(tHandlingDataSA*))FUNC_HandlingDataMgr_ConvertDataToGameUnits)(m_HandlingSA.get());
    }
    catch (...)
    {
        WriteDebugEvent("Failed recalculate vehicle handling!");
    }
}

[TracerDS]

@TracerDS Hello. Maybe I should add WriteDebugEvent to some events?

what for

For try-catch

Example:

void CHandlingEntrySA::Recalculate() noexcept
{
    // Real GTA class?
    if (!m_HandlingSA)
        return;

    try
    {
        // Copy our stored field to GTA's
        memcpy(m_HandlingSA.get(), &m_Handling, sizeof(m_Handling));
        ((void(_stdcall*)(tHandlingDataSA*))FUNC_HandlingDataMgr_ConvertDataToGameUnits)(m_HandlingSA.get());
    }
    catch (...)
    {
        WriteDebugEvent("Failed recalculate vehicle handling!");
    }
}

No need.
In this case that try catch is kinda meh. Only ConvertDataToGameUnits could potentially throw but judging from the name it shouldnt

[G-Moris]

@TracerDS Would it be worth making AddVehicle return std::unique_ptr?

[TracerDS]

@TracerDS Would it be worth making AddVehicle return std::unique_ptr?

if it returns raw pointer then yes

[G-Moris]

@TracerDS Do you like the idea of refining the function this way?

CHandlingEntry* CHandlingManager::GetModelHandlingData(std::uint32_t model) const noexcept
{
    // Within range?
    if (!CVehicleManager::IsValidModel(model))
        return nullptr;

    auto entries = m_ModelEntries.find(model);
    if (entries == m_ModelEntries.end())
    {
        // Get our Handling ID
        eHandlingTypes eHandling = GetHandlingID(model);

        auto entry = std::make_unique<CHandlingEntry>(&m_OriginalHandlingData[eHandling]);
        entries = m_ModelEntries.emplace(model, std::move(entry)).first;
    }

    return entries->second.get();
}

[TracerDS]

@TracerDS Do you like the idea of refining the function this way?

CHandlingEntry* CHandlingManager::GetModelHandlingData(std::uint32_t model) const noexcept
{
    // Within range?
    if (!CVehicleManager::IsValidModel(model))
        return nullptr;

    auto entries = m_ModelEntries.find(model);
    if (entries == m_ModelEntries.end())
    {
        // Get our Handling ID
        eHandlingTypes eHandling = GetHandlingID(model);

        auto entry = std::make_unique<CHandlingEntry>(&m_OriginalHandlingData[eHandling]);
        entries = m_ModelEntries.emplace(model, std::move(entry)).first;
    }

    return entries->second.get();
}

I very much do not like the std::move(entry) on an unique_ptr. Btw thats not a refactor but a huge change how GetModelHandlingData fetches data.

this is pretty bad because it will always construct entry, even if insertion fails

[G-Moris]

@TracerDS Do you like the idea of refining the function this way?

CHandlingEntry* CHandlingManager::GetModelHandlingData(std::uint32_t model) const noexcept
{
    // Within range?
    if (!CVehicleManager::IsValidModel(model))
        return nullptr;

    auto entries = m_ModelEntries.find(model);
    if (entries == m_ModelEntries.end())
    {
        // Get our Handling ID
        eHandlingTypes eHandling = GetHandlingID(model);

        auto entry = std::make_unique<CHandlingEntry>(&m_OriginalHandlingData[eHandling]);
        entries = m_ModelEntries.emplace(model, std::move(entry)).first;
    }

    return entries->second.get();
}

I very much do not like the std::move(entry) on an unique_ptr. Btw thats not a refactor but a huge change how GetModelHandlingData fetches data.

this is pretty bad because it will always construct entry, even if insertion fails

@TracerDS Do you like the idea of refining the function this way?

CHandlingEntry* CHandlingManager::GetModelHandlingData(std::uint32_t model) const noexcept
{
    // Within range?
    if (!CVehicleManager::IsValidModel(model))
        return nullptr;

    auto entries = m_ModelEntries.find(model);
    if (entries == m_ModelEntries.end())
    {
        // Get our Handling ID
        eHandlingTypes eHandling = GetHandlingID(model);

        auto entry = std::make_unique<CHandlingEntry>(&m_OriginalHandlingData[eHandling]);
        entries = m_ModelEntries.emplace(model, std::move(entry)).first;
    }

    return entries->second.get();
}

I very much do not like the std::move(entry) on an unique_ptr. Btw thats not a refactor but a huge change how GetModelHandlingData fetches data.

this is pretty bad because it will always construct entry, even if insertion fails

std::move transfers the owner to m_ModelEntries, at least I don't see anything wrong or risky in this. Can set a condition with the "entry" check

[TracerDS]

std::move transfers the owner to m_ModelEntries, at least I don't see anything wrong or risky in this. Can set a condition with the "entry" check

No. std::move doesnt "move" anything.

[G-Moris]

std::move transfers the owner to m_ModelEntries, at least I don't see anything wrong or risky in this. Can set a condition with the "entry" check

No. std::move doesnt "move" anything.

It transfers ownership, as I know

[TracerDS]

It transfers ownership, as I know

No. It only converts the value to rvalue. Thats all. If the value is already a rvalue then the conversion does nothing.
It only marks the value can be transferred but does not automatically move the ownership.

This is the basic implementation of std::move:

template <typename T>
typename remove_reference<T>::type&& move(T&& arg)
{
  return static_cast<typename remove_reference<T>::type&&>(arg);
}

Lets format it into int only:

template<class T> struct remove_reference { typedef T type; };
template<class T> struct remove_reference<T&> { typedef T type; };
template<class T> struct remove_reference<T&&> { typedef T type; };

inline
int&& move(int&& arg)
{
  return static_cast<int&&>(arg);
}

As you can see no transfer is done by move manually.

[G-Moris]

As you can see no transfer is done by move manually.

Is that better?

// Get our Handling ID
eHandlingTypes eHandling = GetHandlingID(model);

m_ModelEntries[model] = std::make_unique<CHandlingEntry>(&m_OriginalHandlingData[eHandling]);
if (!m_ModelEntries[model])
    return nullptr;

entries = m_ModelEntries.find(model);

[TracerDS]

Is that better?

// Get our Handling ID
eHandlingTypes eHandling = GetHandlingID(model);

m_ModelEntries[model] = std::make_unique<CHandlingEntry>(&m_OriginalHandlingData[eHandling]);
if (!m_ModelEntries[model])
    return nullptr;

entries = m_ModelEntries.find(model);

Yes

[G-Moris]

Yes

Is it possible to change to this?

// Original handling data
static std::unordered_map<std::size_t, tHandlingData>                   m_OriginalHandlingData;
static std::unordered_map<std::size_t, std::unique_ptr<CHandlingEntry>> m_OriginalEntries;

// Model handling data
static std::unordered_map<std::size_t, std::unique_ptr<CHandlingEntry>> m_ModelEntries;
static std::unordered_map<std::size_t, bool>                            m_bModelHandlingChanged;

static std::map<std::string, eHandlingProperty> m_HandlingNames;

[TracerDS]

// Original handling data
static std::unordered_map<std::size_t, tHandlingData>                   m_OriginalHandlingData;
static std::unordered_map<std::size_t, std::unique_ptr<CHandlingEntry>> m_OriginalEntries;

// Model handling data
static std::unordered_map<std::size_t, std::unique_ptr<CHandlingEntry>> m_ModelEntries;
static std::unordered_map<std::size_t, bool>                            m_bModelHandlingChanged;

static std::map<std::string, eHandlingProperty> m_HandlingNames;

what are the original declarations?

[G-Moris]

what are the original declarations?

std::unordered_map will help us for a more dynamic and convenient use

    std::map<std::string, eHandlingProperty> m_HandlingNames;
    
    // Original handling data unaffected by handling.cfg changes
    static SFixedArray<tHandlingData, HT_MAX> m_OriginalHandlingData;

    // Array with the original handling entries
    static SFixedArray<std::unique_ptr<CHandlingEntry>, HT_MAX> m_OriginalEntries;

    // Array with the model handling entries
    static SFixedArray<std::unique_ptr<CHandlingEntry>, HT_MAX> m_ModelEntries;

    SFixedArray<bool, HT_MAX> m_bModelHandlingChanged;

[Dutchman101]

I've requested reviews from @TheNormalnij and @tederis, this PR has to be scrutinized for the same reasons as author's prior PR's. Thanks for the PR!

[TheNormalnij]

can be unique_ptr with access by referense

[G-Moris]

Should I change this on the server?

[G-Moris]

Should I change this on the server?

@TheNormalnij

[TheNormalnij]

yes