From cfc02599e54e02337dd56bb61f70b2e61eb9ce8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 21 Apr 2022 00:14:36 +0200 Subject: [PATCH] Trade: drop {MeshAttribute,SceneField}::Custom. I realized those are too annoying when writing a glTF exporter which contains a lot of switches over enums. And as further shown by the diff, those were only inflicting additional pain in *all* switch statements, nothing else, no other added value. And everywhere else the helpers are the designated way to deal with those, so there's no point in having an explicit enum value denoting start of a "custom range". It wasn't even any convenient to have it in the enum, as the extra effort needed for casting actually made it *exactly* the same length as if I'd just use a separately-defined constant. --- src/Magnum/MeshTools/Compile.cpp | 1 - src/Magnum/MeshTools/RemoveDuplicates.cpp | 1 - src/Magnum/SceneTools/sceneconverter.cpp | 11 ++--- src/Magnum/Trade/MeshData.cpp | 3 -- src/Magnum/Trade/MeshData.h | 48 ++++++++++---------- src/Magnum/Trade/SceneData.cpp | 3 -- src/Magnum/Trade/SceneData.h | 53 ++++++++++++----------- src/Magnum/Trade/Test/MeshDataTest.cpp | 4 +- src/Magnum/Trade/Test/SceneDataTest.cpp | 4 +- 9 files changed, 61 insertions(+), 67 deletions(-) diff --git a/src/Magnum/MeshTools/Compile.cpp b/src/Magnum/MeshTools/Compile.cpp index 0f3e422bf..5f16c789c 100644 --- a/src/Magnum/MeshTools/Compile.cpp +++ b/src/Magnum/MeshTools/Compile.cpp @@ -125,7 +125,6 @@ GL::Mesh compileInternal(const Trade::MeshData& meshData, GL::Buffer&& indices, #ifdef MAGNUM_TARGET_GLES2 case Trade::MeshAttribute::ObjectId: #endif - case Trade::MeshAttribute::Custom: break; /* LCOV_EXCL_STOP */ } diff --git a/src/Magnum/MeshTools/RemoveDuplicates.cpp b/src/Magnum/MeshTools/RemoveDuplicates.cpp index 4bff989e6..3d0ab3f3c 100644 --- a/src/Magnum/MeshTools/RemoveDuplicates.cpp +++ b/src/Magnum/MeshTools/RemoveDuplicates.cpp @@ -533,7 +533,6 @@ Trade::MeshData removeDuplicatesFuzzy(const Trade::MeshData& data, const Float f /* These have unbounded range. Do nothing but enumerate all these here to silence warnings about unused enum values. */ case Trade::MeshAttribute::Position: - case Trade::MeshAttribute::Custom: break; /* These shouldn't be floating point */ diff --git a/src/Magnum/SceneTools/sceneconverter.cpp b/src/Magnum/SceneTools/sceneconverter.cpp index 6a87e99f2..ee0d75559 100644 --- a/src/Magnum/SceneTools/sceneconverter.cpp +++ b/src/Magnum/SceneTools/sceneconverter.cpp @@ -680,8 +680,9 @@ is specified as well, the IDs reference attributes of the first mesh.)") for(UnsignedInt k = 0; k != mesh->attributeCount(); ++k) { const Trade::MeshAttribute name = mesh->attributeName(k); - /* Calculate bounds, if requested and this is not an - implementation-specific format */ + /* Calculate bounds, if requested, if this is not an + implementation-specific format and if it's not a custom + attribute */ Containers::String bounds; if(args.isSet("bounds") && !isVertexFormatImplementationSpecific(mesh->attributeFormat(k))) switch(name) { case Trade::MeshAttribute::Position: @@ -706,12 +707,6 @@ is specified as well, the IDs reference attributes of the first mesh.)") Debug{} << mesh->objectIdsAsArray(namedAttributeId(*mesh, k)); bounds = calculateBounds(mesh->objectIdsAsArray(namedAttributeId(*mesh, k))); break; - - /* And also all other custom attribs. Not saying - default: here so we get notified when we forget to - handle newly added attribute names */ - case Trade::MeshAttribute::Custom: - break; } arrayAppend(info.attributes, InPlaceInit, diff --git a/src/Magnum/Trade/MeshData.cpp b/src/Magnum/Trade/MeshData.cpp index af709a187..49dbd7ab9 100644 --- a/src/Magnum/Trade/MeshData.cpp +++ b/src/Magnum/Trade/MeshData.cpp @@ -899,9 +899,6 @@ Debug& operator<<(Debug& debug, const MeshAttribute value) { _c(ObjectId) #undef _c /* LCOV_EXCL_STOP */ - - /* To silence compiler warning about unhandled values */ - case MeshAttribute::Custom: CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ } return debug << (packed ? "" : "(") << Debug::nospace << reinterpret_cast(UnsignedShort(value)) << Debug::nospace << (packed ? "" : ")"); diff --git a/src/Magnum/Trade/MeshData.h b/src/Magnum/Trade/MeshData.h index a89538715..3477984ae 100644 --- a/src/Magnum/Trade/MeshData.h +++ b/src/Magnum/Trade/MeshData.h @@ -40,13 +40,26 @@ namespace Magnum { namespace Trade { +namespace Implementation { + enum: UnsignedShort { MeshAttributeCustom = 32768 }; +} + /** @brief Mesh attribute name @m_since{2020,06} -@see @ref MeshData, @ref MeshAttributeData, @ref VertexFormat, - @ref AbstractImporter::meshAttributeForName(), - @ref AbstractImporter::meshAttributeName() +See @ref MeshData for more information. + +Apart from the builtin attribute names it's possible to have custom ones, +which use the upper 15 bits of the enum range. Those are detected via +@ref isMeshAttributeCustom() and can be converted to and from a numeric +identifier using @ref meshAttributeCustom(MeshAttribute) and +@ref meshAttributeCustom(UnsignedShort). Unlike the builtin ones, these can +be of any type. An importer that exposes custom attributes then may also +provide a string mapping using @ref AbstractImporter::meshAttributeForName() +and @ref AbstractImporter::meshAttributeName(). See documentation of a +particular importer for details. +@see @ref MeshAttributeData, @ref VertexFormat */ /* 16 bits because 8 bits is not enough to cover all potential per-edge, per-face, per-instance and per-meshlet attributes */ @@ -145,16 +158,7 @@ enum class MeshAttribute: UnsignedShort { * Corresponds to @ref Shaders::GenericGL::ObjectId. * @see @ref MeshData::objectIdsAsArray() */ - ObjectId, - - /** - * This and all higher values are for importer-specific attributes. Can be - * of any type. See documentation of a particular importer for details. - * @see @ref isMeshAttributeCustom(), - * @ref meshAttributeCustom(MeshAttribute), - * @ref meshAttributeCustom(UnsignedShort) - */ - Custom = 32768 + ObjectId }; /** @@ -167,30 +171,30 @@ MAGNUM_TRADE_EXPORT Debug& operator<<(Debug& debug, MeshAttribute value); @brief Whether a mesh attribute is custom @m_since{2020,06} -Returns @cpp true @ce if @p name has a value larger or equal to -@ref MeshAttribute::Custom, @cpp false @ce otherwise. +Returns @cpp true @ce if @p name has a value in the upper 15 bits of the enum +range, @cpp false @ce otherwise. @see @ref meshAttributeCustom(UnsignedShort), @ref meshAttributeCustom(MeshAttribute), @ref AbstractImporter::meshAttributeName() */ constexpr bool isMeshAttributeCustom(MeshAttribute name) { - return UnsignedShort(name) >= UnsignedShort(MeshAttribute::Custom); + return UnsignedShort(name) >= Implementation::MeshAttributeCustom; } /** @brief Create a custom mesh attribute @m_since{2020,06} -Returns a custom mesh attribute with index @p id. The index is expected to be -less than the value of @ref MeshAttribute::Custom. Use -@ref meshAttributeCustom(MeshAttribute) to get the index back. +Returns a custom mesh attribute with index @p id. The index is expected to fit +into 15 bits. Use @ref meshAttributeCustom(MeshAttribute) to get the index +back. */ /* Constexpr so it's usable for creating compile-time MeshAttributeData instances */ constexpr MeshAttribute meshAttributeCustom(UnsignedShort id) { - return CORRADE_CONSTEXPR_ASSERT(id < UnsignedShort(MeshAttribute::Custom), + return CORRADE_CONSTEXPR_ASSERT(id < Implementation::MeshAttributeCustom, "Trade::meshAttributeCustom(): index" << id << "too large"), - MeshAttribute(UnsignedShort(MeshAttribute::Custom) + id); + MeshAttribute(Implementation::MeshAttributeCustom + id); } /** @@ -204,7 +208,7 @@ is custom. constexpr UnsignedShort meshAttributeCustom(MeshAttribute name) { return CORRADE_CONSTEXPR_ASSERT(isMeshAttributeCustom(name), "Trade::meshAttributeCustom():" << name << "is not custom"), - UnsignedShort(name) - UnsignedShort(MeshAttribute::Custom); + UnsignedShort(name) - Implementation::MeshAttributeCustom; } /** diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index 602d3a976..c86a70159 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -131,9 +131,6 @@ Debug& operator<<(Debug& debug, const SceneField value) { _c(ImporterState) #undef _c /* LCOV_EXCL_STOP */ - - /* To silence compiler warning about unhandled values */ - case SceneField::Custom: CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ } return debug << (packed ? "" : "(") << Debug::nospace << reinterpret_cast(UnsignedInt(value)) << Debug::nospace << (packed ? "" : ")"); diff --git a/src/Magnum/Trade/SceneData.h b/src/Magnum/Trade/SceneData.h index cbbdb8f4d..ff586f7a6 100644 --- a/src/Magnum/Trade/SceneData.h +++ b/src/Magnum/Trade/SceneData.h @@ -85,14 +85,30 @@ Returns the same value as @ref sceneMappingTypeSize(). */ MAGNUM_TRADE_EXPORT UnsignedInt sceneMappingTypeAlignment(SceneMappingType type); +namespace Implementation { + enum: UnsignedInt { SceneFieldCustom = 0x80000000u }; +} + /** @brief Scene field name @m_since_latest See @ref SceneData for more information. -@see @ref SceneFieldData, @ref SceneFieldType, - @ref AbstractImporter::sceneFieldForName(), - @ref AbstractImporter::sceneFieldName() + +Apart from the builtin field names it's possible to have custom ones, which use +the upper 31 bits of the enum range. While it's unlikely to have billions of +custom fields, the enum intentionally reserves a full 31-bit range to avoid the +need to remap field identifiers coming from 3rd party ECS frameworks, for +example. + +Custom fields are detected via @ref isSceneFieldCustom() and can be converted +to and from a numeric identifier using @ref sceneFieldCustom(SceneField) and +@ref sceneFieldCustom(UnsignedInt). Unlike the builtin ones, these can be of +any type. An importer that exposes custom fields then may also provide a string +mapping using @ref AbstractImporter::sceneFieldForName() and +@ref AbstractImporter::sceneFieldName(). See documentation of a particular +importer for details. +@see @ref SceneFieldData, @ref SceneFieldType */ enum class SceneField: UnsignedInt { /* Zero used for an invalid value */ @@ -303,19 +319,7 @@ enum class SceneField: UnsignedInt { * @see @ref SceneData::importerStateAsArray(), * @ref SceneData::importerStateFor() */ - ImporterState, - - /** - * This and all higher values are for importer-specific fields. Can be - * of any type. See documentation of a particular importer for details. - * - * While it's unlikely to have billions of custom fields, the enum - * intentionally reserves a full 31-bit range to avoid the need to remap - * field identifiers coming from 3rd party ECS frameworks, for example. - * @see @ref isSceneFieldCustom(), @ref sceneFieldCustom(SceneField), - * @ref sceneFieldCustom(UnsignedInt) - */ - Custom = 0x80000000u + ImporterState }; /** @@ -328,29 +332,28 @@ MAGNUM_TRADE_EXPORT Debug& operator<<(Debug& debug, SceneField value); @brief Whether a scene field is custom @m_since_latest -Returns @cpp true @ce if @p name has a value larger or equal to -@ref SceneField::Custom, @cpp false @ce otherwise. +Returns @cpp true @ce if @p name has a value in the upper 31 bits of the enum +range, @cpp false @ce otherwise. @see @ref sceneFieldCustom(UnsignedInt), @ref sceneFieldCustom(SceneField), @ref AbstractImporter::sceneFieldName() */ constexpr bool isSceneFieldCustom(SceneField name) { - return UnsignedInt(name) >= UnsignedInt(SceneField::Custom); + return UnsignedInt(name) >= Implementation::SceneFieldCustom; } /** @brief Create a custom scene field @m_since_latest -Returns a custom scene field with index @p id. The index is expected to be less -than the value of @ref SceneField::Custom. Use @ref sceneFieldCustom(SceneField) -to get the index back. +Returns a custom scene field with index @p id. The index is expected to fit +into 31 bits. Use @ref sceneFieldCustom(SceneField) to get the index back. */ /* Constexpr so it's usable for creating compile-time SceneFieldData instances */ constexpr SceneField sceneFieldCustom(UnsignedInt id) { - return CORRADE_CONSTEXPR_ASSERT(id < UnsignedInt(SceneField::Custom), + return CORRADE_CONSTEXPR_ASSERT(id < Implementation::SceneFieldCustom, "Trade::sceneFieldCustom(): index" << id << "too large"), - SceneField(UnsignedInt(SceneField::Custom) + id); + SceneField(Implementation::SceneFieldCustom + id); } /** @@ -364,7 +367,7 @@ custom. constexpr UnsignedInt sceneFieldCustom(SceneField name) { return CORRADE_CONSTEXPR_ASSERT(isSceneFieldCustom(name), "Trade::sceneFieldCustom():" << name << "is not custom"), - UnsignedInt(name) - UnsignedInt(SceneField::Custom); + UnsignedInt(name) - Implementation::SceneFieldCustom; } /** diff --git a/src/Magnum/Trade/Test/MeshDataTest.cpp b/src/Magnum/Trade/Test/MeshDataTest.cpp index 574165955..890f2401a 100644 --- a/src/Magnum/Trade/Test/MeshDataTest.cpp +++ b/src/Magnum/Trade/Test/MeshDataTest.cpp @@ -419,14 +419,14 @@ MeshDataTest::MeshDataTest() { void MeshDataTest::customAttributeName() { CORRADE_VERIFY(!isMeshAttributeCustom(MeshAttribute::Position)); CORRADE_VERIFY(!isMeshAttributeCustom(MeshAttribute(32767))); - CORRADE_VERIFY(isMeshAttributeCustom(MeshAttribute::Custom)); + CORRADE_VERIFY(isMeshAttributeCustom(MeshAttribute(Implementation::MeshAttributeCustom))); CORRADE_VERIFY(isMeshAttributeCustom(MeshAttribute(65535))); CORRADE_COMPARE(UnsignedShort(meshAttributeCustom(0)), 32768); CORRADE_COMPARE(UnsignedShort(meshAttributeCustom(8290)), 41058); CORRADE_COMPARE(UnsignedShort(meshAttributeCustom(32767)), 65535); - CORRADE_COMPARE(meshAttributeCustom(MeshAttribute::Custom), 0); + CORRADE_COMPARE(meshAttributeCustom(MeshAttribute(Implementation::MeshAttributeCustom)), 0); CORRADE_COMPARE(meshAttributeCustom(MeshAttribute(41058)), 8290); CORRADE_COMPARE(meshAttributeCustom(MeshAttribute(65535)), 32767); diff --git a/src/Magnum/Trade/Test/SceneDataTest.cpp b/src/Magnum/Trade/Test/SceneDataTest.cpp index 24f245c2d..c6180a1aa 100644 --- a/src/Magnum/Trade/Test/SceneDataTest.cpp +++ b/src/Magnum/Trade/Test/SceneDataTest.cpp @@ -625,14 +625,14 @@ void SceneDataTest::debugMappingTypePacked() { void SceneDataTest::customFieldName() { CORRADE_VERIFY(!isSceneFieldCustom(SceneField::Rotation)); CORRADE_VERIFY(!isSceneFieldCustom(SceneField(0x0fffffffu))); - CORRADE_VERIFY(isSceneFieldCustom(SceneField::Custom)); + CORRADE_VERIFY(isSceneFieldCustom(SceneField(Implementation::SceneFieldCustom))); CORRADE_VERIFY(isSceneFieldCustom(SceneField(0x80000000u))); CORRADE_COMPARE(UnsignedInt(sceneFieldCustom(0)), 0x80000000u); CORRADE_COMPARE(UnsignedInt(sceneFieldCustom(0xabcd)), 0x8000abcdu); CORRADE_COMPARE(UnsignedInt(sceneFieldCustom(0x7fffffff)), 0xffffffffu); - CORRADE_COMPARE(sceneFieldCustom(SceneField::Custom), 0); + CORRADE_COMPARE(sceneFieldCustom(SceneField(Implementation::SceneFieldCustom)), 0); CORRADE_COMPARE(sceneFieldCustom(SceneField(0x8000abcdu)), 0xabcd); CORRADE_COMPARE(sceneFieldCustom(SceneField(0xffffffffu)), 0x7fffffffu);