Closed afritz1 closed 1 year ago
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.
Have you made any progress on this?
No, have not looked at it, focusing on making the release.
C++20 std::span
is similar and seems to use std::remove_cv_t
. Maybe that would be useful here.
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)
{
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.
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?
Not enough Linux distributions have the latest GCC compilers.
BufferView/2D/3D are in much better shape now.
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
, andstd::is_const_v
on the buffer view constructors and functions but I still get compiler errors when I try to do things like this:I don't want
T
to have to beconst
but currently a lot of my buffer views are likeconst BufferView<const int>
. This feels like it's brushing up against similar issues thatstd::vector
has withconst_iterator
.I want this constructor to exist so I can make a
const BufferView<T>
from aconst Buffer<T>
:I wonder if some mixture of
std::underlying_type
orstd::remove_const
or something like that would help. I've given up at this point :/