Skip to content

Conversation

@Pirulax
Copy link
Contributor

@Pirulax Pirulax commented May 13, 2020

Okay, so in theory this fixes #1436 just fine.

Another way couldve been to just do this:

    for (int i = 0; i <= MAX_MODEL_ID; i++)
    {
        if (CClientPlayerManager::IsValidModel(i))
        {
            lua_pushnumber(luaVM, ++iIndex);
            lua_pushnumber(luaVM, i);
            lua_settable(luaVM, -3);
        }
    }

but i felt like that would have way bigger overhead than doing it this way. But correct me if im wrong.

@PlatinMTA
Copy link
Contributor

As a hotfix maybe you want it to go until ID 5389, because that is the last allocable skin for peds. It will skip like 1500 loops so i guess it's worth it

@Pirulax
Copy link
Contributor Author

Pirulax commented May 13, 2020

As a hotfix maybe you want it to go until ID 5389, because that is the last allocateable skin for peds. It will skip like 1500 loops so i guess it's worth it

Are you a 100 percent sure? Another approach would be to just:

std::vector<std::vector<CClientModel*>> m_modelsByType;

or an std::map, for the first one. But Im not sure if thats necessary. To be quite honest, that seems overcomplicated to me, because nobody uses this function in render(if so, then they're plain stupid).

Copy link
Member

@sbx320 sbx320 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a few minor comments. Another alternative would be to only track added models (and hardcode the rest), but that'd be fragile and prone to future breakage if other model types are added.

@Pirulax
Copy link
Contributor Author

Pirulax commented May 13, 2020

Oh god, im so stupid at git, please help 🤣

error: pathspec 'MTASA.sln.lnk' did not match any file(s) known to git

while trying to run:
git commit *.* -m "made changes as suggested by sbx"

Update: Ive googled around a bit more, and found out about -a. Now it works 😅

git commit -a -m "made changes as suggested by sbx"

@Pirulax Pirulax changed the title fixed fix #1436 - getValidPedModels() to include engineRequestModel skins as well. May 13, 2020
@StrixG StrixG changed the title fix #1436 - getValidPedModels() to include engineRequestModel skins as well. Fix #1436: getValidPedModels() to include engineRequestModel skins as well May 13, 2020
@StrixG StrixG added the bug Something isn't working label May 14, 2020
@Pirulax
Copy link
Contributor Author

Pirulax commented May 22, 2020

Is this fine as it is?

My professional test method was:

local testCount = 100;

local requesteds = {}
for i = 1, testCount do
    requesteds[engineRequestModel("ped")] = true;
end

local found = 0;
for _, id in pairs(getValidPedModels()) do
    if (requesteds[id]) then 
        found = found + 1
     end
end
assert(found == testCount, "oh shit... here we go again!")

@StrixG StrixG added this to the Backlog milestone Jul 1, 2020
@ghost ghost merged commit ed2abb3 into multitheftauto:master Oct 27, 2020
@ghost ghost modified the milestones: Confirmed Issues, 1.6 Oct 27, 2020
@Pirulax
Copy link
Contributor Author

Pirulax commented Oct 27, 2020

Issues:

  • Because of the way engineFreeModel works ID's arent actually unreferenced (Removed) in the model manager, which causes this function to return free-d Ids
  • Perhaps just add a new function: engineGetModelsByType as suggested by @qaisjp

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

getValidPedModels() won't check if and id was allocated as a ped skin after ID 312

4 participants