From c1d89de729b6db7d4a04cda672cd654efc911235 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 27 Aug 2026 05:10:26 -0400 Subject: [PATCH] [Fix] (Sampler): carry GL_TEXTURE_BORDER_COLOR with its form, convert per GL 4.6 eq 2.2/2.3, and unify the name and scalar-pname error classes --- .../MG_Impl/GLImpl/Sampler/GL_Sampler.cpp | 86 ++++++++++++++----- .../MG_Impl/GLImpl/Sampler/Validators.cpp | 10 ++- .../GLState/SamplerState/SamplerObject.cpp | 30 ++++++- .../GLState/SamplerState/SamplerObject.h | 15 ++++ .../GLState/TextureState/TextureObject.cpp | 22 ++++- .../GLState/TextureState/TextureObject.h | 3 + MobileGL/MG_Util/Math/FixedPointConversion.h | 49 +++++++++++ 7 files changed, 187 insertions(+), 28 deletions(-) create mode 100644 MobileGL/MG_Util/Math/FixedPointConversion.h diff --git a/MobileGL/MG_Impl/GLImpl/Sampler/GL_Sampler.cpp b/MobileGL/MG_Impl/GLImpl/Sampler/GL_Sampler.cpp index 912d6ef3..afc82528 100644 --- a/MobileGL/MG_Impl/GLImpl/Sampler/GL_Sampler.cpp +++ b/MobileGL/MG_Impl/GLImpl/Sampler/GL_Sampler.cpp @@ -13,6 +13,7 @@ #include #include #include +#include namespace MobileGL::MG_Impl::GLImpl { namespace { @@ -22,6 +23,13 @@ namespace MobileGL::MG_Impl::GLImpl { return static_cast(*(const GLint*)param); } + // GL_TEXTURE_BORDER_COLOR is the only sampler parameter with more than one component, and it + // is also the only one whose meaning depends on WHICH entry point wrote it. Everything else + // reads exactly one component and does not care. + Bool IsVectorOnlySamplerPname(GLenum pname) { + return pname == GL_TEXTURE_BORDER_COLOR; + } + Bool ValidateSamplerParameterValue(GLenum pname, const void* param, Bool isFloat, Bool isUnsignedInteger) { if (param == nullptr) return false; @@ -56,8 +64,15 @@ namespace MobileGL::MG_Impl::GLImpl { } } // namespace + // `isIntegerCommand` distinguishes the "I" spellings (glSamplerParameterIiv / Iuiv) from the + // plain ones. It only matters for GL_TEXTURE_BORDER_COLOR, and there it decides everything: + // GL 4.6 core 8.10 says the I forms store the components unmodified with an integer internal + // type, while glSamplerParameteriv converts them to floating point with equation 2.2. Routing + // both to the same setter - which is what this file used to do - meant glSamplerParameteriv + // stored raw integers (so a border of 255 became float 255.0 instead of the spec's ~1.19e-7) + // and glSamplerParameterIiv lost the fact that it was ever an integer at all. void SetSamplerParam_State(GLuint sampler, GLenum pname, const void* param, bool isFloat, - bool isUnsignedInteger) { + bool isUnsignedInteger, bool isIntegerCommand) { if (param == nullptr) return; if (!SamplerImpl::ValidateSamplerName(sampler)) return; @@ -112,6 +127,13 @@ namespace MobileGL::MG_Impl::GLImpl { if (isFloat) { const auto* values = (const GLfloat*)param; samplerObj->SetBorderColor(FloatVec4(values[0], values[1], values[2], values[3])); + } else if (!isIntegerCommand) { + // glSamplerParameteriv: GL 4.6 core equation 2.2 into the FLOAT border colour. + const auto* values = (const GLint*)param; + samplerObj->SetBorderColor(FloatVec4(MG_Util::SignedNormalizedInt32ToFloat(values[0]), + MG_Util::SignedNormalizedInt32ToFloat(values[1]), + MG_Util::SignedNormalizedInt32ToFloat(values[2]), + MG_Util::SignedNormalizedInt32ToFloat(values[3]))); } else if (isUnsignedInteger) { const auto* values = (const GLuint*)param; samplerObj->SetBorderColorUI(UintVec4(values[0], values[1], values[2], values[3])); @@ -128,7 +150,7 @@ namespace MobileGL::MG_Impl::GLImpl { } void GetSamplerParam_State(GLuint sampler, GLenum pname, void* params, bool isFloat, - bool isUnsignedInteger) { + bool isUnsignedInteger, bool isIntegerCommand) { if (params == nullptr) return; if (!SamplerImpl::ValidateSamplerName(sampler)) return; @@ -191,6 +213,16 @@ namespace MobileGL::MG_Impl::GLImpl { out[1] = color.y(); out[2] = color.z(); out[3] = color.w(); + } else if (!isIntegerCommand) { + // glGetSamplerParameteriv: the inverse of the write side, GL 4.6 core equation 2.3. + // Exactly inverse, so a {0,1,2,4} written with glSamplerParameteriv reads back as + // {0,1,2,4}; a bare truncating cast answered {0,0,0,0}. + const auto& color = samplerObj->GetBorderColor(); + auto* out = (GLint*)params; + out[0] = MG_Util::FloatToSignedNormalizedInt32(color.x()); + out[1] = MG_Util::FloatToSignedNormalizedInt32(color.y()); + out[2] = MG_Util::FloatToSignedNormalizedInt32(color.z()); + out[3] = MG_Util::FloatToSignedNormalizedInt32(color.w()); } else if (isUnsignedInteger) { const auto& color = samplerObj->GetBorderColorUI(); auto* out = (GLuint*)params; @@ -293,16 +325,10 @@ namespace MobileGL::MG_Impl::GLImpl { if (sampler == 0) { textureUnit.SetSamplerObject(nullptr); } else { - // GL 3.3 core 3.8.2: BindSampler on a name GenSamplers never returned - or one already - // deleted - is INVALID_OPERATION. SamplerParameter* raises INVALID_VALUE for the same - // name, which is why this cannot go through the shared SamplerImpl validator. - if (!MG_State::pGLContext->ValidateSamplerName(sampler)) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", "BindSampler_State", - std::format("Invalid sampler name {}", sampler))); - return; - } + // GL 4.6 core 8.2: BindSampler on a name GenSamplers never returned - or one already + // deleted - is INVALID_OPERATION, and so is every other sampler entry point on such a + // name, so the shared validator answers for all of them. + if (!SamplerImpl::ValidateSamplerName(sampler)) return; Bool doesSamplerObjectCreated = MG_State::pGLContext->ValidateSamplerObject(sampler); if (!doesSamplerObjectCreated) { MG_State::pGLContext->CreateSamplerObject(sampler); @@ -356,30 +382,50 @@ namespace MobileGL::MG_Impl::GLImpl { /* @INSERTION_POINT:FUNCTION_IMPLEMENTATION@ */ void GetSamplerParameteriv(GLuint sampler, GLenum pname, GLint* params) { - GetSamplerParam_State(sampler, pname, params, false, false); + GetSamplerParam_State(sampler, pname, params, false, false, false); } void SamplerParameterIuiv(GLuint sampler, GLenum pname, const GLuint* param) { - SetSamplerParam_State(sampler, pname, param, false, true); + SetSamplerParam_State(sampler, pname, param, false, true, true); } void SamplerParameterIiv(GLuint sampler, GLenum pname, const GLint* param) { - SetSamplerParam_State(sampler, pname, param, false, false); + SetSamplerParam_State(sampler, pname, param, false, false, true); } void SamplerParameteriv(GLuint sampler, GLenum pname, const GLint* param) { - SetSamplerParam_State(sampler, pname, param, false, false); + SetSamplerParam_State(sampler, pname, param, false, false, false); } void SamplerParameterfv(GLuint sampler, GLenum pname, const GLfloat* param) { - SetSamplerParam_State(sampler, pname, param, true, false); + SetSamplerParam_State(sampler, pname, param, true, false, false); } + // GL 4.6 core 8.10: the scalar spellings take "the value of pname", so a pname with more than one + // component is INVALID_ENUM here rather than something to read four components of. Guarding at + // the entry point rather than downstream is also what stops the vector path reading twelve bytes + // past the caller's single stack scalar - taking the address of a by-value argument and handing + // it to a four-component reader is what these used to do. The texture-side twins already answer + // INVALID_ENUM for GL_TEXTURE_BORDER_COLOR (TexParameteri/f name it as unsupported outright). void SamplerParameteri(GLuint sampler, GLenum pname, GLint param) { + if (IsVectorOnlySamplerPname(pname)) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidEnum, + MakeUnique("MG_Impl/GLImpl", "SamplerParameteri", + "pname has more than one component and needs a vector form.")); + return; + } SamplerParameteriv(sampler, pname, ¶m); } void SamplerParameterf(GLuint sampler, GLenum pname, GLfloat param) { + if (IsVectorOnlySamplerPname(pname)) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidEnum, + MakeUnique("MG_Impl/GLImpl", "SamplerParameterf", + "pname has more than one component and needs a vector form.")); + return; + } SamplerParameterfv(sampler, pname, ¶m); } @@ -388,15 +434,15 @@ namespace MobileGL::MG_Impl::GLImpl { } void GetSamplerParameterIuiv(GLuint sampler, GLenum pname, GLuint* params) { - GetSamplerParam_State(sampler, pname, params, false, true); + GetSamplerParam_State(sampler, pname, params, false, true, true); } void GetSamplerParameterIiv(GLuint sampler, GLenum pname, GLint* params) { - GetSamplerParam_State(sampler, pname, params, false, false); + GetSamplerParam_State(sampler, pname, params, false, false, true); } void GetSamplerParameterfv(GLuint sampler, GLenum pname, GLfloat* params) { - GetSamplerParam_State(sampler, pname, params, true, false); + GetSamplerParam_State(sampler, pname, params, true, false, false); } void GenSamplers(GLsizei count, GLuint* samplers) { diff --git a/MobileGL/MG_Impl/GLImpl/Sampler/Validators.cpp b/MobileGL/MG_Impl/GLImpl/Sampler/Validators.cpp index a1ef8cc2..5bb3bc9a 100644 --- a/MobileGL/MG_Impl/GLImpl/Sampler/Validators.cpp +++ b/MobileGL/MG_Impl/GLImpl/Sampler/Validators.cpp @@ -12,11 +12,17 @@ #include namespace MobileGL::MG_Impl::GLImpl::SamplerImpl { + // GL 4.6 core 8.2: "An INVALID_OPERATION error is generated if sampler is not the name of a + // sampler object previously returned from a call to GenSamplers." That class is shared by every + // sampler entry point - BindSampler, SamplerParameter*, GetSamplerParameter* - so this one gate + // answers for all of them. It used to report INVALID_VALUE (the GL 3.3 wording), which forced + // BindSampler to carry a bespoke duplicate of the same check just to get the class right. Bool ValidateSamplerName(GLuint sampler) { if (!MG_State::pGLContext->ValidateSamplerName(sampler)) { MG_State::pGLContext->RecordError( - ErrorCode::InvalidValue, MakeUnique("MG_Impl/GLImpl", "ValidateSamplerName", - std::format("Invalid sampler name {}", sampler))); + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", "ValidateSamplerName", + std::format("Invalid sampler name {}", sampler))); return false; } return true; diff --git a/MobileGL/MG_State/GLState/SamplerState/SamplerObject.cpp b/MobileGL/MG_State/GLState/SamplerState/SamplerObject.cpp index f2470f65..c1010888 100644 --- a/MobileGL/MG_State/GLState/SamplerState/SamplerObject.cpp +++ b/MobileGL/MG_State/GLState/SamplerState/SamplerObject.cpp @@ -155,9 +155,21 @@ namespace MobileGL { // an answer whichever form was written. Integer <-> float uses the plain value, matching // what glTexParameterIiv/Iuiv mean: those forms are for integer texture formats, whose // border components are the raw integers rather than a normalized fraction. + // + // Which of the three the application actually WROTE is recorded separately in + // borderColorForm, because the derived values erase it: a backend has to know whether to + // forward the colour through glSamplerParameterfv or glSamplerParameterIiv (and which + // VkBorderColor family to ask Vulkan for), and the numbers alone cannot say. That is also + // why every setter's early-out tests the form as well as the value - a float (0,0,0,1) + // followed by an integer (0,0,0,1) is a real state change even though nothing numeric + // moved, and swallowing it would leave the backend syncing the wrong entry point forever. void SamplerObject::SetBorderColor(const FloatVec4& color) { - if (color == m_samplerParameters.borderColor) return; + if (color == m_samplerParameters.borderColor && + m_samplerParameters.borderColorForm == BorderColorForm::Float) { + return; + } + m_samplerParameters.borderColorForm = BorderColorForm::Float; m_samplerParameters.borderColor = color; m_samplerParameters.borderColorI = IntVec4(static_cast(color.x()), static_cast(color.y()), @@ -169,8 +181,12 @@ namespace MobileGL { } void SamplerObject::SetBorderColorI(const IntVec4& color) { - if (color == m_samplerParameters.borderColorI) return; + if (color == m_samplerParameters.borderColorI && + m_samplerParameters.borderColorForm == BorderColorForm::Int) { + return; + } + m_samplerParameters.borderColorForm = BorderColorForm::Int; m_samplerParameters.borderColorI = color; m_samplerParameters.borderColorUI = UintVec4(static_cast(color.x()), static_cast(color.y()), @@ -182,8 +198,12 @@ namespace MobileGL { } void SamplerObject::SetBorderColorUI(const UintVec4& color) { - if (color == m_samplerParameters.borderColorUI) return; + if (color == m_samplerParameters.borderColorUI && + m_samplerParameters.borderColorForm == BorderColorForm::Uint) { + return; + } + m_samplerParameters.borderColorForm = BorderColorForm::Uint; m_samplerParameters.borderColorUI = color; m_samplerParameters.borderColorI = IntVec4(static_cast(color.x()), static_cast(color.y()), @@ -206,6 +226,10 @@ namespace MobileGL { return m_samplerParameters.borderColorUI; } + BorderColorForm SamplerObject::GetBorderColorForm() const { + return m_samplerParameters.borderColorForm; + } + SamplerCompareMode SamplerObject::GetCompareMode() const { return m_samplerParameters.compareMode; } diff --git a/MobileGL/MG_State/GLState/SamplerState/SamplerObject.h b/MobileGL/MG_State/GLState/SamplerState/SamplerObject.h index 1538dda2..9f97b2d6 100644 --- a/MobileGL/MG_State/GLState/SamplerState/SamplerObject.h +++ b/MobileGL/MG_State/GLState/SamplerState/SamplerObject.h @@ -56,6 +56,19 @@ namespace MobileGL { Unknown = -1 }; + // Which of the three GL_TEXTURE_BORDER_COLOR entry-point families last wrote the border colour, + // and therefore which of the three stored representations is AUTHORITATIVE. GL 4.6 core 8.10: + // TexParameterIiv/Iuiv store an integer border colour "unmodified, with an internal data type of + // integer", TexParameterfv stores a floating-point one, and the derived forms are only a + // convenience for a getter of the other spelling. A backend cannot pick the right driver entry + // point (glSamplerParameterIiv vs fv) or the right VkBorderColor family without this: numerically + // the three representations are always populated, so the value alone says nothing about the form. + enum class BorderColorForm : Uint8 { + Float, + Int, + Uint + }; + struct SamplerParameters { SamplerWrapMode wrapS = SamplerWrapMode::Repeat; SamplerWrapMode wrapT = SamplerWrapMode::Repeat; @@ -79,6 +92,7 @@ namespace MobileGL { FloatVec4 borderColor = {0.0f, 0.0f, 0.0f, 0.0f}; IntVec4 borderColorI = {0, 0, 0, 0}; UintVec4 borderColorUI = {0, 0, 0, 0}; + BorderColorForm borderColorForm = BorderColorForm::Float; }; namespace MG_State { @@ -117,6 +131,7 @@ namespace MobileGL { const FloatVec4& GetBorderColor() const; const IntVec4& GetBorderColorI() const; const UintVec4& GetBorderColorUI() const; + BorderColorForm GetBorderColorForm() const; Uint GetExternalIndex() const; Uint16 GetVersion() const; // Globally-unique, never-reused id for this sampler object's lifetime. Lets a diff --git a/MobileGL/MG_State/GLState/TextureState/TextureObject.cpp b/MobileGL/MG_State/GLState/TextureState/TextureObject.cpp index 7da4e400..039e301e 100644 --- a/MobileGL/MG_State/GLState/TextureState/TextureObject.cpp +++ b/MobileGL/MG_State/GLState/TextureState/TextureObject.cpp @@ -113,8 +113,14 @@ namespace MobileGL { return m_sampler->GetBorderColor(); } + // The redundancy filters test the FORM as well as the value: the derived representations + // make a float (0,0,0,1) and an integer (0,0,0,1) numerically identical, but they are + // different GL state and the DirectGLES sync memoises on m_textureParamsVersion. void TextureObjectBase::SetBorderColor(const FloatVec4& color) { - if (color == m_sampler->GetBorderColor()) return; + if (color == m_sampler->GetBorderColor() && + m_sampler->GetBorderColorForm() == BorderColorForm::Float) { + return; + } m_sampler->SetBorderColor(color); ++m_textureParamsVersion; @@ -125,7 +131,10 @@ namespace MobileGL { } void TextureObjectBase::SetBorderColorI(const IntVec4& color) { - if (color == m_sampler->GetBorderColorI()) return; + if (color == m_sampler->GetBorderColorI() && + m_sampler->GetBorderColorForm() == BorderColorForm::Int) { + return; + } m_sampler->SetBorderColorI(color); ++m_textureParamsVersion; @@ -136,12 +145,19 @@ namespace MobileGL { } void TextureObjectBase::SetBorderColorUI(const UintVec4& color) { - if (color == m_sampler->GetBorderColorUI()) return; + if (color == m_sampler->GetBorderColorUI() && + m_sampler->GetBorderColorForm() == BorderColorForm::Uint) { + return; + } m_sampler->SetBorderColorUI(color); ++m_textureParamsVersion; } + BorderColorForm TextureObjectBase::GetBorderColorForm() const { + return m_sampler->GetBorderColorForm(); + } + TextureSwizzleParam TextureObjectBase::GetSwizzleParam(TextureSwizzleParam param) const { switch (param) { case TextureSwizzleParam::Red: diff --git a/MobileGL/MG_State/GLState/TextureState/TextureObject.h b/MobileGL/MG_State/GLState/TextureState/TextureObject.h index ddae84a5..48ef2777 100644 --- a/MobileGL/MG_State/GLState/TextureState/TextureObject.h +++ b/MobileGL/MG_State/GLState/TextureState/TextureObject.h @@ -40,6 +40,8 @@ namespace MobileGL::MG_State::GLState { virtual void SetBorderColorI(const IntVec4& color) = 0; virtual const UintVec4& GetBorderColorUI() const = 0; virtual void SetBorderColorUI(const UintVec4& color) = 0; + // Which of the three setters above last ran; see SamplerParameters::borderColorForm. + virtual BorderColorForm GetBorderColorForm() const = 0; virtual TextureSwizzleParam GetSwizzleParam(TextureSwizzleParam param) const = 0; virtual void SetSwizzleParam(TextureSwizzleParam param, TextureSwizzleParam value) = 0; virtual void SetSwizzleParamRGBA(const Vec4& values) = 0; @@ -129,6 +131,7 @@ namespace MobileGL::MG_State::GLState { void SetBorderColorI(const IntVec4& color) override; const UintVec4& GetBorderColorUI() const override; void SetBorderColorUI(const UintVec4& color) override; + BorderColorForm GetBorderColorForm() const override; TextureSwizzleParam GetSwizzleParam(TextureSwizzleParam param) const override; const Vec4& GetAllSwizzleParams() const override; void SetSwizzleParam(TextureSwizzleParam param, TextureSwizzleParam value) override; diff --git a/MobileGL/MG_Util/Math/FixedPointConversion.h b/MobileGL/MG_Util/Math/FixedPointConversion.h new file mode 100644 index 00000000..7da5110e --- /dev/null +++ b/MobileGL/MG_Util/Math/FixedPointConversion.h @@ -0,0 +1,49 @@ +// MobileGL - MobileGL/MG_Util/Math/FixedPointConversion.h +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#pragma once +#include + +#include +#include + +namespace MobileGL::MG_Util { + // GL 4.6 core 2.3.5 "Fixed-Point Data Conversions", for the 32-bit signed normalized pair that + // GL_TEXTURE_BORDER_COLOR is specified and queried in when the NON-"I" integer entry points are + // used (glTexParameteriv / glSamplerParameteriv / glGetTexParameteriv / glGetSamplerParameteriv). + // The "I" entry points (TexParameterIiv / Iuiv) carry a raw integer border colour instead and + // must NOT go through these. + // + // The two directions have to be an exact pair or a legal round trip is destroyed: the CTS writes + // {0,1,2,4} with glTexParameteriv and demands {0,1,2,4} back from glGetTexParameteriv. Reading + // with a bare static_cast (which is what the truncating read used to do) answers {0,0,0,0} + // because equation 2.2 has already scaled those integers down to ~1e-9. + // + // b = 32, so the scale is 2^31 - 1 = 2147483647. It is held in DOUBLE deliberately: as a binary32 + // it rounds up to 2^31, and the inverse direction would then answer -2147483648 for f = -1.0 + // where the equation says -2147483647. The forward direction is unaffected either way (a small + // integer divided by 2147483647 lands on the same float as one divided by 2^31), so one exact + // constant serves both and the pair stays a true inverse: c -> c/(2^31-1) -> c. + inline constexpr double kSignedNormalizedInt32Scale = 2147483647.0; + + // Equation 2.2: c / (2^(b-1) - 1), clamped below at -1 so the extra negative code (-2^31) does + // not produce a value outside [-1, 1]. + inline Float SignedNormalizedInt32ToFloat(Int32 value) { + return std::max(static_cast(static_cast(value) / kSignedNormalizedInt32Scale), -1.0f); + } + + // Equation 2.3: round(f * (2^(b-1) - 1)). f is clamped to [-1, 1] first, as the equation's domain + // requires; the multiply is done in double so a near-1 float cannot round past INT32_MAX before + // the cast, which is undefined behaviour rather than a saturating one. + inline Int32 FloatToSignedNormalizedInt32(Float value) { + if (std::isnan(value)) return 0; + const Float clamped = std::clamp(value, -1.0f, 1.0f); + const double scaled = std::round(static_cast(clamped) * kSignedNormalizedInt32Scale); + return static_cast(std::clamp(scaled, -2147483648.0, 2147483647.0)); + } +} // namespace MobileGL::MG_Util