Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

Buffer Locked but not Unlocked by Model::Clone() #2854

Open
SirNate0 opened this issue Dec 17, 2021 · 4 comments
Open

Buffer Locked but not Unlocked by Model::Clone() #2854

SirNate0 opened this issue Dec 17, 2021 · 4 comments
Labels

Comments

@SirNate0
Copy link
Contributor

When a Model is cloned and the vertex/index buffers are not shadowed, the function attempts to lock the buffer and copy the data that way. Let's ignore for now that this violates the documentation of the Lock() functions that it is for write-only (/// Lock the buffer for write-only editing. Return data pointer if successful. Optionally discard data outside the range.). The method never Unlock()s the buffers, which could cause problems down the line, I imagine (I've not tested this).

https://github.com/urho3d/Urho3D/blob/900611ceeb05e20cedef707bcb6a2e4ac48fb4e1/Source/Urho3D/Graphics/Model.cpp#L645-L651

https://github.com/urho3d/Urho3D/blob/900611ceeb05e20cedef707bcb6a2e4ac48fb4e1/Source/Urho3D/Graphics/Model.cpp#L673-L679

@eugeneko
Copy link
Contributor

There's no reason to use Lock instead of GetShadowData, Lock won't be able to read GPU buffer anyway.

@SirNate0
Copy link
Contributor Author

Is that a "will never work" or a "generally won't work but with certain drivers may work"? If the former, we should probably just remove the else branches and just log the error.

@eugeneko
Copy link
Contributor

eugeneko commented Dec 19, 2021

In OpenGL Urho3D there is literally no GPU memory access, upload call is performed Unlock.
In DX11, there is write-only GPU access on Lock.
In DX9 there may or may not be read GPU access on Lock but it doesn't matter because screw DX9.

In general, Lock can reliably read buffer data only if buffer is shadowed, because it just returns the shadow in this case. So, there's no reason to ever use Lock for reading, because you have GetShadowData for returning shadow.

And even if Lock can actually read GPU data on DX9, it is completely irrelevant.

"generally won't work but with certain drivers may work"

It may be the opposite, btw. "Generally won't work, will crash your Driver/OS with certain drivers"

@github-actions
Copy link

Marking this stale since there has been no activity for 30 days.
It will be closed if there is no activity for another 15 days.

@github-actions github-actions bot added the stale label Jan 19, 2022
@1vanK 1vanK added backlog and removed stale labels Jan 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants