Add support for buffer reference in buffer view constructors

[afritz1]

The buffer view classes in components/utilities/ (BufferView, BufferView2D, BufferView3D) are currently a little underutilized and inconvenient, and I would like them to work more naturally with the buffer classes (Buffer, Buffer2D, etc.). A problem I'm facing with that though is related to const-ness.

I have tried and tried all kinds of const/not-const, std::enable_if_t, and std::is_const_v on the buffer view constructors and functions but I still get compiler errors when I try to do things like this:

#include "Buffer.h"
#include "BufferView.h"

class MyClass
{
    Buffer<int> buffer;

    const BufferView<int> getBufferView() const
    {
        const BufferView<int> bufferView(this->buffer); // error: Buffer's T is const, BufferView's T is not.
        return bufferView;
    }
}

I don't want T to have to be const but currently a lot of my buffer views are like const BufferView<const int>. This feels like it's brushing up against similar issues that std::vector has with const_iterator.

I want this constructor to exist so I can make a const BufferView<T> from a const Buffer<T>:

BufferView(const Buffer<T> &buffer)

I wonder if some mixture of std::underlying_type or std::remove_const or something like that would help. I've given up at this point :/

[afritz1]

I think the source of the problem is that BufferView::data is T*, so if we just do std::remove_const on the Buffer::get() in BufferView(const Buffer<T>&), then we could assign the pointer. This breaks the rules a little bit but the rules will be upheld by all of the public BufferView functions.

[EndlessVoid1]

Have you made any progress on this?

[afritz1]

No, have not looked at it, focusing on making the release.

[afritz1]

C++20 std::span is similar and seems to use std::remove_cv_t. Maybe that would be useful here.

[makemeunsee]

I tried using span directly, and indeed it looks useful; some bits could be simplified. I havent found an actual usage of any Buffer constructed with offsets, so I havent tried but it should be as straightforward as calling .subspan(viewOffset, viewLength). Here's is a diff showing the usage of span replacing BufferView in a few select case:

index a7072c8d..38082c17 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -15,7 +15,7 @@ PROJECT(OpenTESArena)
 set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/")
 
 # Set global C++ standard for all targets.
-set(CMAKE_CXX_STANDARD 17)
+set(CMAKE_CXX_STANDARD 20)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS ON)
 
diff --git a/OpenTESArena/src/Assets/MIFFile.cpp b/OpenTESArena/src/Assets/MIFFile.cpp
index 033f3816..451390e1 100644
--- a/OpenTESArena/src/Assets/MIFFile.cpp
+++ b/OpenTESArena/src/Assets/MIFFile.cpp
@@ -408,9 +408,10 @@ BufferView<const ArenaTypes::MIFLock> MIFFile::Level::getLOCK() const
        return BufferView<const ArenaTypes::MIFLock>(this->lock.data(), static_cast<int>(this->lock.size()));
 }
 
-BufferView<const ArenaTypes::MIFTrigger> MIFFile::Level::getTRIG() const
+std::span<const ArenaTypes::MIFTrigger> MIFFile::Level::getTRIG() const
 {
-       return BufferView<const ArenaTypes::MIFTrigger>(this->trig.data(), static_cast<int>(this->trig.size()));
+       std::span<const ArenaTypes::MIFTrigger> sp{trig};
+       return sp.subspan(0, trig.size());
 }
 
 bool MIFFile::init(const char *filename)
diff --git a/OpenTESArena/src/Assets/MIFFile.h b/OpenTESArena/src/Assets/MIFFile.h
index ed73157d..fbf6f1e1 100644
--- a/OpenTESArena/src/Assets/MIFFile.h
+++ b/OpenTESArena/src/Assets/MIFFile.h
@@ -3,6 +3,7 @@
 
 #include <array>
 #include <cstdint>
+#include <span>
 #include <string>
 #include <vector>
 
@@ -77,7 +78,7 @@ public:
 
                BufferView<const ArenaTypes::MIFTarget> getTARG() const;
                BufferView<const ArenaTypes::MIFLock> getLOCK() const;
-               BufferView<const ArenaTypes::MIFTrigger> getTRIG() const;
+               std::span<const ArenaTypes::MIFTrigger> getTRIG() const;
        };
 private:
        WEInt width;
diff --git a/OpenTESArena/src/Assets/MIFUtils.h b/OpenTESArena/src/Assets/MIFUtils.h
index eef56e21..ee5ebeb7 100644
--- a/OpenTESArena/src/Assets/MIFUtils.h
+++ b/OpenTESArena/src/Assets/MIFUtils.h
@@ -2,6 +2,7 @@
 #define MIF_UTILS_H
 
 #include <cstdint>
+#include <span>
 #include <string>
 
 #include "../Math/Vector2.h"
