filament icon indicating copy to clipboard operation
filament copied to clipboard

Import native vertex/index buffers into buffer objects

Open palver123 opened this issue 2 years ago • 2 comments

Is your feature request related to a problem? Please describe. We have been using Filament in our CAD app for PBR and reusing the (native) vertex and index buffers between our renderer and Filament deemed impossible without changing Filament. We would like to propose extending the Filament driver API with importing native buffers into Filament buffer objects just like you can import native textures. This can come in handy for other projects which mix their own rendering engine with Filament's.

Describe the solution you'd like

Driver API

// 'id' is an id<MTLBuffer> bridged as void* in Metal, GLuint in OpenGL
DECL_DRIVER_API_R_N(backend::BufferObjectHandle, importBufferObject,
        intptr_t, id,
        backend::BufferObjectBinding, bindingType,
        backend::BufferUsage, usage,
        uint32_t, byteCount)

// setupExternalImage() has to be more generic to support buffers, not just texture images
// DECL_DRIVER_API_SYNCHRONOUS_N(void, setupExternalImage, void*, image)
DECL_DRIVER_API_SYNCHRONOUS_N(void, setupExternalResource, intptr_t, externalResource)

// This was never used, at least in the git revision we forked from. May be kept if necessary
//DECL_DRIVER_API_SYNCHRONOUS_N(void, cancelExternalImage, void*, image)

Before investing the time, I'd like to hear your opinion about this feature. If you are interested, I'd deliver it in (at least) 2 PRs, 1 PR per backend. Since our solution was based on a 2 year old Filament revision, I need time to prepare each.

Describe alternatives you've considered We considered attaching/wrapping an optional 'external buffer' to/in Filament vertex and index buffers (see here). This solution worked, but we weren't satisfied because we had to modify the buffer creation code, and callers would have to specify in advance if they are going to pass in external buffers or not.

OS and backend Our proposed solution is Metal and OpenGL only. We have zero Vulkan knowledge and no engineering budget to implement it in Vulkan (even though it probably is not so hard). The biggest question to us is: is it worth anything to you if we only add such a feature to one or two backends?

Quality? Our solution has been used by 40k daily users on iOS, macOS (with MacCatalyst) and Windows (via OpenGL ES3 backend) on various hardware configurations for 2 years. We never tried it on Android and I know that's your main platform. Therefore I think the OpenGL part would need more attention from your part than the Metal part (we have deeper knowledge in Metal anyway).

palver123 avatar Oct 31 '23 12:10 palver123

Meanwhile I found that you removed cancelExternalImage too

palver123 avatar Oct 31 '23 14:10 palver123

For illustration I added:

  • a branch with the proposed API changes, but no implementation: https://github.com/google/filament/compare/main...shapr3d:filament:import_native_buffers
  • another branch with a Metal implementation and a unit test: https://github.com/shapr3d/filament/compare/import_native_buffers...shapr3d:filament:import_native_buffers_mtl

Now adding a unit test was the hardest part. I may be doing something wrong but I faced several obstacles:

  • main branch didn't compile with Xcode 14.2, so my branches didn't either.
    • -Wall ends up after -Wno-conversion. I tweaked filament/backend/CMakeLists.txt to unblock myself. Is this a known issue, or am I doing sth wrong on my local dev setup?
    • MTLGPUFamilyMac1, MTLResourceUsageSample and one of the MTLRenderCommandEncoder::useResources() overloads got deprecated and I had to comment out some code.
  • I was extending the BackendTest::VertexBufferUpdate unit test and found that it fails when ran with the Metal backend on main branch because:
    • The Program instance has no name so MetalShaderCompiler::createProgram() runs onto a nullptr. p.diagnostics("whatevs", {}); solves this, just wanted to let you know.
    • A driver purge was missing at the end of the test, so the dtor of the driver hit an assertion. Understandable since the scheduled callbacks were not drained by a purge().
    • Something is mixed up with the UBO in this test, so I completely removed it. My assumption is that both te VB and UBO are bound to slot 0 of the vertex shader, but I couldn't confirm this 😞 .

My point is that I got the new unit test working on my machine, but I had to cut some nasty corners on main, to even run the existing ones, so I will need a little help with ironing out the above problems before it can land in a PR.

palver123 avatar Nov 02 '23 17:11 palver123