Fix TextVideoMemBlitter staging buffer issue. #3419
Conversation
src/renderer_vk.cpp
Outdated
| { | ||
| BGFX_PROFILER_SCOPE("scratchBuffer::flush", kColorResource); | ||
| scratchBuffer.flush(); | ||
| scratchBuffer.reset(); |
There was a problem hiding this comment.
Is there reason to not have flush inside reset?
There was a problem hiding this comment.
They are two separate things IMO. Currently they are used one after the other, indeed.
There was a problem hiding this comment.
Note that in the master branch, flush() was being called at the right time, but after the reset(). This effectively made flush() a no-op, because we only try to flush the memory region that we touched. However, I guess we were lucky, and it did always update?
There was a problem hiding this comment.
They are two separate things IMO. Currently they are used one after the other, indeed.
I understand this, but I see case where flush is called independently from reset but it looks like calling reset without flush first would be a bug always? Is there case where calling reset without flush would be desirable?
There was a problem hiding this comment.
Merged them into one function.
src/renderer_vk.cpp
Outdated
|
|
||
|
|
||
| void ScratchBufferVK::flush() | ||
| void ScratchBufferVK::flush_and_reset() |
There was a problem hiding this comment.
Following bgfx code style would be flushAndReset, but it's better to implement as reset(bool _flush = true)
There was a problem hiding this comment.
Went with flush(_reset = true), because that makes a lot more sense to me.
2 participants
Fixes #3418 properly, and replaces the patch in 72f9b8b.
CC @synaptor.