From bd666406f4635168b0dc1f4ce68810a6b580e13d Mon Sep 17 00:00:00 2001 From: Olivier Prat Date: Tue, 19 Dec 2017 14:17:17 +0100 Subject: [PATCH] Removed glColor reset hack in MeshPartPayload and replaced it by a reset of the color attribute to white in the execution of the setInputFormat command of the various GLBackends --- .../src/RenderablePolyLineEntityItem.cpp | 1 + .../src/RenderableShapeEntityItem.cpp | 5 ++-- .../src/RenderableWebEntityItem.cpp | 1 - libraries/gpu-gl/src/gpu/gl/GLBackend.cpp | 4 +++ libraries/gpu-gl/src/gpu/gl/GLBackend.h | 1 + .../gpu-gl/src/gpu/gl41/GL41BackendInput.cpp | 13 ++++++++ .../gpu-gl/src/gpu/gl45/GL45BackendInput.cpp | 14 +++++++++ libraries/render-utils/src/GeometryCache.cpp | 30 +++++++++++++++++++ libraries/render-utils/src/GeometryCache.h | 6 ++++ .../render-utils/src/MeshPartPayload.cpp | 10 ------- tests/gpu-test/src/TestShapes.cpp | 9 +++--- 11 files changed, 75 insertions(+), 19 deletions(-) diff --git a/libraries/entities-renderer/src/RenderablePolyLineEntityItem.cpp b/libraries/entities-renderer/src/RenderablePolyLineEntityItem.cpp index fbf85fa8c2..21764dff7f 100644 --- a/libraries/entities-renderer/src/RenderablePolyLineEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderablePolyLineEntityItem.cpp @@ -303,6 +303,7 @@ void PolyLineEntityRenderer::doRender(RenderArgs* args) { batch.setInputBuffer(0, _verticesBuffer, 0, sizeof(Vertex)); #ifndef POLYLINE_ENTITY_USE_FADE_EFFECT + // glColor4f must be called after setInputFormat if it must be taken into account if (_isFading) { batch._glColor4f(1.0f, 1.0f, 1.0f, Interpolate::calculateFadeRatio(_fadeStartTime)); } else { diff --git a/libraries/entities-renderer/src/RenderableShapeEntityItem.cpp b/libraries/entities-renderer/src/RenderableShapeEntityItem.cpp index 3524709395..b00ad0259e 100644 --- a/libraries/entities-renderer/src/RenderableShapeEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableShapeEntityItem.cpp @@ -137,11 +137,10 @@ void ShapeEntityRenderer::doRender(RenderArgs* args) { }); if (proceduralRender) { - batch._glColor4f(outColor.r, outColor.g, outColor.b, outColor.a); if (render::ShapeKey(args->_globalShapeKey).isWireframe()) { - geometryCache->renderWireShape(batch, geometryShape); + geometryCache->renderWireShape(batch, geometryShape, outColor); } else { - geometryCache->renderShape(batch, geometryShape); + geometryCache->renderShape(batch, geometryShape, outColor); } } else { // FIXME, support instanced multi-shape rendering using multidraw indirect diff --git a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp index cdd260e73c..b253e73b87 100644 --- a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp @@ -188,7 +188,6 @@ void WebEntityRenderer::doRender(RenderArgs* args) { }); batch.setResourceTexture(0, _texture); float fadeRatio = _isFading ? Interpolate::calculateFadeRatio(_fadeStartTime) : 1.0f; - batch._glColor4f(1.0f, 1.0f, 1.0f, fadeRatio); DependencyManager::get()->bindWebBrowserProgram(batch, fadeRatio < OPAQUE_ALPHA_THRESHOLD); DependencyManager::get()->renderQuad(batch, topLeft, bottomRight, texMin, texMax, glm::vec4(1.0f, 1.0f, 1.0f, fadeRatio), _geometryId); diff --git a/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp b/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp index 6fb0d7b152..93077e240b 100644 --- a/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp +++ b/libraries/gpu-gl/src/gpu/gl/GLBackend.cpp @@ -605,6 +605,10 @@ void GLBackend::do_glColor4f(const Batch& batch, size_t paramOffset) { if (_input._colorAttribute != newColor) { _input._colorAttribute = newColor; glVertexAttrib4fv(gpu::Stream::COLOR, &_input._colorAttribute.r); + // Color has been changed and is not white. To prevent colors from bleeding + // between different objects, we need to set the _hadColorAttribute flag + // as if a previous render call had potential colors + _input._hadColorAttribute = (newColor != glm::vec4(1.0f, 1.0f, 1.0f, 1.0f)); } (void)CHECK_GL_ERROR(); } diff --git a/libraries/gpu-gl/src/gpu/gl/GLBackend.h b/libraries/gpu-gl/src/gpu/gl/GLBackend.h index 1908db614d..5558d3ada1 100644 --- a/libraries/gpu-gl/src/gpu/gl/GLBackend.h +++ b/libraries/gpu-gl/src/gpu/gl/GLBackend.h @@ -253,6 +253,7 @@ protected: struct InputStageState { bool _invalidFormat { true }; + bool _hadColorAttribute{ true }; Stream::FormatPointer _format; std::string _formatKey; diff --git a/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp b/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp index e8ebcbe05c..42bd56e6e4 100644 --- a/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp +++ b/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp @@ -62,6 +62,8 @@ void GL41Backend::updateInput() { // now we need to bind the buffers and assign the attrib pointers if (_input._format) { + bool hasColorAttribute{ false }; + const Buffers& buffers = _input._buffers; const Offsets& offsets = _input._bufferOffsets; const Offsets& strides = _input._bufferStrides; @@ -98,6 +100,8 @@ void GL41Backend::updateInput() { uintptr_t pointer = (uintptr_t)(attrib._offset + offsets[bufferNum]); GLboolean isNormalized = attrib._element.isNormalized(); + hasColorAttribute = hasColorAttribute || (slot == Stream::COLOR); + for (size_t locNum = 0; locNum < locationCount; ++locNum) { if (attrib._element.isInteger()) { glVertexAttribIPointer(slot + (GLuint)locNum, count, type, stride, @@ -117,6 +121,15 @@ void GL41Backend::updateInput() { } } } + + if (_input._hadColorAttribute && !hasColorAttribute) { + // The previous input stage had a color attribute but this one doesn't so reset + // color to pure white. + const auto white = glm::vec4(1.0f, 1.0f, 1.0f, 1.0f); + glVertexAttrib4fv(Stream::COLOR, &white.r); + _input._colorAttribute = white; + } + _input._hadColorAttribute = hasColorAttribute; } // everything format related should be in sync now _input._invalidFormat = false; diff --git a/libraries/gpu-gl/src/gpu/gl45/GL45BackendInput.cpp b/libraries/gpu-gl/src/gpu/gl45/GL45BackendInput.cpp index ece62e15f1..4a43fc988c 100644 --- a/libraries/gpu-gl/src/gpu/gl45/GL45BackendInput.cpp +++ b/libraries/gpu-gl/src/gpu/gl45/GL45BackendInput.cpp @@ -32,6 +32,8 @@ void GL45Backend::updateInput() { // Assign the vertex format required if (_input._format) { + bool hasColorAttribute{ false }; + _input._attribBindingBuffers.reset(); const Stream::Format::AttributeMap& attributes = _input._format->getAttributes(); @@ -54,6 +56,9 @@ void GL45Backend::updateInput() { GLboolean isNormalized = attrib._element.isNormalized(); GLenum perLocationSize = attrib._element.getLocationSize(); + + hasColorAttribute = hasColorAttribute || (slot == Stream::COLOR); + for (GLuint locNum = 0; locNum < locationCount; ++locNum) { GLuint attriNum = (GLuint)(slot + locNum); newActivation.set(attriNum); @@ -84,6 +89,15 @@ void GL45Backend::updateInput() { glVertexBindingDivisor(bufferChannelNum, frequency); #endif } + + if (_input._hadColorAttribute && !hasColorAttribute) { + // The previous input stage had a color attribute but this one doesn't so reset + // color to pure white. + const auto white = glm::vec4(1.0f, 1.0f, 1.0f, 1.0f); + glVertexAttrib4fv(Stream::COLOR, &white.r); + _input._colorAttribute = white; + } + _input._hadColorAttribute = hasColorAttribute; } // Manage Activation what was and what is expected now diff --git a/libraries/render-utils/src/GeometryCache.cpp b/libraries/render-utils/src/GeometryCache.cpp index 76c354bdf8..2616d08600 100644 --- a/libraries/render-utils/src/GeometryCache.cpp +++ b/libraries/render-utils/src/GeometryCache.cpp @@ -760,6 +760,20 @@ void GeometryCache::renderWireShape(gpu::Batch& batch, Shape shape) { _shapes[shape].drawWire(batch); } +void GeometryCache::renderShape(gpu::Batch& batch, Shape shape, const glm::vec4& color) { + batch.setInputFormat(getSolidStreamFormat()); + // Color must be set after input format + batch._glColor4f(color.r, color.g, color.b, color.a); + _shapes[shape].draw(batch); +} + +void GeometryCache::renderWireShape(gpu::Batch& batch, Shape shape, const glm::vec4& color) { + batch.setInputFormat(getSolidStreamFormat()); + // Color must be set after input format + batch._glColor4f(color.r, color.g, color.b, color.a); + _shapes[shape].drawWire(batch); +} + void setupBatchInstance(gpu::Batch& batch, gpu::BufferPointer colorBuffer) { gpu::BufferView colorView(colorBuffer, COLOR_ELEMENT); batch.setInputBuffer(gpu::Stream::COLOR, colorView); @@ -811,6 +825,14 @@ void GeometryCache::renderWireCube(gpu::Batch& batch) { renderWireShape(batch, Cube); } +void GeometryCache::renderCube(gpu::Batch& batch, const glm::vec4& color) { + renderShape(batch, Cube, color); +} + +void GeometryCache::renderWireCube(gpu::Batch& batch, const glm::vec4& color) { + renderWireShape(batch, Cube, color); +} + void GeometryCache::renderSphere(gpu::Batch& batch) { renderShape(batch, Sphere); } @@ -819,6 +841,14 @@ void GeometryCache::renderWireSphere(gpu::Batch& batch) { renderWireShape(batch, Sphere); } +void GeometryCache::renderSphere(gpu::Batch& batch, const glm::vec4& color) { + renderShape(batch, Sphere, color); +} + +void GeometryCache::renderWireSphere(gpu::Batch& batch, const glm::vec4& color) { + renderWireShape(batch, Sphere, color); +} + void GeometryCache::renderGrid(gpu::Batch& batch, const glm::vec2& minCorner, const glm::vec2& maxCorner, int majorRows, int majorCols, float majorEdge, int minorRows, int minorCols, float minorEdge, diff --git a/libraries/render-utils/src/GeometryCache.h b/libraries/render-utils/src/GeometryCache.h index cd8c43f1df..0585cc9e55 100644 --- a/libraries/render-utils/src/GeometryCache.h +++ b/libraries/render-utils/src/GeometryCache.h @@ -251,14 +251,20 @@ public: // Dynamic geometry void renderShape(gpu::Batch& batch, Shape shape); void renderWireShape(gpu::Batch& batch, Shape shape); + void renderShape(gpu::Batch& batch, Shape shape, const glm::vec4& color); + void renderWireShape(gpu::Batch& batch, Shape shape, const glm::vec4& color); size_t getShapeTriangleCount(Shape shape); void renderCube(gpu::Batch& batch); void renderWireCube(gpu::Batch& batch); + void renderCube(gpu::Batch& batch, const glm::vec4& color); + void renderWireCube(gpu::Batch& batch, const glm::vec4& color); size_t getCubeTriangleCount(); void renderSphere(gpu::Batch& batch); void renderWireSphere(gpu::Batch& batch); + void renderSphere(gpu::Batch& batch, const glm::vec4& color); + void renderWireSphere(gpu::Batch& batch, const glm::vec4& color); size_t getSphereTriangleCount(); void renderGrid(gpu::Batch& batch, const glm::vec2& minCorner, const glm::vec2& maxCorner, diff --git a/libraries/render-utils/src/MeshPartPayload.cpp b/libraries/render-utils/src/MeshPartPayload.cpp index 1ea3e1a705..cf895a73fa 100644 --- a/libraries/render-utils/src/MeshPartPayload.cpp +++ b/libraries/render-utils/src/MeshPartPayload.cpp @@ -122,11 +122,6 @@ void MeshPartPayload::bindMesh(gpu::Batch& batch) { batch.setInputFormat((_drawMesh->getVertexFormat())); batch.setInputStream(0, _drawMesh->getVertexStream()); - - // TODO: Get rid of that extra call - if (!_hasColorAttrib) { - batch._glColor4f(1.0f, 1.0f, 1.0f, 1.0f); - } } void MeshPartPayload::bindMaterial(gpu::Batch& batch, const ShapePipeline::LocationsPointer locations, bool enableTextures) const { @@ -526,11 +521,6 @@ void ModelMeshPartPayload::bindMesh(gpu::Batch& batch) { batch.setInputStream(0, _drawMesh->getVertexStream()); } } - - // TODO: Get rid of that extra call - if (!_hasColorAttrib) { - batch._glColor4f(1.0f, 1.0f, 1.0f, 1.0f); - } } void ModelMeshPartPayload::bindTransform(gpu::Batch& batch, const ShapePipeline::LocationsPointer locations, RenderArgs::RenderMode renderMode) const { diff --git a/tests/gpu-test/src/TestShapes.cpp b/tests/gpu-test/src/TestShapes.cpp index 253d89cf61..67a348c002 100644 --- a/tests/gpu-test/src/TestShapes.cpp +++ b/tests/gpu-test/src/TestShapes.cpp @@ -29,19 +29,18 @@ void TestShapes::renderTest(size_t testId, RenderArgs* args) { float seconds = secTimestampNow() - startSecs; seconds /= 4.0f; batch.setModelTransform(Transform()); - batch._glColor4f(0.8f, 0.25f, 0.25f, 1.0f); + const auto color = glm::vec4(0.8f, 0.25f, 0.25f, 1.0f); bool wire = (seconds - floorf(seconds) > 0.5f); int shapeIndex = ((int)seconds) % TYPE_COUNT; if (wire) { - geometryCache->renderWireShape(batch, SHAPE[shapeIndex]); + geometryCache->renderWireShape(batch, SHAPE[shapeIndex], color); } else { - geometryCache->renderShape(batch, SHAPE[shapeIndex]); + geometryCache->renderShape(batch, SHAPE[shapeIndex], color); } batch.setModelTransform(Transform().setScale(1.01f)); - batch._glColor4f(1, 1, 1, 1); - geometryCache->renderWireCube(batch); + geometryCache->renderWireCube(batch, glm::vec4(1,1,1,1)); }