Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: replace atomic models with clump models #3003

Closed
wants to merge 25 commits into from
Closed

Feature: replace atomic models with clump models #3003

wants to merge 25 commits into from

Conversation

CrosRoad95
Copy link
Contributor

@CrosRoad95 CrosRoad95 commented May 12, 2023

Goal of this pull request is to make possible to replace atomic models ( model with one submodel ) with clump models ( model with multiple submodels ).
To do this, i created new function that converts model engineConvertAtomicToClumpModel( modelId )
Only "atomic" models can be converted to clumps, otherwise function return false
Probably better solution would be to add function to change model type, but in practice you want "atomic" or "clump" objects, if you want new vehicles, peds models just request new id's.
In my opinion it should be separete function because attempt to put it into engineReplaceModel may lead to a lot of confusions.
Also, i consider another function to get model type, so for example by default: 370 returns "weapon", 404 "vehicle", 3425 "clump", 1337 "atomic".

engineConvertAtomicToClumpModel(1337)
a = createObject(1337, 4, 10, 5)
b = createObject(370, 15, 10, 5) -- don't need to convert to clump because this is model that already is clump
dff = engineLoadDFF ( "nt_windmill.dff" )
txd = engineLoadTXD ( "farmstuff.txd" )
engineImportTXD ( txd, 1337 )
engineImportTXD ( txd, 370 )
engineReplaceModel ( dff, 1337 )
engineReplaceModel ( dff, 370 )

addCommandHandler("restore", function()
  engineRestoreModel ( 1337 )
  engineRestoreModel ( 370 )
end)

before, only last atomic is visible:
image
after, all (2) atomics are visible:
image
1337 model on the left, 370 jetpack on the right

This pull request is still in progress, assume there are some memory leaks, i consider if it is important to revert model back to atomic

@lopezloo lopezloo added the enhancement New feature or request label May 12, 2023
@CrosRoad95 CrosRoad95 marked this pull request as ready for review May 12, 2023 18:37
@lopezloo
Copy link
Member

lopezloo commented May 19, 2023

i consider if it is important to revert model back to atomic

It must be done on disconnect and engineRestoreModel. Is it already implemented?

Is the new API (engineConvertAtomicToClumpModel) really needed? Couldn't it be done automatically in engineReplaceModel (convert model to clump if specified model has multiple atomics)?

@CrosRoad95
Copy link
Contributor Author

i consider if it is important to revert model back to atomic

It must be done on disconnect and engineRestoreModel. Is it already implemented?

Is the new API (engineConvertAtomicToClumpModel) really needed? Couldn't it be done automatically in engineReplaceModel (convert model to clump if specified model has multiple atomics)?

it may be a case where people already have had models with multiple atomics/clump models but they didn't know and this change may cause old code to not be fully compatible thats why i added this function.

"engineRestoreModel" is working,
and yea, maybe i should keep track which model has been converted to clump, and on disconnect revert changes

@CrosRoad95
Copy link
Contributor Author

in ~2 weeks i'm going add functionality where models changes using "engineConvertAtomicToClumpModel(1337)" will be reverted back to atomic when you left the server. if anyone has any objectives, let me know

@CrosRoad95
Copy link
Contributor Author

Now it works a bit differently, instead of "convert" you change internal type of model, for now only availiable options are "clump" and "atomic", limitation is that you can change from atomic to clump, but not from clump to atomic until you previously set it to clump. I have made it to prevent potential crashes because i'm 100% sure that crash will happen if you convert ped to atomic, atomic to vehicle ect.

engineSetModelInfoType(1337, "clump") -- valid because it was atomic
engineSetModelInfoType(1337, "atomic") -- valid because it originally was "atomic"
engineSetModelInfoType(3425, "atomic") -- invalid because it originally was "clump"
engineSetModelInfoType(370, "clump") -- not supported, throw error because this is "weapon" and for now this conversion is not supported

there's second function "engineGetModelInfoType" that return internal type of model, so for example engineGetModelInfoType(370) is weapon, engineGetModelInfoType(1337) is atomic, engineGetModelInfoType(3425) is clump
now if you leave server, all models are reverted back to atomic if they were changed to clump.
Someone in future can implement support for other type change so for example you will be able to convert clump to atomic ( what is kinda useless )

first you have to change type, then replace it ( or probably restream world when type change )

there are things i, you need to test:

  1. changing custom allocated models
  2. maybe there are streaming issues?
  3. maybe some models after conversion fails?

@lopezloo
Copy link
Member

lopezloo commented Jun 29, 2023

I think we should discuss more about possibility of merging this into existing API (engineReplaceModel). It's up to server owners to take care of models. If they have models with multiple atomics then it's their liability.

@CrosRoad95
Copy link
Contributor Author

I think we should discuss more about possibility of merging this into existing API (engineReplaceModel). It's up to server owners to take care of models. If they have models with multiple atomics then it's their liability.

removed all new lua functions, now if you replace model it get converted to clump automatically, restore will convert model back to atomic if needed, same when you leave the server.
Maybe functions responsible for conversion could be improved but apart of that i think this pr is ready to finish

@Pirulax
Copy link
Contributor

Pirulax commented Jul 9, 2023

What would be the use-case for this though?
Could you provide a few examples?

@CrosRoad95
Copy link
Contributor Author

mostly for future pull requests, it opens doors to many features.

return ((void(__cdecl*)(RpAtomic*, int))0x732230)(pRpAtomic, id);
}

typedef struct
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it's better not to use C-style struct declaration.


CModelInfo* pModelInfo = g_pGame->GetModelInfo(m_iModelID, true);

if (!pModelInfo->IsValid() || m_eModelType != eClientModelType::OBJECT)
Copy link
Member

Choose a reason for hiding this comment

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

GetModelInfo can return nullptr what should be taken into account in this conditional expression.

{
CModelInfo* pModelInfo = g_pGame->GetModelInfo(m_iModelID, true);

if (!pModelInfo->IsValid() || m_eModelType != eClientModelType::OBJECT)
Copy link
Member

Choose a reason for hiding this comment

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

The same

Client/game_sa/CModelInfoSA.cpp Outdated Show resolved Hide resolved
Client/game_sa/CModelInfoSA.cpp Outdated Show resolved Hide resolved
Client/game_sa/CModelInfoSA.cpp Outdated Show resolved Hide resolved
m_pInterface->pRwObject = nullptr;
((void(__thiscall*)(CAtomicModelInfoSAInterface * pThis, RpAtomic * pAtomic)) vfbl->SetAtomic)(m_pInterface, pClonedAtomic);

ppModelInfo[m_dwModelID] = m_pInterface;
Copy link
Member

Choose a reason for hiding this comment

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

The same potential memory leak

Client/game_sa/CModelInfoSA.cpp Outdated Show resolved Hide resolved
Client/game_sa/CModelInfoSA.cpp Outdated Show resolved Hide resolved
@CrosRoad95 CrosRoad95 closed this by deleting the head repository Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants