mtasa-blue icon indicating copy to clipboard operation
mtasa-blue copied to clipboard

dxDrawModel3D reimplementation

Open tederis opened this issue 2 years ago • 31 comments

Brings dxDrawModel3D function(#3172) back. The syntax is compatible with the previous one: bool dxDrawModel3D(int model, float x, float y, float z, float rx, float ry, float rz, [float sx = 1.0f, float sy = 1.0f, float sz = 1.0f).

tederis avatar Dec 09 '23 10:12 tederis

The mode argument is debatable. Maybe it's better to replace it with , bool blocking = false argument? What do you think?

tederis avatar Dec 09 '23 11:12 tederis

I feel draw functions shouldn't manage model loading. Using this draw function with blocking mode is totally wrong, it causes FPS drops. Async mode is bad too, it causes drawing in a wrong frame. So only "loaded" mode is good here. I think we need two additional functions for this:

bool engineLoadModel( number modelId, [ bool async = true, function callback)
bool engineUnloadModel( number modelId )

TheNormalnij avatar Dec 09 '23 12:12 TheNormalnij

I feel draw functions shouldn't manage model loading. Using this draw function with blocking mode is totally wrong, it causes FPS drops. Async mode is bad too, it causes drawing in a wrong frame. So only "loaded" mode is good here. I think we need two additional functions for this:

bool engineLoadModel( number modelId, [ bool async = true, function callback)
bool engineUnloadModel( number modelId )

I feel the same. But the nonblocking model loading is exactly what SA streamer is doing. I mean this is natural for SA.

engineLoadModel is good but the fact is there is no way to preserve a model in memory without a refs count increment. I'm not sure if preserving a model in memory by force is something good.

engineUnloadModel raises an another question. What if a model exists on a map when this function is called? Should we remove it as well? Or we should decrement a refs count?

Looks like these functions do not fit that easy in the context of dxDrawModel3D.

tederis avatar Dec 09 '23 13:12 tederis

Are the rendering issues (models becoming invisible at certain view-angles for example) fixed in this PR?

PlatinMTA avatar Dec 09 '23 16:12 PlatinMTA

Are the rendering issues (models becoming invisible at certain view-angles for example) fixed in this PR?

Of course. This PR is written from scratch keeping in mind all problems of the previous one. Moreover, it would be weird to just reopen a PR with the same problems.

tederis avatar Dec 09 '23 16:12 tederis

engineLoadModel is good but the fact is there is no way to preserve a model in memory without a refs count increment. I'm not sure if preserving a model in memory by force is something good.

engineUnloadModel raises an another question. What if a model exists on a map when this function is called? Should we remove it as well? Or we should decrement a refs count?

engineLoadModel and engineUnoadModel are bad names. First function should increase refs count and load model. Second function should decrease refs count and unload model when refs count is 0.

TheNormalnij avatar Dec 09 '23 17:12 TheNormalnij

engineLoadModel is good but the fact is there is no way to preserve a model in memory without a refs count increment. I'm not sure if preserving a model in memory by force is something good. engineUnloadModel raises an another question. What if a model exists on a map when this function is called? Should we remove it as well? Or we should decrement a refs count?

engineLoadModel and engineUnoadModel are bad names. First function should increase refs count and load model. Second function should decrease refs count and unload model when refs count is 0.

Okay, I'll think about it.

tederis avatar Dec 09 '23 18:12 tederis

I think such a function (that loads models) could be handy in other situations too (like pre-loading a section of the world for custom maps, etc). Also, in theory it's possible to add support for rendering a CClientDFF directly no?

Pirulax avatar Dec 12 '23 15:12 Pirulax

Hey, any updates on this?

Pirulax avatar Dec 27 '23 19:12 Pirulax

Hey, any updates on this?

Yes, I aggregated all suggestions so that the final PR will contain the following: dxDrawModel3D function that works with preloaded only models (or CClientDFF elements) engineLoadModel function engineUnloadModel function

It requires some polishing and the internal testing. I hope to finish it soon.

tederis avatar Dec 27 '23 20:12 tederis

Though, by using SA naming, engineStreamingRequestModel and engineStreamingRemoveModel (Or Unload?) might be more suitable?|, not sure, might be confusing, as in reality we're just adding/removing refs. I'm worried people might get confused as to what the difference between engineLoadDFF and engineLoadModel is

Pirulax avatar Dec 29 '23 16:12 Pirulax

Though, by using SA naming, engineStreamingRequestModel and engineStreamingRemoveModel (Or Unload?) might be more suitable?|, not sure, might be confusing, as in reality we're just adding/removing refs. I'm worried people might get confused as to what the difference between engineLoadDFF and engineLoadModel is

I thought about it too. And I have come up with the following syntax: bool engineStreamingRequestModel(uint model [, bool async = false, bool addRef = false]) -- Note that each resource can keep just one reference for each model.

bool engineStreamingReleaseModel(uint model) -- Removes the reference if it was previously incremented by engineStreamingRequestModel.

string engineStreamingGetModelState(uint model) -- Return "unavailable", "loading" or "loaded".

There also can be event "onClientModelLoaded". But I'm not sure if this event is actually needed.

tederis avatar Dec 31 '23 06:12 tederis

How will it work with addRef = false and addRef = true ?

TheNormalnij avatar Dec 31 '23 08:12 TheNormalnij

How will it work with addRef = false and addRef = true ?

The better name for this argument is "retain". It increments the ref count for the specific model. But only one additional reference per resource. When addRef = false it's just trying to load a model without any guarantees. It's the way SA is doing the streaming. addRef = false is convinient for the case when you need a model just in the current(async = false) or the next frame(async = true).

tederis avatar Dec 31 '23 08:12 tederis

"the next frame"

I remember gta uses some sort of multi threading for file loading. Does gta guarantee that the model will be loaded exactly on the next frame?

TheNormalnij avatar Dec 31 '23 08:12 TheNormalnij

"the next frame" I remember gta uses some sort of multi threading for file loading. Does gta guarantee that the model will be loaded exactly on the next frame?

No, even SA does not guarantee that. I previously mentioned the next frame to emphasize the asynchronous nature of the argument. Of course the precise moment isn't specified.

tederis avatar Dec 31 '23 08:12 tederis

engineStreamingRequestModel with addRef = false can be considered as "dear function, please try to load a model. But if it fails it's OK, at least you tried."

addRef = true is more aggressive and will try to load a model until the success(if the model is valid of course), plus adds a reference.

tederis avatar Dec 31 '23 08:12 tederis

I think it's even better to replace addRef = false with persistent = false. It's more illustrative.

tederis avatar Dec 31 '23 09:12 tederis

@tederis It is viable to have this feature now, right?

Fernando-A-Rocha avatar May 31 '24 09:05 Fernando-A-Rocha

@tederis It is viable to have this feature now, right?

There could be graphical issues with translucent objects but aside of it, it's viable. There were also proposals that complicate the entire system but they aren't essential. I will try to find time to give it another look.

tederis avatar May 31 '24 10:05 tederis

Will there still be any updates? I hope very much, as this function could be very useful

YSAFE avatar Jul 21 '24 02:07 YSAFE

This PR was initially small and simple. During code reviews several suggestions was made that complicate the initial idea. These suggestions may look minor but hide many edge cases that require thorough approach. In this regard I propose to separate dxDrawModel3D function from engineStreamingRequestModel/engineStreamingReleaseModel functions which will be addressed in the dedicated PR. This will close the breach in MTA Lua API and make this PR simpler to finish and merge it. What do you think @TheNormalnij ?

tederis avatar Jul 25 '24 16:07 tederis

I'll assist you with engineStreamingRequestModel and engineStreamingReleaseModel.

TheNormalnij avatar Jul 27 '24 10:07 TheNormalnij

Great, this pull will be of great importance.

YSAFE avatar Jul 27 '24 20:07 YSAFE

@tederis Please resolve conflicts. Also, with PR #3611 now merged, what else needs to be done, or is it about ready?

Dutchman101 avatar Aug 14 '24 18:08 Dutchman101

In my opinion, @Dutchman101, @tederis commented that the problem was the 3D model allocator, with that already resolved by PR #3611 we can comment that now we only need to resolve some parts.

YSAFE avatar Aug 14 '24 20:08 YSAFE

@tederis Please resolve conflicts. Also, with PR #3611 now merged, what else needs to be done, or is it about ready?

Done. I have also added a render path for transparent objects. So this PR is fully functional now. But there is still a question whether this function should always work with "loaded" models or there can be several modes(the current implementation) [See the second comment by @TheNormalnij]. My personal opinion is that there's nothing wrong with it and can be considered as a "sugar" for the convenience.

tederis avatar Aug 15 '24 04:08 tederis

I suggest to remove modes completely, because they have unpredictable behavior (the function doesn't guarantee render) and cause fps drops (in blocking mode) and model flickering (all modes). This function can guarantee predictable result in this example:

local modelId = 1337

local function drawMyModel()
    dxDrawModel3D(modelId, 0, 0, 4, 0, 0, 0)
end

local function startDraw()
    engineStreamingRequestModel(modelId, true, true)
    addEventHandler("onClientPreRender", root, drawMyModel)
end

local function stopDraw()
    engineStreamingReleaseModel(modelId, true)
    removeEventHandler("onClientPreRender", root, drawMyModel)
end

TheNormalnij avatar Aug 17 '24 09:08 TheNormalnij