Skip to content

Commit

Permalink
Minor Driver API simplification.
Browse files Browse the repository at this point in the history
As far as I can tell, there is no need for the `enabledAttributes`
argument in `setRenderPrimitiveBuffer`. This helps pave the way for
the upcoming BufferObject API.
  • Loading branch information
prideout committed Mar 24, 2021
1 parent c4a12fe commit 3d2734b
Show file tree
Hide file tree
Showing 13 changed files with 36 additions and 50 deletions.
3 changes: 2 additions & 1 deletion filament/backend/include/backend/DriverEnums.h
Original file line number Diff line number Diff line change
Expand Up @@ -724,9 +724,10 @@ struct Attribute {
static constexpr uint8_t FLAG_NORMALIZED = 0x1;
//! attribute is an integer
static constexpr uint8_t FLAG_INTEGER_TARGET = 0x2;
static constexpr uint8_t BUFFER_UNUSED = 0xFF;
uint32_t offset = 0; //!< attribute offset in bytes
uint8_t stride = 0; //!< attribute stride in bytes
uint8_t buffer = 0xFF; //!< attribute buffer index
uint8_t buffer = BUFFER_UNUSED; //!< attribute buffer index
ElementType type = ElementType::BYTE; //!< attribute element type
uint8_t flags = 0x0; //!< attribute flags
};
Expand Down
3 changes: 1 addition & 2 deletions filament/backend/include/private/backend/DriverAPI.inc
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,7 @@ DECL_DRIVER_API_0(nextSubpass)
DECL_DRIVER_API_N(setRenderPrimitiveBuffer,
backend::RenderPrimitiveHandle, rph,
backend::VertexBufferHandle, vbh,
backend::IndexBufferHandle, ibh,
uint32_t, enabledAttributes)
backend::IndexBufferHandle, ibh)

DECL_DRIVER_API_N(setRenderPrimitiveRange,
backend::RenderPrimitiveHandle, rph,
Expand Down
4 changes: 2 additions & 2 deletions filament/backend/src/metal/MetalDriver.mm
Original file line number Diff line number Diff line change
Expand Up @@ -802,11 +802,11 @@
}

void MetalDriver::setRenderPrimitiveBuffer(Handle<HwRenderPrimitive> rph,
Handle<HwVertexBuffer> vbh, Handle<HwIndexBuffer> ibh, uint32_t enabledAttributes) {
Handle<HwVertexBuffer> vbh, Handle<HwIndexBuffer> ibh) {
auto primitive = handle_cast<MetalRenderPrimitive>(mHandleMap, rph);
auto vertexBuffer = handle_cast<MetalVertexBuffer>(mHandleMap, vbh);
auto indexBuffer = handle_cast<MetalIndexBuffer>(mHandleMap, ibh);
primitive->setBuffers(vertexBuffer, indexBuffer, enabledAttributes);
primitive->setBuffers(vertexBuffer, indexBuffer);
}

