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!

[Dutchman101]

Thanks for addressing all code reviews, the ones requested by me as per this comment were also fulfilled.

Good work with the PR, merging, @G-Moris

// Edit: you need to address this one asap, for now we'll have the PR's changes in (and in the upcoming build) with that review comment up for clarification later, because the PR already gives more performance boost than any (trivial) effect of copying data would, so it'll allow early testing and performance benefits for users. Still, answer it, and resolve it properly, thanks.

[Dutchman101]

Comment due to #mtasa-blue in discord bridge not having that this PR was in fact merged.

After discussing with TheNormalnij, we need to keep these things in mind too: #3848 (comment) (see "Edit" at bottom)

[G-Moris]

Sorry, I'll make the edits today, I just didn't have time before.