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

dxDrawModel3D reimplementation #3266

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tederis
Copy link
Collaborator

@tederis tederis commented Dec 9, 2023

Brings dxDrawModel3D function(#3172) back. The syntax is compatible with the previous one, but has a new optional argument that allows to control how a model should be streamed in: 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, string mode = "async"]), where mode can be "loaded", "async", "blocking". When mode is "loaded" then a model will be only visible when it's already loaded at the function's call moment.

@tederis
Copy link
Collaborator Author

tederis commented Dec 9, 2023

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

@TheNormalnij
Copy link
Contributor

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 )

@tederis
Copy link
Collaborator Author

tederis commented Dec 9, 2023

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.

@PlatinMTA
Copy link
Contributor

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

@tederis
Copy link
Collaborator Author

tederis commented Dec 9, 2023

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.

@TheNormalnij
Copy link
Contributor

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.

@tederis
Copy link
Collaborator Author

tederis commented Dec 9, 2023

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.

@Pirulax
Copy link
Contributor

Pirulax commented Dec 12, 2023

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?

@StrixG StrixG added the enhancement New feature or request label Dec 21, 2023
@Pirulax
Copy link
Contributor

Pirulax commented Dec 27, 2023

Hey, any updates on this?

@tederis
Copy link
Collaborator Author

tederis commented Dec 27, 2023

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.

@Pirulax
Copy link
Contributor

Pirulax commented Dec 29, 2023

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

#define HOOKSIZE_CRenderer_EverythingBarRoads 5
DWORD RETURN_CRenderer_EverythingBarRoads = 0x553C7D;
DWORD DO_CRenderer_EverythingBarRoads = 0x7EE180;
void _declspec(naked) HOOK_CRenderer_EverythingBarRoads()
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not work that well for objects with alpha.
Have you tried that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, this won't work well for translucent objects. Obviously, translucent objects require a dedicated render path involving the batch sorting. I'll fix it, okay.

Copy link
Contributor

@Pirulax Pirulax Jan 1, 2024

Choose a reason for hiding this comment

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

I think for now it's fine for it to not work, it can be fixed later.
I just wanted to note this

@tederis
Copy link
Collaborator Author

tederis commented Dec 31, 2023

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.

@TheNormalnij
Copy link
Contributor

TheNormalnij commented Dec 31, 2023 via email

@tederis
Copy link
Collaborator Author

tederis commented Dec 31, 2023

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).

@TheNormalnij
Copy link
Contributor

TheNormalnij commented Dec 31, 2023 via email

@tederis
Copy link
Collaborator Author

tederis commented Dec 31, 2023

"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
Copy link
Collaborator Author

tederis commented Dec 31, 2023

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
Copy link
Collaborator Author

tederis commented Dec 31, 2023

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

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.

None yet

5 participants