void MetalDriver::setRenderPrimitiveRange(Handle<HwRenderPrimitive> rph,
Expand Down
3 changes: 1 addition & 2 deletions filament/backend/src/metal/MetalHandles.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,7 @@ struct MetalUniformBuffer : public HwUniformBuffer {
};

struct MetalRenderPrimitive : public HwRenderPrimitive {
void setBuffers(MetalVertexBuffer* vertexBuffer, MetalIndexBuffer* indexBuffer,
uint32_t enabledAttributes);
void setBuffers(MetalVertexBuffer* vertexBuffer, MetalIndexBuffer* indexBuffer);
// The pointers to MetalVertexBuffer and MetalIndexBuffer are "weak".
// The MetalVertexBuffer and MetalIndexBuffer must outlive the MetalRenderPrimitive.

Expand Down
8 changes: 4 additions & 4 deletions filament/backend/src/metal/MetalHandles.mm
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ BufferDescriptor b(nullptr, 0u, [](void* buffer, size_t size, void* user) {
buffer(context, size) { }

void MetalRenderPrimitive::setBuffers(MetalVertexBuffer* vertexBuffer, MetalIndexBuffer*
indexBuffer, uint32_t enabledAttributes) {
indexBuffer) {
this->vertexBuffer = vertexBuffer;
this->indexBuffer = indexBuffer;

Expand All @@ -319,8 +319,9 @@ BufferDescriptor b(nullptr, 0u, [](void* buffer, size_t size, void* user) {

uint32_t bufferIndex = 0;
for (uint32_t attributeIndex = 0; attributeIndex < attributeCount; attributeIndex++) {
if (!(enabledAttributes & (1U << attributeIndex))) {
const uint8_t flags = vertexBuffer->attributes[attributeIndex].flags;
const auto& attribute = vertexBuffer->attributes[attributeIndex];
if (attribute.buffer != Attribute::BUFFER_UNUSED) {
const uint8_t flags = attribute.flags;
const MTLVertexFormat format = (flags & Attribute::FLAG_INTEGER_TARGET) ?
MTLVertexFormatUInt4 : MTLVertexFormatFloat4;

Expand All @@ -337,7 +338,6 @@ BufferDescriptor b(nullptr, 0u, [](void* buffer, size_t size, void* user) {
};
continue;
}
const auto& attribute = vertexBuffer->attributes[attributeIndex];

buffers.push_back(vertexBuffer->buffers[attribute.buffer]);
offsets.push_back(attribute.offset);
Expand Down
3 changes: 1 addition & 2 deletions filament/backend/src/noop/NoopDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,7 @@ void NoopDriver::nextSubpass(int) {
}

void NoopDriver::setRenderPrimitiveBuffer(Handle<HwRenderPrimitive> rph,
Handle<HwVertexBuffer> vbh, Handle<HwIndexBuffer> ibh,
uint32_t enabledAttributes) {
Handle<HwVertexBuffer> vbh, Handle<HwIndexBuffer> ibh) {
}

void NoopDriver::setRenderPrimitiveRange(Handle<HwRenderPrimitive> rph,
Expand Down
31 changes: 15 additions & 16 deletions filament/backend/src/opengl/OpenGLDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2358,8 +2358,7 @@ GLsizei OpenGLDriver::getAttachments(std::array<GLenum, 6>& attachments,
}

void OpenGLDriver::setRenderPrimitiveBuffer(Handle<HwRenderPrimitive> rph,
Handle<HwVertexBuffer> vbh, Handle<HwIndexBuffer> ibh,
uint32_t enabledAttributes) {
Handle<HwVertexBuffer> vbh, Handle<HwIndexBuffer> ibh) {
DEBUG_MARKER()
auto& gl = mContext;

Expand All @@ -2376,11 +2375,11 @@ void OpenGLDriver::setRenderPrimitiveBuffer(Handle<HwRenderPrimitive> rph,
rp->gl.indicesType = ib->elementSize == 4 ? GL_UNSIGNED_INT : GL_UNSIGNED_SHORT;
rp->maxVertexCount = eb->vertexCount;
for (size_t i = 0, n = eb->attributes.size(); i < n; i++) {
if (enabledAttributes & (1U << i)) {
uint8_t bi = eb->attributes[i].buffer;
assert_invariant(bi != 0xFF);
const auto& attribute = eb->attributes[i];
if (attribute.buffer != Attribute::BUFFER_UNUSED) {
uint8_t bi = attribute.buffer;
gl.bindBuffer(GL_ARRAY_BUFFER, eb->gl.buffers[bi]);
if (UTILS_UNLIKELY(eb->attributes[i].flags & Attribute::FLAG_INTEGER_TARGET)) {
if (UTILS_UNLIKELY(attribute.flags & Attribute::FLAG_INTEGER_TARGET)) {

// Emscripten regressed at the following PR so we work around it for now.
// https://github.com/emscripten-core/emscripten/pull/11225
Expand All @@ -2389,25 +2388,25 @@ void OpenGLDriver::setRenderPrimitiveBuffer(Handle<HwRenderPrimitive> rph,
#endif

glVertexAttribIPointer(GLuint(i),
getComponentCount(eb->attributes[i].type),
getComponentType(eb->attributes[i].type),
eb->attributes[i].stride,
(void*) uintptr_t(eb->attributes[i].offset));
getComponentCount(attribute.type),
getComponentType(attribute.type),
attribute.stride,
(void*) uintptr_t(attribute.offset));
} else {
glVertexAttribPointer(GLuint(i),
getComponentCount(eb->attributes[i].type),
getComponentType(eb->attributes[i].type),
getNormalization(eb->attributes[i].flags & Attribute::FLAG_NORMALIZED),
eb->attributes[i].stride,
(void*) uintptr_t(eb->attributes[i].offset));
getComponentCount(attribute.type),
getComponentType(attribute.type),
getNormalization(attribute.flags & Attribute::FLAG_NORMALIZED),
attribute.stride,
(void*) uintptr_t(attribute.offset));
}

gl.enableVertexAttribArray(GLuint(i));
} else {

// In some OpenGL implementations, we must supply a properly-typed placeholder for
// every integer input that is declared in the vertex shader.
if (UTILS_UNLIKELY(eb->attributes[i].flags & Attribute::FLAG_INTEGER_TARGET)) {
if (UTILS_UNLIKELY(attribute.flags & Attribute::FLAG_INTEGER_TARGET)) {
glVertexAttribI4ui(GLuint(i), 0, 0, 0, 0);
} else {
glVertexAttrib4f(GLuint(i), 0, 0, 0, 0);
Expand Down
5 changes: 2 additions & 3 deletions filament/backend/src/vulkan/VulkanDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1164,11 +1164,10 @@ void VulkanDriver::nextSubpass(int) {
}

void VulkanDriver::setRenderPrimitiveBuffer(Handle<HwRenderPrimitive> rph,
Handle<HwVertexBuffer> vbh, Handle<HwIndexBuffer> ibh,
uint32_t enabledAttributes) {
Handle<HwVertexBuffer> vbh, Handle<HwIndexBuffer> ibh) {
auto primitive = handle_cast<VulkanRenderPrimitive>(mHandleMap, rph);
primitive->setBuffers(handle_cast<VulkanVertexBuffer>(mHandleMap, vbh),
handle_cast<VulkanIndexBuffer>(mHandleMap, ibh), enabledAttributes);
handle_cast<VulkanIndexBuffer>(mHandleMap, ibh));
}

void VulkanDriver::setRenderPrimitiveRange(Handle<HwRenderPrimitive> rph,
Expand Down
13 changes: 3 additions & 10 deletions filament/backend/src/vulkan/VulkanHandles.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ void VulkanRenderPrimitive::setPrimitiveType(backend::PrimitiveType pt) {
}

void VulkanRenderPrimitive::setBuffers(VulkanVertexBuffer* vertexBuffer,
VulkanIndexBuffer* indexBuffer, uint32_t enabledAttributes) {
VulkanIndexBuffer* indexBuffer) {
this->vertexBuffer = vertexBuffer;
this->indexBuffer = indexBuffer;
const size_t nattrs = vertexBuffer->attributes.size();
Expand All @@ -882,9 +882,6 @@ void VulkanRenderPrimitive::setBuffers(VulkanVertexBuffer* vertexBuffer,
memset(varray.attributes, 0, sizeof(varray.attributes));
memset(varray.buffers, 0, sizeof(varray.buffers));

// Position should always be present.
assert_invariant(enabledAttributes & 1);

// For each enabled attribute, append to each of the above lists. Note that a single VkBuffer
// handle might be appended more than once, which is perfectly fine.
uint32_t bufferIndex = 0;
Expand All @@ -894,7 +891,7 @@ void VulkanRenderPrimitive::setBuffers(VulkanVertexBuffer* vertexBuffer,

// HACK: Re-use the positions buffer as a dummy buffer for disabled attributes.
// We know that position exists (see earlier assert)
if (!(enabledAttributes & (1U << attribIndex))) {
if (attrib.buffer == Attribute::BUFFER_UNUSED) {

// HACK: Presently, Filament's vertex shaders declare all attributes as either vec4 or
// uvec4 (the latter is for bone indices), and positions are always at least 32 bits per
Expand All @@ -903,11 +900,7 @@ void VulkanRenderPrimitive::setBuffers(VulkanVertexBuffer* vertexBuffer,
const bool isInteger = attrib.flags & Attribute::FLAG_INTEGER_TARGET;
vkformat = isInteger ? VK_FORMAT_R8G8B8A8_UINT : VK_FORMAT_R8G8B8A8_SNORM;

if (UTILS_LIKELY(enabledAttributes & 1)) {
attrib = vertexBuffer->attributes[0];
} else {
continue;
}
attrib = vertexBuffer->attributes[0];
}

buffers.push_back(vertexBuffer->buffers[attrib.buffer]->getGpuBuffer());
Expand Down
3 changes: 1 addition & 2 deletions filament/backend/src/vulkan/VulkanHandles.h
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,7 @@ struct VulkanTexture : public HwTexture {
struct VulkanRenderPrimitive : public HwRenderPrimitive {
explicit VulkanRenderPrimitive(VulkanContext& context) {}
void setPrimitiveType(backend::PrimitiveType pt);
void setBuffers(VulkanVertexBuffer* vertexBuffer, VulkanIndexBuffer* indexBuffer,
uint32_t enabledAttributes);
void setBuffers(VulkanVertexBuffer* vertexBuffer, VulkanIndexBuffer* indexBuffer);
VulkanVertexBuffer* vertexBuffer = nullptr;
VulkanIndexBuffer* indexBuffer = nullptr;
VkPrimitiveTopology primitiveTopology;
Expand Down
3 changes: 1 addition & 2 deletions filament/backend/test/TrianglePrimitive.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ TrianglePrimitive::TrianglePrimitive(filament::backend::DriverApi& driverApi,

mRenderPrimitive = mDriverApi.createRenderPrimitive(0);

mDriverApi.setRenderPrimitiveBuffer(mRenderPrimitive, mVertexBuffer, mIndexBuffer,
enabledAttributes.getValue());
mDriverApi.setRenderPrimitiveBuffer(mRenderPrimitive, mVertexBuffer, mIndexBuffer);
mDriverApi.setRenderPrimitiveRange(mRenderPrimitive, PrimitiveType::TRIANGLES, 0, 0, 2, 3);
}

Expand Down
3 changes: 1 addition & 2 deletions filament/src/Engine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,7 @@ void FEngine::init() {

mFullScreenTriangleRph = driverApi.createRenderPrimitive();
driverApi.setRenderPrimitiveBuffer(mFullScreenTriangleRph,
mFullScreenTriangleVb->getHwHandle(), mFullScreenTriangleIb->getHwHandle(),
mFullScreenTriangleVb->getDeclaredAttributes().getValue());
mFullScreenTriangleVb->getHwHandle(), mFullScreenTriangleIb->getHwHandle());
driverApi.setRenderPrimitiveRange(mFullScreenTriangleRph, PrimitiveType::TRIANGLES,
0, 0, 2, (uint32_t)mFullScreenTriangleIb->getIndexCount());

Expand Down
4 changes: 2 additions & 2 deletions filament/src/RenderPrimitive.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ void FRenderPrimitive::init(backend::DriverApi& driver,
auto const& ebh = vertexBuffer->getHwHandle();
auto const& ibh = indexBuffer->getHwHandle();

driver.setRenderPrimitiveBuffer(mHandle, ebh, ibh, (uint32_t)enabledAttributes.getValue());
driver.setRenderPrimitiveBuffer(mHandle, ebh, ibh);
driver.setRenderPrimitiveRange(mHandle, entry.type,
(uint32_t)entry.offset, (uint32_t)entry.minIndex, (uint32_t)entry.maxIndex,
(uint32_t)entry.count);
Expand All @@ -67,7 +67,7 @@ void FRenderPrimitive::set(FEngine& engine, RenderableManager::PrimitiveType typ

FEngine::DriverApi& driver = engine.getDriverApi();

driver.setRenderPrimitiveBuffer(mHandle, ebh, ibh, (uint32_t)enabledAttributes.getValue());
driver.setRenderPrimitiveBuffer(mHandle, ebh, ibh);
driver.setRenderPrimitiveRange(mHandle, type,
(uint32_t)offset, (uint32_t)minIndex, (uint32_t)maxIndex, (uint32_t)count);

Expand Down

0 comments on commit 3d2734b

Please sign in to comment.