From 0ce6c8d8d04776b2c2f26199820a768dea973438 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 19 Mar 2019 18:49:54 -0700 Subject: [PATCH] rework scissor/viewport API a bit in the backend - the IGNORE_VIEWPORT flag is gone. It wasn't actually needed and what it was doing was ambiguous. - removed the backend viewport() API which wasn't actually needed. - beginRenderPass() now ALWAYS sets the viewport and scissor but doesn't necessarily clears honoring the scissor (based on the IGNORE_SCISSOR flag). - the GLES backend now just sets the scissor when it needs it, as opposed to always trying to keep it set. we now rely on state tracking to de-dup the gl calls. --- filament/src/ShadowMap.cpp | 11 ++--- filament/src/details/MaterialInstance.h | 24 +++++----- filament/src/driver/DriverAPI.inc | 6 --- filament/src/driver/metal/MetalDriver.mm | 34 ++++++-------- filament/src/driver/opengl/OpenGLDriver.cpp | 46 ++++++------------- filament/src/driver/vulkan/VulkanDriver.cpp | 45 ++++++++---------- .../include/filament/driver/DriverEnums.h | 1 - 7 files changed, 64 insertions(+), 103 deletions(-) diff --git a/filament/src/ShadowMap.cpp b/filament/src/ShadowMap.cpp index 82c6407c05..c85bcc2d32 100644 --- a/filament/src/ShadowMap.cpp +++ b/filament/src/ShadowMap.cpp @@ -120,14 +120,11 @@ void ShadowMap::beginRenderPass(DriverApi& driver) const noexcept { params.flags.discardStart = TargetBufferFlags::DEPTH; params.flags.discardEnd = TargetBufferFlags::COLOR_AND_STENCIL; params.clearDepth = 1.0; - params.viewport.width = params.viewport.height = mShadowMapDimension; - // Disable scissor and viewport to avoid bugs in some drivers where the GPU memory is reloaded - // needlessly. - params.flags.clear |= RenderPassFlags::IGNORE_SCISSOR | RenderPassFlags::IGNORE_VIEWPORT; + params.viewport = mViewport; + // disable scissor for clearing so the whole surface is cleared, but set the viewport to the + // the inset-by-1 rectangle. + params.flags.clear |= RenderPassFlags::IGNORE_SCISSOR; driver.beginRenderPass(mShadowMapRenderTarget, params); - - driver.viewport(mViewport.left, mViewport.bottom, - mViewport.width, mViewport.height); } void ShadowMap::update( diff --git a/filament/src/details/MaterialInstance.h b/filament/src/details/MaterialInstance.h index edcd928111..c0dd40a156 100644 --- a/filament/src/details/MaterialInstance.h +++ b/filament/src/details/MaterialInstance.h @@ -55,9 +55,8 @@ public: if (mSbHandle) { driver.bindSamplers(BindingPoints::PER_MATERIAL_INSTANCE, mSbHandle); } - driver.setViewportScissor( - mScissorRect[0], mScissorRect[1], - uint32_t(mScissorRect[2]), uint32_t(mScissorRect[3])); + driver.setViewportScissor(mScissorRect.left, mScissorRect.bottom, + mScissorRect.width, mScissorRect.height); } template @@ -77,15 +76,17 @@ public: SamplerGroup const& getSamplerGroup() const noexcept { return mSamplers; } void setScissor(int32_t left, int32_t bottom, uint32_t width, uint32_t height) noexcept { - mScissorRect[0] = left; - mScissorRect[1] = bottom; - mScissorRect[2] = (int32_t)std::min(width, (uint32_t)std::numeric_limits::max()); - mScissorRect[3] = (int32_t)std::min(height, (uint32_t)std::numeric_limits::max()); + mScissorRect = { left, bottom, + std::min(width, (uint32_t)std::numeric_limits::max()), + std::min(height, (uint32_t)std::numeric_limits::max()) + }; } void unsetScissor() noexcept { - mScissorRect[0] = mScissorRect[1] = 0; - mScissorRect[2] = mScissorRect[3] = std::numeric_limits::max(); + mScissorRect = { 0, 0, + (uint32_t)std::numeric_limits::max(), + (uint32_t)std::numeric_limits::max() + }; } void setPolygonOffset(float scale, float constant) noexcept { @@ -115,8 +116,9 @@ private: uint64_t mMaterialSortingKey = 0; // Scissor rectangle is specified as: Left Bottom Width Height. - int32_t mScissorRect[4] = { - 0, 0, std::numeric_limits::max(), std::numeric_limits::max() + driver::Viewport mScissorRect = { 0, 0, + (uint32_t)std::numeric_limits::max(), + (uint32_t)std::numeric_limits::max() }; }; diff --git a/filament/src/driver/DriverAPI.inc b/filament/src/driver/DriverAPI.inc index a439df0a32..354944ab95 100644 --- a/filament/src/driver/DriverAPI.inc +++ b/filament/src/driver/DriverAPI.inc @@ -425,12 +425,6 @@ DECL_DRIVER_API_1(commit, * ----------------------- */ -DECL_DRIVER_API_4(viewport, - ssize_t, left, - ssize_t, bottom, - size_t, width, - size_t, height) - DECL_DRIVER_API_2(bindUniformBuffer, size_t, index, Driver::UniformBufferHandle, ubh) diff --git a/filament/src/driver/metal/MetalDriver.mm b/filament/src/driver/metal/MetalDriver.mm index c0252cc187..a87f560c35 100644 --- a/filament/src/driver/metal/MetalDriver.mm +++ b/filament/src/driver/metal/MetalDriver.mm @@ -443,8 +443,20 @@ void MetalDriver::beginRenderPass(Driver::RenderTargetHandle rth, // Filament's default winding is counter clockwise. [mContext->currentCommandEncoder setFrontFacingWinding:MTLWindingCounterClockwise]; - viewport(params.viewport.left, params.viewport.bottom, params.viewport.width, - params.viewport.height); + // Flip the viewport, because Metal's screen space is vertically flipped that of Filament's. + NSInteger renderTargetHeight = + mContext->currentRenderTarget->isDefaultRenderTarget() ? + mContext->currentSurface->surfaceHeight : mContext->currentRenderTarget->height; + MTLViewport metalViewport { + .originX = static_cast(params.viewport.left), + .originY = renderTargetHeight - static_cast(params.viewport.bottom) - + static_cast(params.viewport.height), + .width = static_cast(params.viewport.width), + .height = static_cast(params.viewport.height), + .znear = 0.0, + .zfar = 1.0 + }; + [mContext->currentCommandEncoder setViewport:metalViewport]; // Metal requires a new command encoder for each render pass, and they cannot be reused. // We must bind certain states for each command encoder, so we dirty the states here to force a @@ -514,24 +526,6 @@ void MetalDriver::commit(Driver::SwapChainHandle sch) { mContext->currentDrawable = nil; } -void MetalDriver::viewport(ssize_t left, ssize_t bottom, size_t width, size_t height) { - ASSERT_PRECONDITION(mContext->currentCommandEncoder != nullptr, "currentCommandEncoder is null"); - // Flip the viewport, because Metal's screen space is vertically flipped that of Filament's. - NSInteger renderTargetHeight = - mContext->currentRenderTarget->isDefaultRenderTarget() ? - mContext->currentSurface->surfaceHeight : mContext->currentRenderTarget->height; - MTLViewport metalViewport { - .originX = static_cast(left), - .originY = renderTargetHeight - static_cast(bottom) - - static_cast(height), - .height = static_cast(height), - .width = static_cast(width), - .znear = 0.0, - .zfar = 1.0 - }; - [mContext->currentCommandEncoder setViewport:metalViewport]; -} - void MetalDriver::bindUniformBuffer(size_t index, Driver::UniformBufferHandle ubh) { mContext->uniformState[index].updateState(UniformBufferState { .bound = true, diff --git a/filament/src/driver/opengl/OpenGLDriver.cpp b/filament/src/driver/opengl/OpenGLDriver.cpp index 1a9944895b..ea7b127c52 100644 --- a/filament/src/driver/opengl/OpenGLDriver.cpp +++ b/filament/src/driver/opengl/OpenGLDriver.cpp @@ -1273,7 +1273,6 @@ void OpenGLDriver::createRenderTargetR(Driver::RenderTargetHandle rth, bindFramebuffer(GL_FRAMEBUFFER, rt->gl.fbo); disable(GL_SCISSOR_TEST); glClear(GL_COLOR_BUFFER_BIT); - enable(GL_SCISSOR_TEST); #endif } else { rt->gl.colorLevel = 0; @@ -2160,15 +2159,21 @@ void OpenGLDriver::beginRenderPass(Driver::RenderTargetHandle rth, } } - if (!(clearFlags & RenderPassFlags::IGNORE_VIEWPORT)) { - viewport(params.viewport.left, params.viewport.bottom, - params.viewport.width, params.viewport.height); - } + setViewport(params.viewport.left, params.viewport.bottom, + params.viewport.width, params.viewport.height); + + setScissor(params.viewport.left, params.viewport.bottom, + params.viewport.width, params.viewport.height); const bool respectScissor = !(clearFlags & RenderPassFlags::IGNORE_SCISSOR); const bool clearColor = clearFlags & TargetBufferFlags::COLOR; const bool clearDepth = clearFlags & TargetBufferFlags::DEPTH; const bool clearStencil = clearFlags & TargetBufferFlags::STENCIL; + if (respectScissor) { + enable(GL_SCISSOR_TEST); + } else { + disable(GL_SCISSOR_TEST); + } if (respectScissor && GLES31_HEADERS && bugs.clears_hurt_performance) { // With OpenGL ES, we clear the viewport using geometry to improve performance on certain // OpenGL drivers. e.g. on Adreno this avoids needless loads from the GMEM. @@ -2176,16 +2181,10 @@ void OpenGLDriver::beginRenderPass(Driver::RenderTargetHandle rth, clearDepth, params.clearDepth, clearStencil, params.clearStencil); } else { - if (!respectScissor) { - disable(GL_SCISSOR_TEST); - } // With OpenGL we always clear using glClear() clearWithRasterPipe(clearColor, params.clearColor, clearDepth, params.clearDepth, clearStencil, params.clearStencil); - if (!respectScissor) { - enable(GL_SCISSOR_TEST); - } } } @@ -2234,7 +2233,6 @@ void OpenGLDriver::resolve(GLRenderTarget const* rt, TargetBufferFlags discardFl bindFramebuffer(GL_DRAW_FRAMEBUFFER, rt->gl.fbo_draw); disable(GL_SCISSOR_TEST); glBlitFramebuffer(0, 0, rt->width, rt->height, 0, 0, rt->width, rt->height, mask, GL_NEAREST); - enable(GL_SCISSOR_TEST); CHECK_GL_ERROR(utils::slog.e) } } @@ -2392,6 +2390,7 @@ void OpenGLDriver::setRenderPrimitiveRange(Driver::RenderPrimitiveHandle rph, } } +// Sets up a scissor rectangle that automatically gets clipped against the viewport. void OpenGLDriver::setViewportScissor( int32_t left, int32_t bottom, uint32_t width, uint32_t height) { DEBUG_MARKER() @@ -2416,9 +2415,7 @@ void OpenGLDriver::setViewportScissor( // rather than negative values to satisfy OpenGL requirements. scissor.z = std::max(0, right - scissor.x); scissor.w = std::max(0, top - scissor.y); - update_state(state.window.scissor, scissor, [scissor]() { - glScissor(scissor.x, scissor.y, scissor.z, scissor.w); - }); + setScissor(scissor.x, scissor.y, scissor.z, scissor.w); } /* @@ -2594,22 +2591,6 @@ void OpenGLDriver::readStreamPixels(Driver::StreamHandle sh, // Setting rendering state // ------------------------------------------------------------------------------------------------ -void OpenGLDriver::viewport(ssize_t left, ssize_t bottom, size_t width, size_t height) { - DEBUG_MARKER() - - GLint l = (GLint)left; - GLint b = (GLint)bottom; - GLsizei w = (GLsizei)width; - GLsizei h = (GLsizei)height; - - // Note that we also set the scissor when binding a material instance, but the following - // scissor call is still necessary, so that the post-process quad doesn't get clipped. - setScissor(l, b, w, h); - - setViewport(l, b, w, h); - CHECK_GL_ERROR(utils::slog.e) -} - void OpenGLDriver::bindUniformBuffer(size_t index, Driver::UniformBufferHandle ubh) { DEBUG_MARKER() GLUniformBuffer* ub = handle_cast(ubh); @@ -2908,7 +2889,6 @@ void OpenGLDriver::blit(TargetBufferFlags buffers, srcRect.left, srcRect.bottom, srcRect.left + srcRect.width, srcRect.bottom + srcRect.height, dstRect.left, dstRect.bottom, dstRect.left + dstRect.width, dstRect.bottom + dstRect.height, mask, glFilterMode); - enable(GL_SCISSOR_TEST); CHECK_GL_ERROR(utils::slog.e) // In a sense, blitting to a texture level is similar to calling setTextureData on it; in @@ -2951,6 +2931,8 @@ void OpenGLDriver::draw( polygonOffset(state.polygonOffset.slope, state.polygonOffset.constant); + enable(GL_SCISSOR_TEST); + glDrawRangeElements(GLenum(rp->type), rp->minIndex, rp->maxIndex, rp->count, rp->gl.indicesType, reinterpret_cast(rp->offset)); diff --git a/filament/src/driver/vulkan/VulkanDriver.cpp b/filament/src/driver/vulkan/VulkanDriver.cpp index 77764865d2..5cc23d863c 100644 --- a/filament/src/driver/vulkan/VulkanDriver.cpp +++ b/filament/src/driver/vulkan/VulkanDriver.cpp @@ -650,10 +650,25 @@ void VulkanDriver::beginRenderPass(Driver::RenderTargetHandle rth, renderPassInfo.pClearValues = &clearValues[0]; vkCmdBeginRenderPass(swapContext.cmdbuffer, &renderPassInfo, VK_SUBPASS_CONTENTS_INLINE); - if (!(params.flags.clear & RenderPassFlags::IGNORE_VIEWPORT)) { - viewport(params.viewport.left, params.viewport.bottom, - params.viewport.width, params.viewport.height); - } + + VkViewport viewport = mContext.viewport = { + .x = (float) params.viewport.left, + .y = (float) params.viewport.bottom, + .width = (float) params.viewport.width, + .height = (float) params.viewport.height, + .minDepth = 0.0f, + .maxDepth = 1.0f + }; + VkRect2D scissor { + .extent = { (uint32_t) viewport.width, (uint32_t) viewport.height }, + .offset = { std::max(0, (int32_t) viewport.x), std::max(0, (int32_t) viewport.y) } + }; + + mCurrentRenderTarget->transformClientRectToPlatform(&scissor); + vkCmdSetScissor(mContext.cmdbuffer, 0, 1, &scissor); + + mCurrentRenderTarget->transformClientRectToPlatform(&viewport); + vkCmdSetViewport(mContext.cmdbuffer, 0, 1, &viewport); mContext.currentRenderPass = renderPassInfo; } @@ -765,28 +780,6 @@ void VulkanDriver::commit(Driver::SwapChainHandle sch) { ASSERT_POSTCONDITION(result == VK_SUCCESS, "vkQueuePresentKHR error."); } -void VulkanDriver::viewport(ssize_t left, ssize_t bottom, size_t width, size_t height) { - assert(mContext.cmdbuffer && mCurrentRenderTarget); - VkViewport viewport = mContext.viewport = { - .x = (float) left, - .y = (float) bottom, - .height = (float) height, - .width = (float) width, - .minDepth = 0.0f, - .maxDepth = 1.0f - }; - VkRect2D scissor { - .extent = { (uint32_t) width, (uint32_t) height }, - .offset = { std::max(0, (int32_t) left), std::max(0, (int32_t) bottom) } - }; - - mCurrentRenderTarget->transformClientRectToPlatform(&scissor); - vkCmdSetScissor(mContext.cmdbuffer, 0, 1, &scissor); - - mCurrentRenderTarget->transformClientRectToPlatform(&viewport); - vkCmdSetViewport(mContext.cmdbuffer, 0, 1, &viewport); -} - void VulkanDriver::bindUniformBuffer(size_t index, Driver::UniformBufferHandle ubh) { auto* buffer = handle_cast(mHandleMap, ubh); // The driver API does not currently expose offset / range, but it will do so in the future. diff --git a/libs/filabridge/include/filament/driver/DriverEnums.h b/libs/filabridge/include/filament/driver/DriverEnums.h index ef16dd08eb..e72652acfb 100644 --- a/libs/filabridge/include/filament/driver/DriverEnums.h +++ b/libs/filabridge/include/filament/driver/DriverEnums.h @@ -95,7 +95,6 @@ struct RenderPassFlags { // Extra RenderPass-only flags stashed in the "clear" field. static const uint8_t IGNORE_SCISSOR = 0x10; - static const uint8_t IGNORE_VIEWPORT = 0x20; }; struct RenderPassParams {