CModelInfoSA::IsValid has mixed logic, probably because of expanded GTA limits

[patrikjuvonen]

Describe the bug
CModelInfoSA::IsValid has mixed logic, where any model ID between 20000 and MODELINFO_MAX is always considered valid. This causes a problem because it seems interface isn't always initialized (correctly)? Calling certain model info using a model ID (i.e. 20000) results in a crash (see #446 for crash info).

See https://github.com/multitheftauto/mtasa-blue/blob/master/Client/game_sa/CModelInfoSA.cpp#L517

bool CModelInfoSA::IsValid()
{
    if (m_dwModelID >= 20000 && m_dwModelID < MODELINFO_MAX)
        return true;
    return ppModelInfo[m_dwModelID] != 0;
}

To reproduce

uint        uiModelID = 20000;
ushort      usModelID = CModelNames::ResolveModelID(uiModelID);
CModelInfo* pModelInfo = g_pGame->GetModelInfo(usModelID);
if (pModelInfo)
    float fDistance = pModelInfo->GetLODDistance(); // Crash

Expected behaviour
Properly return false if the ID is not actually valid.

Version
1.5.6-release-16542.0.000

Additional context

• See engineSetModelLODDistance(20000,1000) causes client crash #446 -> 0009139: fix invalid model ID in engine LOD functions cause a crash #299
• We need to remove any boundary checks (usModelID < 20000) outside CModelInfoSA in the future, as CModelInfoSA::IsValid should return correct status.

#299 (comment) @qaisjp commented:

This bug seems to be associated with https://github.com/multitheftauto/mtasa-blue/pull/155/files.

It returns true on this line: https://github.com/multitheftauto/mtasa-blue/pull/155/files#diff-3beea7b726cdd57f96a6d8226af4eb9fR538

Removing the offending line does not fix the problem. Returning false fixes the problem. Is return true a typo?

#299 (comment) @Sergeanur commented:

I don't quite remember, but I think CModelInfoSA::IsValid was used on animation request, so return true was required.

#784 (comment) @forkerer commented:

As far as I'm aware, (and in the IDB i'm using), 20000 is original size of that array in gta sa, trying to change physical properties of anything above that crashes the client. I think the additional space reserved in CGameSA (up to 26000) was made either by mistake, or in case of mta team wanting to expand some limits.

[ccw808]

IDs above 19999:
Textures(TXD) 20000 - 24999
Collisions(COL) 25000 - 25255
Item Placement(IPL) 25256 - 25510
Paths(DAT) 25511 - 25574
Anims(IFP) 25575 - 25754