diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index a8fe42d3..eb0d9d7d 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -2134,19 +2134,31 @@ namespace MobileGL::MG_Backend::DirectGLES { if (tailSpanDirty) { // Scissor box. Resolved and shadowed like the viewport above, and // for the same reason: what has to reach the driver is NOT simply the parameter - // field. (0,0,0,0) is where RenderStateParameters::ScissorBox starts and the only - // thing that ever writes it is glScissor, so that value means "the application has - // never called glScissor" - it is not a GL scissor box. GL's initial box is the - // whole window, which the frontend has no way to spell before a surface exists. - // The pre-resync code got away with pushing the field verbatim only by accident: - // the shadow held the same default, the field never compared unequal, and the ES - // context kept its own correct default. Under the forced full push that accident - // is gone, glScissor(0,0,0,0) shrinks the scissor to an EMPTY rectangle, and - // everything drawn with GL_SCISSOR_TEST enabled before the app's first glScissor - // is clipped away - Minecraft 26.2 keeps only its unscissored sky and hand and - // loses the terrain and the whole GUI. + // field. RenderStateParameters::ScissorBoxes starts all-zero, which means "the + // application has never called glScissor" - it is not a GL scissor box. GL's + // initial box is the whole window, which the frontend has no way to spell before a + // surface exists. The pre-resync code got away with pushing the field verbatim only + // by accident: the shadow held the same default, the field never compared unequal, + // and the ES context kept its own correct default. Under the forced full push that + // accident is gone, glScissor(0,0,0,0) shrinks the scissor to an EMPTY rectangle, + // and everything drawn with GL_SCISSOR_TEST enabled before the app's first + // glScissor is clipped away - Minecraft 26.2 keeps only its unscissored sky and + // hand and loses the terrain and the whole GUI. + // + // The condition is the WRITTEN FLAG, not the extent. An empty rectangle is a + // perfectly legal thing to ask for - glScissor(0,0,0,0) means "the scissor test + // rejects every fragment" - so testing `width <= 0 || height <= 0` substituted the + // whole surface for a deliberately empty box and inverted the request into "accept + // every fragment", no matter how many times the application had already called + // glScissor. KHR-GL43.viewport_array.scissor_zero_dimension is exactly that: all 16 + // boxes zero-sized with the test enabled, requiring the draw to be clipped away + // entirely. Reading the flag preserves the Minecraft protection bit-for-bit - before + // the first glScissor the bit is clear and the surface size is still substituted - + // while an explicit empty box now reaches the driver verbatim. Negative extents + // cannot arrive here at all: all three entry points reject them with + // GL_INVALID_VALUE before storing (GL_RenderState.cpp's ValidateNonNegativeExtent). IntVec4 backendScissorBox = parameters.ScissorBoxes[0]; - if (backendScissorBox.z() <= 0 || backendScissorBox.w() <= 0) { + if ((parameters.ScissorBoxWrittenMask & 1u) == 0) { Int surfaceWidth = 0; Int surfaceHeight = 0; if (QueryCurrentSurfaceSize(surfaceWidth, surfaceHeight)) { diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ViewportArrayScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ViewportArrayScenario.cpp index c53c3105..44a75679 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ViewportArrayScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ViewportArrayScenario.cpp @@ -520,5 +520,215 @@ void main() { fragColor = vec4(float(gsIndex) * 16.0 / 255.0, 0.0, 0.0, 1.0); } DestroyIntTarget(target); } + // --- 4. an explicitly EMPTY scissor box clips, it does not mean "never written" -------- + // + // Deliberately NOT a ViewportArrayScenario case, because it must run on DirectGLES - the + // backend that got it wrong - and that fixture skips there. It needs none of the routing: + // one viewport, one scissor rectangle, no geometry stage. + // + // glScissor(0, 0, 0, 0) is legal GL meaning "the scissor test rejects every fragment", + // but it is byte-identical to the all-zero rectangle a context starts with, whose meaning + // is the OPPOSITE ("the whole window", which the frontend cannot spell before a surface + // exists). DirectGLES resolved the collision from the EXTENT, so it substituted the whole + // surface for a deliberately empty box and inverted the request into "clip nothing" - + // and did so on every draw, at any origin, no matter how many times the application had + // already called glScissor. KHR-GL43.viewport_array.scissor_zero_dimension is the + // conformance shape of exactly this, and it is what the written-flag now separates. + + const char* const kFullScreenVertexSource = R"(#version 330 core +void main() { + // One clip-space-covering triangle straight from gl_VertexID: no buffers, no attributes, + // and nothing that could clip the draw except the scissor rectangle under test. + const vec2 corners[3] = vec2[3](vec2(-1.0, -1.0), vec2(3.0, -1.0), vec2(-1.0, 3.0)); + gl_Position = vec4(corners[gl_VertexID], 0.0, 1.0); +} +)"; + + const char* const kConstantIntFragmentSource = R"(#version 330 core +layout(location = 0) out int fragColor; +void main() { fragColor = 7; } +)"; + constexpr GLint kPainted = 7; + + class EmptyScissorScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + + m_program = BuildQuadProgram(); + ASSERT_NE(m_program, 0u) << "full-screen program failed to build: " << m_buildLog; + glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + + glGenTextures(1, &m_texture); + glBindTexture(GL_TEXTURE_2D, m_texture); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glTexImage2D(GL_TEXTURE_2D, 0, GL_R32I, kSurfaceSide, kSurfaceSide, 0, GL_RED_INTEGER, GL_INT, + nullptr); + glGenFramebuffers(1, &m_fbo); + glBindFramebuffer(GL_FRAMEBUFFER, m_fbo); + glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, m_texture, 0); + ASSERT_EQ(glCheckFramebufferStatus(GL_FRAMEBUFFER), GL_FRAMEBUFFER_COMPLETE) + << "R32I is required to be colour-renderable; an incomplete target would make every " + "assertion below vacuous"; + + glViewport(0, 0, kSurfaceSide, kSurfaceSide); + glDisable(GL_DEPTH_TEST); + ResetScissorState(); + ASSERT_EQ(glGetError(), GL_NO_ERROR) << "setup left a GL error behind"; + } + + void TearDown() override { + if (!Ready() || IsSkipped()) return; + // The context is shared with every other scenario in the process, and a leftover + // 0x0 scissor box with the test enabled would silently blank whatever runs next. + ResetScissorState(); + glScissor(0, 0, kSurfaceSide, kSurfaceSide); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + if (m_program != 0) glDeleteProgram(m_program); + glBindFramebuffer(GL_FRAMEBUFFER, 0); + if (m_fbo != 0) glDeleteFramebuffers(1, &m_fbo); + if (m_texture != 0) glDeleteTextures(1, &m_texture); + while (glGetError() != GL_NO_ERROR) { + } + } + + static void ResetScissorState() { + for (int i = 0; i < kViewportCount; ++i) { + glDisablei(GL_SCISSOR_TEST, static_cast(i)); + } + glDisable(GL_SCISSOR_TEST); + } + + // Uploaded, not cleared, for the reason FillIntTarget gives - and here for a second + // one that is decisive: glClear is ITSELF scissored, so a clear issued under the very + // state this case is testing would be clipped away and prove nothing. + void FillTarget() const { + const std::vector unwritten(static_cast(kSurfaceSide) * kSurfaceSide, kUnwritten); + glBindTexture(GL_TEXTURE_2D, m_texture); + glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kSurfaceSide, kSurfaceSide, GL_RED_INTEGER, GL_INT, + unwritten.data()); + } + + static std::vector ReadTarget() { + std::vector pixels(static_cast(kSurfaceSide) * kSurfaceSide, 0); + glReadPixels(0, 0, kSurfaceSide, kSurfaceSide, GL_RED_INTEGER, GL_INT, pixels.data()); + return pixels; + } + + GLuint BuildQuadProgram() { + const GLuint vs = CompileOne(GL_VERTEX_SHADER, kFullScreenVertexSource); + if (vs == 0) return 0; + const GLuint fs = CompileOne(GL_FRAGMENT_SHADER, kConstantIntFragmentSource); + if (fs == 0) { + glDeleteShader(vs); + return 0; + } + const GLuint program = glCreateProgram(); + glAttachShader(program, vs); + glAttachShader(program, fs); + glLinkProgram(program); + GLint linked = 0; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + glDeleteShader(vs); + glDeleteShader(fs); + if (linked) return program; + GLint length = 0; + glGetProgramiv(program, GL_INFO_LOG_LENGTH, &length); + std::vector log(static_cast(length > 1 ? length : 1), '\0'); + glGetProgramInfoLog(program, static_cast(log.size()), nullptr, log.data()); + m_buildLog = log.data(); + glDeleteProgram(program); + return 0; + } + + GLuint CompileOne(GLenum stage, const char* source) { + const GLuint shader = glCreateShader(stage); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + GLint compiled = 0; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + if (compiled) return shader; + GLint length = 0; + glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length); + std::vector log(static_cast(length > 1 ? length : 1), '\0'); + glGetShaderInfoLog(shader, static_cast(log.size()), nullptr, log.data()); + m_buildLog = log.data(); + glDeleteShader(shader); + return 0; + } + + std::string m_buildLog; + GLuint m_program = 0; + GLuint m_vao = 0; + GLuint m_fbo = 0; + GLuint m_texture = 0; + }; + + TEST_F(EmptyScissorScenario, AnExplicitlyEmptyScissorBoxClipsEveryFragment) { + // Positive control FIRST. Without it a regression that simply lost the draw entirely + // would sail through the half below, which only asserts that nothing was painted. + FillTarget(); + glEnable(GL_SCISSOR_TEST); + glScissor(0, 0, kSurfaceSide, kSurfaceSide); + glUseProgram(m_program); + glBindVertexArray(m_vao); + glDrawArrays(GL_TRIANGLES, 0, 3); + ASSERT_EQ(glGetError(), GL_NO_ERROR); + { + const std::vector pixels = ReadTarget(); + ASSERT_EQ(pixels.front(), kPainted) << "control: a full-surface scissor box must not clip"; + ASSERT_EQ(pixels.back(), kPainted) << "control: a full-surface scissor box must not clip"; + } + + // The case itself, and note it runs AFTER an explicit glScissor - the old + // extent-based sentinel misfired here too, which is what made this a live rendering + // bug and not just a first-frame startup quirk. + FillTarget(); + glScissor(0, 0, 0, 0); + glDrawArrays(GL_TRIANGLES, 0, 3); + ASSERT_EQ(glGetError(), GL_NO_ERROR); + { + const std::vector pixels = ReadTarget(); + for (size_t i = 0; i < pixels.size(); ++i) { + ASSERT_EQ(pixels[i], kUnwritten) + << "texel " << i << " was painted through a 0x0 scissor box: the empty rectangle was " + "substituted with the whole surface, inverting 'clip everything' into 'clip nothing'"; + } + } + } + + TEST_F(EmptyScissorScenario, IndexedZeroDimensionScissorBoxesClipEveryFragment) { + // The conformance shape: setup4x4Scissor(..., set_zeros=true) writes all 16 boxes + // through glScissorArrayv with zero extents at a 4x4 grid of origins and enables the + // test on every index. Index 0's box is (0, 0, 0, 0) - byte-identical to the + // never-written default - which is precisely the collision the written flag breaks. + // Backends that collapse every index to 0 (DirectGLES today) still pass: index 0's + // box is empty, so the draw is clipped away, which is what the case requires. + FillTarget(); + std::vector boxes(static_cast(kViewportCount) * 4, 0); + for (int i = 0; i < kViewportCount; ++i) { + boxes[static_cast(i) * 4 + 0] = (i % kGridSide) * kCellSize; + boxes[static_cast(i) * 4 + 1] = (i / kGridSide) * kCellSize; + // width and height stay 0 - that IS the case. + } + glScissorArrayv(0, kViewportCount, boxes.data()); + for (int i = 0; i < kViewportCount; ++i) { + glEnablei(GL_SCISSOR_TEST, static_cast(i)); + } + glUseProgram(m_program); + glBindVertexArray(m_vao); + glDrawArrays(GL_TRIANGLES, 0, 3); + ASSERT_EQ(glGetError(), GL_NO_ERROR); + + const std::vector pixels = ReadTarget(); + for (size_t i = 0; i < pixels.size(); ++i) { + ASSERT_EQ(pixels[i], kUnwritten) << "texel " << i << " was painted through a zero-extent indexed " + "scissor box"; + } + } + } // namespace } // namespace MGITest diff --git a/MobileGL/MG_State/GLState/RenderState/RenderState.cpp b/MobileGL/MG_State/GLState/RenderState/RenderState.cpp index f2eb4503..8a394c37 100644 --- a/MobileGL/MG_State/GLState/RenderState/RenderState.cpp +++ b/MobileGL/MG_State/GLState/RenderState/RenderState.cpp @@ -843,6 +843,21 @@ namespace MobileGL { stored = box; stateChanged = true; } + // "The application has written this rectangle" is a DIFFERENT predicate from "the + // value moved", and the backends need the first one: glScissor(0, 0, 0, 0) as the + // very first scissor call leaves every stored box byte-identical to its + // never-written default, and that call is precisely the one whose meaning a + // backend must stop guessing at (see ScissorBoxWrittenMask). + // + // The transition has to count as a state change for the version too. DirectGLES' + // SyncRenderState early-outs on an unchanged render-state version BEFORE it + // reaches the span memcmp that would otherwise notice the mask, so a version-less + // flag flip would sit in the parameter block and never be pushed. It is a + // once-per-index transition, so the steady state still costs nothing. + if (m_parameters.ScissorBoxWrittenMask != kAllViewportsMask) { + m_parameters.ScissorBoxWrittenMask = kAllViewportsMask; + stateChanged = true; + } if (stateChanged) ++m_version; } @@ -855,9 +870,15 @@ namespace MobileGL { MOBILEGL_ASSERT(false, "Scissor box index out of range: %u", index); return; } - if (m_parameters.ScissorBoxes[index] == box) return; + // See SetScissorBox: a first write is state even when it does not move the value, + // so the unchanged-value early-out may only fire once this index is already + // marked written. + const Uint32 writtenBit = 1u << index; + const Bool alreadyWritten = (m_parameters.ScissorBoxWrittenMask & writtenBit) != 0; + if (alreadyWritten && m_parameters.ScissorBoxes[index] == box) return; m_parameters.ScissorBoxes[index] = box; + m_parameters.ScissorBoxWrittenMask |= writtenBit; ++m_version; } diff --git a/MobileGL/MG_State/GLState/RenderState/RenderState.h b/MobileGL/MG_State/GLState/RenderState/RenderState.h index 73b51fbd..05e90b24 100644 --- a/MobileGL/MG_State/GLState/RenderState/RenderState.h +++ b/MobileGL/MG_State/GLState/RenderState/RenderState.h @@ -328,6 +328,18 @@ namespace MobileGL { // turns it into a real glEnable/glDisable. Uint32 ScissorTestEnabledMask = 0; Array ScissorBoxes{}; // x, y, width, height + // One bit per viewport, set the first time the application writes that index's scissor + // rectangle - glScissor broadcasts and sets all 16, glScissorIndexed/glScissorArrayv set + // the indices they name. It exists because the RECTANGLE cannot answer "has the + // application spoken?": ScissorBoxes starts all-zero (its spec initial value is the size + // of a window the frontend does not know yet, see the RenderState constructor), and + // glScissor(0, 0, 0, 0) is a legal GL state meaning "the scissor test rejects every + // fragment". A backend that reads an empty rectangle as the never-written sentinel + // therefore INVERTS that request into "accept every fragment"; DirectGLES did exactly + // that and KHR-GL43.viewport_array.scissor_zero_dimension caught it. Deliberately beside + // ScissorBoxes so it shares their tail span (after LogicOp) and DirectGLES' span memcmp + // picks a transition up like any other state. + Uint32 ScissorBoxWrittenMask = 0; // glEnable(GL_CLIP_DISTANCE0 + i) for i in [0, 8), one bit each. A bitmask rather than // eight bools because every consumer wants the set, not an individual flag, and because // the SYNC_CAPABILITY/SET_CAPABILITY macros key off a "Enabled" field name that diff --git a/MobileGL/MG_Test/State/RenderStateTest.cpp b/MobileGL/MG_Test/State/RenderStateTest.cpp index 66ed6033..b4a2a7f7 100644 --- a/MobileGL/MG_Test/State/RenderStateTest.cpp +++ b/MobileGL/MG_Test/State/RenderStateTest.cpp @@ -559,3 +559,76 @@ TEST_F(RenderStateTest, IndexedRectangleQueriesRejectAnOutOfRangeIndex) { MG_Impl::GLImpl::GetDoublei_v(GL_DEPTH_RANGE, kMaxViewports - 1, doubles); ExpectSingleGlError(GL_NO_ERROR); } + +// --------------------------------------------------------------------------------------------- +// "Has the application written this scissor rectangle?" - the flag, not the extent +// --------------------------------------------------------------------------------------------- +// glScissor(0, 0, 0, 0) is legal GL and means "the scissor test rejects every fragment", but it +// is byte-identical to the never-written default, whose meaning is the opposite ("the whole +// window", which the frontend cannot spell before a surface exists). DirectGLES resolved the two +// by looking at the EXTENT and so inverted every deliberately empty box into the full surface - +// KHR-GL43.viewport_array.scissor_zero_dimension is exactly that draw, and it came back holding +// the drawn colour where the untouched fill was required. +// +// These drive RenderState directly instead of the GL entry points on purpose: the flag's whole +// content is what it says BEFORE the first scissor call of a context, and this binary shares one +// context across every case in the file, so a pristine object is the only place that state +// still exists by the time these run. + +namespace { + constexpr Uint32 kAllViewportsWritten = + RenderStateParameters::MAX_VIEWPORTS >= 32 ? ~0u : (1u << RenderStateParameters::MAX_VIEWPORTS) - 1u; +} // namespace + +TEST_F(RenderStateTest, AnEmptyScissorBoxIsDistinguishableFromNeverHavingBeenWritten) { + MG_State::GLState::RenderState state; + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, 0u) + << "a fresh context has never been given a scissor box, and the all-zero rectangle it " + "starts with must not be mistaken for one"; + + // Not one stored byte moves here - every box already held (0,0,0,0) - and yet this is the + // call that turns "the frontend does not know the window size" into "reject every fragment". + state.SetScissorBox(IntVec4(0, 0, 0, 0)); + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, kAllViewportsWritten); + for (GLuint index = 0; index < kMaxViewports; ++index) { + EXPECT_EQ(state.GetScissorBoxIndexed(index), IntVec4(0, 0, 0, 0)) << "index " << index; + } +} + +TEST_F(RenderStateTest, AnIndexedScissorWriteClaimsOnlyItsOwnIndex) { + MG_State::GLState::RenderState state; + state.SetScissorBoxIndexed(5, IntVec4(0, 0, 0, 0)); + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, 1u << 5); + + state.SetScissorBoxIndexed(0, IntVec4(0, 0, 0, 0)); + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, (1u << 5) | 1u); + + // ARB_viewport_array makes the non-indexed setter a write to every index, so it claims all 16. + state.SetScissorBox(IntVec4(1, 2, 3, 4)); + EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, kAllViewportsWritten); +} + +TEST_F(RenderStateTest, TheFirstScissorWriteBumpsTheVersionEvenWhenTheValueDoesNotMove) { + // Load-bearing, and not merely tidy: DirectGLES' SyncRenderState early-outs on an unchanged + // render-state version BEFORE it reaches the span memcmp that would otherwise notice the + // flag. A version-less transition would sit in the parameter block, never be pushed, and the + // empty box would go on rendering as the whole surface. + MG_State::GLState::RenderState state; + const Uint initial = state.GetVersion(); + state.SetScissorBox(IntVec4(0, 0, 0, 0)); + EXPECT_GT(state.GetVersion(), initial) << "claiming the rectangle is itself a state change"; + + // Once claimed, a genuinely redundant write stays free - the flag costs one transition, not + // a version bump per call. + const Uint settled = state.GetVersion(); + state.SetScissorBox(IntVec4(0, 0, 0, 0)); + EXPECT_EQ(state.GetVersion(), settled); + + MG_State::GLState::RenderState indexed; + const Uint indexedInitial = indexed.GetVersion(); + indexed.SetScissorBoxIndexed(3, IntVec4(0, 0, 0, 0)); + EXPECT_GT(indexed.GetVersion(), indexedInitial); + const Uint indexedSettled = indexed.GetVersion(); + indexed.SetScissorBoxIndexed(3, IntVec4(0, 0, 0, 0)); + EXPECT_EQ(indexed.GetVersion(), indexedSettled); +}