Skip to content

Fix texture copy when using multiple canvas #1481

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
+30 −10
Closed

Fix texture copy when using multiple canvas #1481

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0215b7
Fix texture copy when using multiple canvas
CedricGuillemet Mar 14, 2025
96b0c81
Blit is part of the device context instead of FrameBuffer
CedricGuillemet Mar 19, 2025
dd3952e
do not wast viewId if not necessary
CedricGuillemet Mar 19, 2025
5d307c2
InvalidateView
CedricGuillemet Mar 20, 2025
aeb967f
Merge branch 'master' of https://github.com/BabylonJS/BabylonNative i…
CedricGuillemet Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Core/Graphics/InternalInclude/Babylon/Graphics/DeviceContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ namespace Babylon::Graphics
TextureInfo GetTextureInfo(bgfx::TextureHandle handle);
static bx::AllocatorI& GetDefaultAllocator() { return m_allocator; }

// call following method when a submit call is associated with the current acquired viewId
void SetViewAsUsed();
Copy link
Member

Choose a reason for hiding this comment

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

I guess we have to expose this new function because FrameBuffer has to tell DeviceContext this needs to be done when a Submit happens. Is there any other case (other than Submit) where we would need to do this, or is this really tied to FrameBuffer::Submit?

If we need to expose this as a new function, would InvalidateView be a better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Framebuffer tells device context the view contains draw call. I've added a call for Clear . InvalidateView works for me!

void Blit(bgfx::Encoder& encoder, bgfx::TextureHandle dst, uint16_t dstX, uint16_t dstY, bgfx::TextureHandle src, uint16_t srcX = 0, uint16_t srcY = 0, uint16_t width = UINT16_MAX, uint16_t height = UINT16_MAX);
private:
friend UpdateToken;

Expand All @@ -130,5 +133,8 @@ namespace Babylon::Graphics
std::mutex m_textureHandleToInfoMutex{};

static inline bx::DefaultAllocator m_allocator{};

bool m_currentViewIsUsed{false};
bgfx::ViewId m_lastAcquiredViewId{bgfx::kInvalidHandle};
};
}
1 change: 0 additions & 1 deletion Core/Graphics/InternalInclude/Babylon/Graphics/FrameBuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ namespace Babylon::Graphics
void SetScissor(bgfx::Encoder& encoder, float x, float y, float width, float height);
void Submit(bgfx::Encoder& encoder, bgfx::ProgramHandle programHandle, uint8_t flags);
void SetStencil(bgfx::Encoder& encoder, uint32_t stencilState);
void Blit(bgfx::Encoder& encoder, bgfx::TextureHandle dst, uint16_t dstX, uint16_t dstY, bgfx::TextureHandle src, uint16_t srcX = 0, uint16_t srcY = 0, uint16_t width = UINT16_MAX, uint16_t height = UINT16_MAX);

bool HasDepth() const { return m_hasDepth; }
bool HasStencil() const { return m_hasStencil; }
Expand Down
17 changes: 16 additions & 1 deletion Core/Graphics/Source/DeviceContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@ namespace Babylon::Graphics

bgfx::ViewId DeviceContext::AcquireNewViewId(bgfx::Encoder& encoder)
{
return m_graphicsImpl.AcquireNewViewId(encoder);
m_currentViewIsUsed = false;
m_lastAcquiredViewId = m_graphicsImpl.AcquireNewViewId(encoder);
return m_lastAcquiredViewId;
}

void DeviceContext::AddTexture(bgfx::TextureHandle handle, uint16_t width, uint16_t height, bool hasMips, uint16_t numLayers, bgfx::TextureFormat::Enum format)
Expand All @@ -129,4 +131,17 @@ namespace Babylon::Graphics
{
return m_graphicsImpl.GetId();
}

void DeviceContext::Blit(bgfx::Encoder& encoder, bgfx::TextureHandle dst, uint16_t dstX, uint16_t dstY, bgfx::TextureHandle src, uint16_t srcX, uint16_t srcY, uint16_t width, uint16_t height)
{
// Increment viewId so blit happens after the last drawcall of the last viewId if drawcall(s) have been submitted to the view.
// Otherwise, reuse last acquired viewId to not waste an Id.
auto viewId = m_currentViewIsUsed ? AcquireNewViewId(encoder) : m_lastAcquiredViewId;
encoder.blit(viewId, dst, dstX, dstY, src, srcX, srcY, width, height);
}

void DeviceContext::SetViewAsUsed()
{
m_currentViewIsUsed = true;
}
}
8 changes: 1 addition & 7 deletions Core/Graphics/Source/FrameBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,13 +130,7 @@ namespace Babylon::Graphics
{
SetBgfxViewPortAndScissor(encoder, m_desiredViewPort, m_desiredScissor);
encoder.submit(m_viewId.value(), programHandle, 0, flags);
}

void FrameBuffer::Blit(bgfx::Encoder& encoder, bgfx::TextureHandle dst, uint16_t dstX, uint16_t dstY, bgfx::TextureHandle src, uint16_t srcX, uint16_t srcY, uint16_t width, uint16_t height)
{
// In order for Blit to work properly we need to force the creation of a new ViewID.
SetBgfxViewPortAndScissor(encoder, m_desiredViewPort, m_desiredScissor);
encoder.blit(m_viewId.value(), dst, dstX, dstY, src, srcX, srcY, width, height);
m_deviceContext.SetViewAsUsed();
}

void FrameBuffer::SetStencil(bgfx::Encoder& encoder, uint32_t stencilState)
Expand Down
5 changes: 3 additions & 2 deletions Plugins/NativeEngine/Source/NativeEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1431,11 +1431,12 @@ namespace Babylon
{
const auto textureDestination = info[0].As<Napi::Pointer<Graphics::Texture>>().Get();
const auto textureSource = info[1].As<Napi::Pointer<Graphics::Texture>>().Get();


// Append a task instead of calling blit immediately or blit happens before drawcalls.
arcana::make_task(m_update.Scheduler(), *m_cancellationSource, [this, textureDestination, textureSource, cancellationSource = m_cancellationSource]() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try removing this? I think we might need to add this to the command queue for the ordering to be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried and copy doesn't happen in the correct order when removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try putting it in the command queue?

return arcana::make_task(m_runtimeScheduler, *m_cancellationSource, [this, textureDestination, textureSource, updateToken = m_update.GetUpdateToken(), cancellationSource = m_cancellationSource]() {
bgfx::Encoder* encoder = m_update.GetUpdateToken().GetEncoder();
GetBoundFrameBuffer(*encoder).Blit(*encoder, textureDestination->Handle(), 0, 0, textureSource->Handle());
m_deviceContext.Blit(*encoder, textureDestination->Handle(), 0, 0, textureSource->Handle());
}).then(arcana::inline_scheduler, *m_cancellationSource, [this, cancellationSource{m_cancellationSource}](const arcana::expected<void, std::exception_ptr>& result) {
if (!cancellationSource->cancelled() && result.has_error())
{
Expand Down