diff --git a/OpenTESArena/src/World/MapGeneration.cpp b/OpenTESArena/src/World/MapGeneration.cpp
index 96dcef7e..5794f3c5 100644
--- a/OpenTESArena/src/World/MapGeneration.cpp
+++ b/OpenTESArena/src/World/MapGeneration.cpp
@@ -1,5 +1,6 @@
 #include <algorithm>
 #include <unordered_map>
+#include <span>
 
 #include "ArenaCityUtils.h"
 #include "ArenaInteriorUtils.h"
@@ -1341,11 +1342,8 @@ namespace MapGeneration
                                }
 
                                // Assign text/sound triggers to the current block.
-                               const BufferView<const ArenaTypes::MIFTrigger> &blockTRIG = blockLevel.getTRIG();
-                               for (int i = 0; i < blockTRIG.getCount(); i++)
+                               for (auto &trigger : blockLevel.getTRIG())
                                {
-                                       const auto &trigger = blockTRIG.get(i);
-
                                        ArenaTypes::MIFTrigger tempTrigger;
                                        tempTrigger.x = xOffset + trigger.x;
                                        tempTrigger.y = zOffset + trigger.y;
@@ -2377,11 +2375,8 @@ void MapGeneration::readMifTriggers(const BufferView<const MIFFile::Level> &leve
        {
                const MIFFile::Level &level = levels.get(i);
                LevelDefinition &levelDef = outLevelDefs.get(i);
-               const BufferView<const ArenaTypes::MIFTrigger> triggers = level.getTRIG();
-
-               for (int j = 0; j < triggers.getCount(); j++)
+               for (auto& trigger : level.getTRIG())
                {
-                       const ArenaTypes::MIFTrigger &trigger = triggers.get(j);
                        MapGeneration::readArenaTrigger(trigger, inf, &levelDef, outLevelInfoDef, &triggerMappings);
                }
        }
diff --git a/components/utilities/BufferView.h b/components/utilities/BufferView.h
index 29289f8e..8bd3a4e9 100644
--- a/components/utilities/BufferView.h
+++ b/components/utilities/BufferView.h
@@ -23,13 +23,6 @@ public:
                this->reset();
        }
 
-       // View across a subset of a range of data. Provided for bounds-checking the view range
-       // inside a full range (data, data + count) at initialization.
-       BufferView(T *data, int count, int viewOffset, int viewCount)
-       {
-               this->init(data, count, viewOffset, viewCount);
-       }
-
        // View across a range of data.
        BufferView(T *data, int count)
        {
diff --git a/components/utilities/BufferView2D.h b/components/utilities/BufferView2D.h
index a3cd2c50..37d4c4f1 100644
--- a/components/utilities/BufferView2D.h
+++ b/components/utilities/BufferView2D.h
@@ -34,13 +34,6 @@ public:
                this->reset();
        }
 
-       // View across a subset of a 2D range of data. The original 2D range's dimensions are required
-       // for proper look-up (and bounds-checking).
-       BufferView2D(T *data, int width, int height, int viewX, int viewY, int viewWidth, int viewHeight)
-       {
-               this->init(data, width, height, viewX, viewY, viewWidth, viewHeight);
-       }
-
        // View across a 2D range of data.
        BufferView2D(T *data, int width, int height)
        {
diff --git a/components/utilities/BufferView3D.h b/components/utilities/BufferView3D.h
index c1d7b49a..b6f6529f 100644
--- a/components/utilities/BufferView3D.h
+++ b/components/utilities/BufferView3D.h
@@ -37,14 +37,6 @@ public:
                this->reset();
        }
 
-       // View across a subset of a 3D range of data. The original 3D range's dimensions are required
-       // for proper look-up (and bounds-checking).
-       BufferView3D(T *data, int width, int height, int depth, int viewX, int viewY, int viewZ,
-               int viewWidth, int viewHeight, int viewDepth)
-       {
-               this->init(data, width, height, depth, viewX, viewY, viewZ, viewWidth, viewHeight, viewDepth);
-       }
-
        // View across a 3D range of data.
        BufferView3D(T *data, int width, int height, int depth)
        {

[afritz1]

I like the example but it isn't showing me anything new. Going to wait a while before adopting C++20 anyway. My point was that maybe the implementation details of std::span could help with how BufferView is written.

I also want to keep the slice constructors for later. I have an idea for how to make the non-sliced instances not have a look-up penalty.

My next shot in the dark might be to have a ReadOnlyBufferView<T> class that can take a non-const Buffer& and promote to const internally without needing const in the typename. Really the point here is to separate the ownership of a contiguous range from its usage without accidentally manipulating the const-ness.

[makemeunsee]

yes, baring span, or re-implementing an equivalent (but that's likely the kind of code you want in the standard library and not in your project), the next best and clean thing is a dedicated class for the read-only cases. I think I'll give it a try in a branch and may submit an MR then; worst case I'll get to know a bit better the code.

out of curiosity, what motivates waiting before adopting C++20?

[afritz1]

Not enough Linux distributions have the latest GCC compilers.

[afritz1]

BufferView/2D/3D are in much better shape now.