diff --git a/src/Magnum/SceneTools/Combine.h b/src/Magnum/SceneTools/Combine.h index 390c8dc24..24a0bf1f3 100644 --- a/src/Magnum/SceneTools/Combine.h +++ b/src/Magnum/SceneTools/Combine.h @@ -53,8 +53,10 @@ Fields pointing to existing memory are copied to the output, fields with (sized) @cpp nullptr @ce mapping or data views are treated as placeholders for copying the data later, with memory left uninitialized. If you however want to have placeholder mapping data shared among multiple fields you have to allocate -them upfront. Note that offset-only @ref Trade::SceneFieldData instances are -not supported in the @p fields array. +them upfront. Fields with a string @ref Trade::SceneFieldType can't have +placeholder data views or @cpp nullptr @ce string data pointers, as they're +used to calculate the total string data size. Note that offset-only +@ref Trade::SceneFieldData instances are not supported in the @p fields array. The resulting fields are always tightly packed (not interleaved). Returned data flags have both @ref Trade::DataFlag::Mutable and @ref Trade::DataFlag::Owned, diff --git a/src/Magnum/SceneTools/Implementation/combine.h b/src/Magnum/SceneTools/Implementation/combine.h index 4137fea58..5b2953cf2 100644 --- a/src/Magnum/SceneTools/Implementation/combine.h +++ b/src/Magnum/SceneTools/Implementation/combine.h @@ -29,8 +29,9 @@ #include #include #include -#include #include +#include +#include #include #include "Magnum/Math/Functions.h" @@ -52,9 +53,10 @@ union CombineItemView { Containers::StridedArrayView2D types; Containers::MutableStridedBitArrayView2D bits; + Containers::MutableStringView strings; }; -template void combineCopyMappings(const Containers::ArrayView fields, const Containers::ArrayView itemViews, const Containers::ArrayView> itemViewMappings) { +template void combineCopyMappings(const Containers::ArrayView fields, const Containers::ArrayView itemViews, const Containers::ArrayView> itemViewMappings) { std::size_t latestMapping = 0; for(std::size_t i = 0; i != fields.size(); ++i) { /* If there are no shared object mappings, itemViewMappings should be @@ -85,6 +87,29 @@ template void combineCopyMappings(const Containers::ArrayView std::size_t stringOffsetFieldSize(const Containers::StridedArrayView1D& field) { + return Containers::arrayCast(field).back(); +} +/* Ranges have the total string size as the max "end" of all offset+size + pairs. Again, the null terminator is included in the size so no special + handling needed. */ +template std::size_t stringRangeFieldSize(const Containers::StridedArrayView1D& field) { + std::size_t max = 0; + for(const Containers::Pair i: Containers::arrayCast>(field)) + max = Math::max(std::size_t(i.first() + i.second()), max); + return max; +} +/* Null-terminated ranges have the size implicitly calculated using strlen, + returning + 1 as it needs to include the last null terminator as well. */ +template std::size_t stringRangeNullTerminatedFieldSize(const char* string, const Containers::StridedArrayView1D& field) { + std::size_t max = 0; + for(const T i: Containers::arrayCast(field)) + max = i + Math::max(std::strlen(string + i), max); + return max + 1; +} + inline Trade::SceneData combineFields(const Trade::SceneMappingType mappingType, const UnsignedLong mappingBound, const Containers::ArrayView fields) { const std::size_t mappingTypeSize = sceneMappingTypeSize(mappingType); const std::size_t mappingTypeAlignment = sceneMappingTypeAlignment(mappingType); @@ -96,15 +121,18 @@ inline Trade::SceneData combineFields(const Trade::SceneMappingType mappingType, NOT HAVE IT, UGH. */ std::map, UnsignedInt> uniqueMappings; Containers::Array items; - Containers::Array> itemViewMappings{NoInit, fields.size()}; - - /* The item views are referenced from ArrayTuple::Item, not using a - growable array in order to avoid an accidental reallocation. It's either - of the two views in the union based on whether it's a mapping or data - view and what the data field type is */ + Containers::Array> itemViewMappings{NoInit, fields.size()}; + + /* The item views are referenced from ArrayTuple::Item. It's either of the + three views in the union --- from the group of (up to) 3 views per + field, first is for the mapping (unless shared with another view) and is + always `types`, second for the data (either `types` or `bits`) and third + for the string data (`strings`, if the field is a string). In most cases + they array won't be fully used but we need to avoid accidental + reallocation so the array is made with an upper bound on size. */ /** @todo once never-reallocating allocators are present, use them instead of the manual offset */ - Containers::Array itemViews{fields.size()*2}; + Containers::Array itemViews{fields.size()*3}; std::size_t itemViewOffset = 0; /* Go through all fields and collect ArrayTuple allocations for these */ @@ -151,14 +179,65 @@ inline Trade::SceneData combineFields(const Trade::SceneMappingType mappingType, NoInit, Containers::Size2D{std::size_t(field.size()), field.fieldArraySize() ? field.fieldArraySize() : 1}, itemViews[itemViewOffset].bits); + ++itemViewOffset; } else { arrayAppend(items, InPlaceInit, NoInit, std::size_t(field.size()), sceneFieldTypeSize(fieldType)*(field.fieldArraySize() ? field.fieldArraySize() : 1), sceneFieldTypeAlignment(fieldType), itemViews[itemViewOffset].types); + ++itemViewOffset; + + /* For string fields we need to allocate also for the actual string + data. For space reasons the SceneFieldData stores only the data + pointer, size is implicit, so need to calculate it as the max of + end pointers of all strings */ + if(Trade::Implementation::isSceneFieldTypeString(fieldType)) { + const Containers::StridedArrayView1D fieldData = field.fieldData(); + CORRADE_ASSERT(!field.size() || fieldData.data(), + "SceneTools::combineFields(): string field" << i << "has a placeholder data", (Trade::SceneData{Trade::SceneMappingType::UnsignedInt, 0, nullptr, {}})); + + const char* const stringData = field.stringData(); + CORRADE_ASSERT(!field.size() || stringData, + "SceneTools::combineFields(): string field" << i << "has a placeholder string data", (Trade::SceneData{Trade::SceneMappingType::UnsignedInt, 0, nullptr, {}})); + + std::size_t size; + if(field.size() == 0) + size = 0; + else if(fieldType == Trade::SceneFieldType::StringOffset8) + size = stringOffsetFieldSize(fieldData); + else if(fieldType == Trade::SceneFieldType::StringOffset16) + size = stringOffsetFieldSize(fieldData); + else if(fieldType == Trade::SceneFieldType::StringOffset32) + size = stringOffsetFieldSize(fieldData); + else if(fieldType == Trade::SceneFieldType::StringOffset64) + size = stringOffsetFieldSize(fieldData); + else if(fieldType == Trade::SceneFieldType::StringRange8) + size = stringRangeFieldSize(fieldData); + else if(fieldType == Trade::SceneFieldType::StringRange16) + size = stringRangeFieldSize(fieldData); + else if(fieldType == Trade::SceneFieldType::StringRange32) + size = stringRangeFieldSize(fieldData); + else if(fieldType == Trade::SceneFieldType::StringRange64) + size = stringRangeFieldSize(fieldData); + else if(fieldType == Trade::SceneFieldType::StringRangeNullTerminated8) + size = stringRangeNullTerminatedFieldSize(stringData, fieldData); + else if(fieldType == Trade::SceneFieldType::StringRangeNullTerminated16) + size = stringRangeNullTerminatedFieldSize(stringData, fieldData); + else if(fieldType == Trade::SceneFieldType::StringRangeNullTerminated32) + size = stringRangeNullTerminatedFieldSize(stringData, fieldData); + else if(fieldType == Trade::SceneFieldType::StringRangeNullTerminated64) + size = stringRangeNullTerminatedFieldSize(stringData, fieldData); + else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ + + itemViewMappings[i].third() = itemViewOffset; + arrayAppend(items, InPlaceInit, + NoInit, + size, + itemViews[itemViewOffset].strings); + ++itemViewOffset; + } } - ++itemViewOffset; } CORRADE_INTERNAL_ASSERT(itemViewOffset <= itemViews.size()); @@ -207,6 +286,14 @@ inline Trade::SceneData combineFields(const Trade::SceneMappingType mappingType, /** @todo isn't there some less awful way to create a 2D view, sigh */ Utility::copy(Containers::arrayCast<2, const char>(src, sceneFieldTypeSize(fieldType)*(field.fieldArraySize() ? field.fieldArraySize() : 1)), itemViews[itemViewMappings[i].second()].types); + + /* If the field is a string, copy also the actual string data. The + size was calculated above and is recorded into the output + view. */ + if(Trade::Implementation::isSceneFieldTypeString(fieldType)) { + const Containers::MutableStringView dst = itemViews[itemViewMappings[i].third()].strings; + Utility::copy(Containers::arrayView(field.stringData(), dst.size()), dst); + } } } @@ -228,6 +315,12 @@ inline Trade::SceneData combineFields(const Trade::SceneMappingType mappingType, /** @todo creating a 1D view isn't really easy either, huh? */ itemViews[itemViewMappings[i].second()].bits.transposed<0, 1>()[0], field.flags()}; + } else if(Trade::Implementation::isSceneFieldTypeString(fieldType)) { + outFields[i] = Trade::SceneFieldData{field.name(), + itemViews[itemViewMappings[i].first()].types, + itemViews[itemViewMappings[i].third()].strings.data(), + fieldType, itemViews[itemViewMappings[i].second()].types, + field.flags()}; } else { outFields[i] = Trade::SceneFieldData{field.name(), itemViews[itemViewMappings[i].first()].types, diff --git a/src/Magnum/SceneTools/Test/CombineTest.cpp b/src/Magnum/SceneTools/Test/CombineTest.cpp index a0b14eea6..c6adc1cd1 100644 --- a/src/Magnum/SceneTools/Test/CombineTest.cpp +++ b/src/Magnum/SceneTools/Test/CombineTest.cpp @@ -24,7 +24,10 @@ */ #include +#include #include +#include +#include #include #include #include @@ -41,16 +44,20 @@ struct CombineTest: TestSuite::Tester { explicit CombineTest(); void fields(); + template void fieldsStrings(); void fieldsAlignment(); void fieldsMappingShared(); void fieldsMappingSharedPartial(); void fieldsMappingPlaceholderFieldPlaceholder(); void fieldsMappingSharedFieldPlaceholder(); + void fieldsStringPlaceholder(); void fieldsOffsetOnly(); void fieldsFromDataOffsetOnly(); }; +using namespace Containers::Literals; + const struct { const char* name; Trade::SceneMappingType objectType; @@ -65,12 +72,18 @@ CombineTest::CombineTest() { addInstancedTests({&CombineTest::fields}, Containers::arraySize(FieldsData)); - addTests({&CombineTest::fieldsAlignment, + addTests({&CombineTest::fieldsStrings, + &CombineTest::fieldsStrings, + &CombineTest::fieldsStrings, + &CombineTest::fieldsStrings, + + &CombineTest::fieldsAlignment, &CombineTest::fieldsMappingShared, &CombineTest::fieldsMappingSharedPartial, &CombineTest::fieldsMappingPlaceholderFieldPlaceholder, &CombineTest::fieldsMappingSharedFieldPlaceholder, + &CombineTest::fieldsStringPlaceholder, &CombineTest::fieldsOffsetOnly, &CombineTest::fieldsFromDataOffsetOnly}); } @@ -263,6 +276,220 @@ void CombineTest::fields() { TestSuite::Compare::Container); } +/* Taken from SceneDataTest */ +template struct StringFieldTraits; +template<> struct StringFieldTraits { + static const char* name() { return "8"; } + static Trade::SceneFieldType offsetType() { return Trade::SceneFieldType::StringOffset8; } + static Trade::SceneFieldType rangeType() { return Trade::SceneFieldType::StringRange8; } + static Trade::SceneFieldType rangeNullTerminatedType() { + return Trade::SceneFieldType::StringRangeNullTerminated8; + } +}; +template<> struct StringFieldTraits { + static const char* name() { return "16"; } + static Trade::SceneFieldType offsetType() { return Trade::SceneFieldType::StringOffset16; } + static Trade::SceneFieldType rangeType() { return Trade::SceneFieldType::StringRange16; } + static Trade::SceneFieldType rangeNullTerminatedType() { + return Trade::SceneFieldType::StringRangeNullTerminated16; + } +}; +template<> struct StringFieldTraits { + static const char* name() { return "32"; } + static Trade::SceneFieldType offsetType() { return Trade::SceneFieldType::StringOffset32; } + static Trade::SceneFieldType rangeType() { return Trade::SceneFieldType::StringRange32; } + static Trade::SceneFieldType rangeNullTerminatedType() { + return Trade::SceneFieldType::StringRangeNullTerminated32; + } +}; +template<> struct StringFieldTraits { + static const char* name() { return "64"; } + static Trade::SceneFieldType offsetType() { return Trade::SceneFieldType::StringOffset64; } + static Trade::SceneFieldType rangeType() { return Trade::SceneFieldType::StringRange64; } + static Trade::SceneFieldType rangeNullTerminatedType() { + return Trade::SceneFieldType::StringRangeNullTerminated64; + } +}; + +template void CombineTest::fieldsStrings() { + setTestCaseTemplateName(StringFieldTraits::name()); + + /* Null-terminated ranges */ + Containers::StringView tagStrings = + "SOFT\0" /* 0 */ + "mouldy!"_s; /* 5, assumes it's stored null-terminated */ + /* With null termination it's 13 bytes. If only 12 would be copied, the + next ArrayTuple item (likely Name::mapping) would get aligned right + after, failing the null terminator check */ + CORRADE_COMPARE(tagStrings.size(), 12); + + const struct Tag { + UnsignedByte mapping; + T rangeNullTerminated; + } tagsData[]{ + {3, 0}, + {7, 5}, + {7, 0}, + {1, 0} + }; + auto tags = Containers::stridedArrayView(tagsData); + + /* Non-null-terminated offsets */ + Containers::StringView nameStrings = + "Chair" /* 5 */ + "Lampshade" /* 14 */ + "Sofa37"_s; /* 20 */ + CORRADE_COMPARE(nameStrings.size(), 20); + + const struct Name { + UnsignedByte mapping; + T offset; + } namesData[]{ + {3, 5}, + {7, 14}, + {1, 20} + }; + auto names = Containers::stridedArrayView(namesData); + + /* Null-terminated offsets */ + Containers::StringView keyStrings = + "color\0" /* 6 */ + "age\0" /* 10 */ + "age"_s; /* 14, assumes it's stored null-terminated */ + + const struct Key { + UnsignedByte mapping; + T offsetNullTerminated; + } keysData[]{ + {11, 6}, + {3, 10}, + {12, 14} + }; + auto keys = Containers::stridedArrayView(keysData); + + Containers::StringView valueStrings = + "light\0brown" /* 0, 11 */ + "ancient" /* 11, 7 */ + "new"_s; /* 18, 3 */ + + /* Non-null-terminated ranges */ + const struct Value { + UnsignedByte mapping; + Containers::Pair range; + } valuesData[]{ + {3, {18, 3}}, + {12, {11, 7}}, + {7, {18, 3}}, + {11, {0, 11}} + }; + auto values = Containers::stridedArrayView(valuesData); + + /* Using just 8-bit mapping to not have any extra padding between things + and thus better catch accidentally forgotten null termination and + such */ + Trade::SceneData scene = combineFields(Trade::SceneMappingType::UnsignedByte, 167, { + Trade::SceneFieldData{Trade::sceneFieldCustom(0), + tags.slice(&Tag::mapping), + tagStrings.data(), StringFieldTraits::rangeNullTerminatedType(), + tags.slice(&Tag::rangeNullTerminated)}, + Trade::SceneFieldData{Trade::sceneFieldCustom(1), + names.slice(&Name::mapping), + nameStrings.data(), StringFieldTraits::offsetType(), + names.slice(&Name::offset)}, + Trade::SceneFieldData{Trade::sceneFieldCustom(2), + keys.slice(&Key::mapping), + keyStrings.data(), StringFieldTraits::offsetType(), + keys.slice(&Key::offsetNullTerminated), + Trade::SceneFieldFlag::NullTerminatedString}, + Trade::SceneFieldData{Trade::sceneFieldCustom(3), + values.slice(&Value::mapping), + valueStrings.data(), StringFieldTraits::rangeType(), + values.slice(&Value::range)}, + /* Empty string field, shouldn't crash or anything */ + Trade::SceneFieldData{Trade::sceneFieldCustom(4), + Containers::ArrayView{}, + nullptr, StringFieldTraits::offsetType(), + Containers::ArrayView{}}, + }); + + CORRADE_COMPARE(scene.fieldName(0), Trade::sceneFieldCustom(0)); + CORRADE_COMPARE(scene.fieldFlags(0), Trade::SceneFieldFlag::NullTerminatedString); + CORRADE_COMPARE(scene.fieldType(0), StringFieldTraits::rangeNullTerminatedType()); + CORRADE_COMPARE_AS(scene.mapping(0), + tags.slice(&Tag::mapping), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.field(0), + tags.slice(&Tag::rangeNullTerminated), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.fieldStrings(0), + (Containers::StringIterable{"SOFT", "mouldy!", "SOFT", "SOFT"}), + TestSuite::Compare::Container); + /* All should stay null-terminated -- i.e., the null terminator included in + the size calculation when the string gets copied */ + for(Containers::StringView i: scene.fieldStrings(0)) { + CORRADE_COMPARE(i.flags(), Containers::StringViewFlag::NullTerminated); + CORRADE_COMPARE(i[i.size()], '\0'); + } + + CORRADE_COMPARE(scene.fieldName(1), Trade::sceneFieldCustom(1)); + CORRADE_COMPARE(scene.fieldFlags(1), Trade::SceneFieldFlags{}); + CORRADE_COMPARE(scene.fieldType(1), StringFieldTraits::offsetType()); + CORRADE_COMPARE_AS(scene.mapping(1), + names.slice(&Name::mapping), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.field(1), + names.slice(&Name::offset), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.fieldStrings(1), + (Containers::StringIterable{"Chair", "Lampshade", "Sofa37"}), + TestSuite::Compare::Container); + + CORRADE_COMPARE(scene.fieldName(2), Trade::sceneFieldCustom(2)); + CORRADE_COMPARE(scene.fieldFlags(2), Trade::SceneFieldFlag::NullTerminatedString); + CORRADE_COMPARE(scene.fieldType(2), StringFieldTraits::offsetType()); + CORRADE_COMPARE_AS(scene.mapping(2), + keys.slice(&Key::mapping), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.field(2), + keys.slice(&Key::offsetNullTerminated), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.fieldStrings(2), + (Containers::StringIterable{"color", "age", "age"}), + TestSuite::Compare::Container); + /* All should stay null-terminated -- i.e., the null terminator included in + the size calculation when the string gets copied */ + for(Containers::StringView i: scene.fieldStrings(2)) { + CORRADE_COMPARE(i.flags(), Containers::StringViewFlag::NullTerminated); + CORRADE_COMPARE(i[i.size()], '\0'); + } + + CORRADE_COMPARE(scene.fieldName(3), Trade::sceneFieldCustom(3)); + CORRADE_COMPARE(scene.fieldFlags(3), Trade::SceneFieldFlags{}); + CORRADE_COMPARE(scene.fieldType(3), StringFieldTraits::rangeType()); + CORRADE_COMPARE_AS(scene.mapping(3), + values.slice(&Value::mapping), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS((scene.field>(3)), + values.slice(&Value::range), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.fieldStrings(3), + (Containers::StringIterable{"new", "ancient", "new", "light\0brown"_s}), + TestSuite::Compare::Container); + + CORRADE_COMPARE(scene.fieldName(4), Trade::sceneFieldCustom(4)); + CORRADE_COMPARE(scene.fieldFlags(4), Trade::SceneFieldFlags{}); + CORRADE_COMPARE(scene.fieldType(4), StringFieldTraits::offsetType()); + CORRADE_COMPARE_AS(scene.mapping(4), + Containers::ArrayView{}, + TestSuite::Compare::Container); + CORRADE_COMPARE_AS((scene.field(4)), + Containers::ArrayView{}, + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.fieldStrings(4), + Containers::StringIterable{}, + TestSuite::Compare::Container); +} + void CombineTest::fieldsAlignment() { const UnsignedShort meshMappingData[]{15, 23, 47}; const UnsignedByte meshFieldData[]{0, 1, 2}; @@ -520,6 +747,43 @@ void CombineTest::fieldsMappingSharedFieldPlaceholder() { CORRADE_COMPARE(scene.field(Trade::SceneField::MeshMaterial).stride()[0], 4); } +void CombineTest::fieldsStringPlaceholder() { + CORRADE_SKIP_IF_NO_ASSERT(); + + Containers::StringView nameStrings = ""_s; + const struct Name { + UnsignedByte mapping; + UnsignedByte offset; + } namesData[3]{}; + auto names = Containers::stridedArrayView(namesData); + + std::ostringstream out; + Error redirectError{&out}; + /* A null string data pointer could work in this case (because it doesn't + need to be accessed), but disallowing it always for consistency */ + combineFields(Trade::SceneMappingType::UnsignedByte, 167, { + /* Just to verify it prints correct field IDs */ + Trade::SceneFieldData{Trade::SceneField::Mesh, + names.slice(&Name::mapping), + names.slice(&Name::offset)}, + Trade::SceneFieldData{Trade::sceneFieldCustom(16), + names.slice(&Name::mapping), + nullptr, Trade::SceneFieldType::StringOffset8, + names.slice(&Name::offset)}, + }); + /* With placeholder field data it's impossible to know the actual string + size */ + combineFields(Trade::SceneMappingType::UnsignedByte, 167, { + Trade::SceneFieldData{Trade::sceneFieldCustom(16), + names.slice(&Name::mapping), + nameStrings.data(), Trade::SceneFieldType::StringRangeNullTerminated16, + Containers::StridedArrayView1D{{nullptr, 6}, 3}}, + }); + CORRADE_COMPARE(out.str(), + "SceneTools::combineFields(): string field 1 has a placeholder string data\n" + "SceneTools::combineFields(): string field 0 has a placeholder data\n"); +} + void CombineTest::fieldsOffsetOnly() { CORRADE_SKIP_IF_NO_ASSERT();