refactor(scorch): Store scorch structs in a deque and various minor fixes - #3159
refactor(scorch): Store scorch structs in a deque and various minor fixes#3159stephanmeesters wants to merge 5 commits into
Conversation
stephanmeesters
added
Gen
PR Summary by QodoScorch mark queue: dedupe before eviction and store scorches in std::deque
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
328f969 to
9414e54
Compare
stephanmeesters
changed the title
|
The PR description doesn't include why |
Done |
|
It's a bit of a tradeoff. It's clearly a win for the eviction of the oldest scorch mark, but a loss for memory access. An array + head & tail would be faster but make the code more complex so I wouldn't necessarily recommend it. |
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
Fixes and refactors look correct. But I am not sold on the std::deque. Maybe there is a better container type for it?
| Int m_curNumScorchIndices; ///< number of indices used in m_indexScorch. | ||
| TScorch m_scorches[MAX_SCORCH_MARKS]; | ||
| Int m_numScorches; | ||
| std::deque<TScorch> m_scorches; |
There was a problem hiding this comment.
A circular buffer would be the better data type for this I think.
I asked Chat Gippy to generate a Circular Buffer template class. Maybe you can try this kind of direction.
Chat
Code
Here’s a straightforward C++98-compatible circular buffer using a dynamically allocated array. It supports push/pop at both ends, indexing, and fixed capacity.
#include <cstddef>
#include <stdexcept>
#include <iterator>
template <typename T>
class CircularBuffer
{
public:
class iterator;
class const_iterator;
explicit CircularBuffer(std::size_t capacity)
: data_(capacity ? new T[capacity] : 0),
capacity_(capacity),
head_(0),
size_(0)
{
if (capacity == 0)
throw std::invalid_argument("capacity must be greater than zero");
}
~CircularBuffer()
{
delete[] data_;
}
private:
CircularBuffer(const CircularBuffer&);
CircularBuffer& operator=(const CircularBuffer&);
public:
bool empty() const
{
return size_ == 0;
}
bool full() const
{
return size_ == capacity_;
}
std::size_t size() const
{
return size_;
}
std::size_t capacity() const
{
return capacity_;
}
void push_back(const T& value)
{
if (full())
throw std::overflow_error("circular buffer is full");
data_[index(size_)] = value;
++size_;
}
void pop_front()
{
if (empty())
throw std::underflow_error("circular buffer is empty");
head_ = next(head_);
--size_;
}
T& front()
{
if (empty())
throw std::underflow_error("circular buffer is empty");
return data_[head_];
}
const T& front() const
{
if (empty())
throw std::underflow_error("circular buffer is empty");
return data_[head_];
}
T& back()
{
if (empty())
throw std::underflow_error("circular buffer is empty");
return data_[index(size_ - 1)];
}
const T& back() const
{
if (empty())
throw std::underflow_error("circular buffer is empty");
return data_[index(size_ - 1)];
}
T& operator[](std::size_t i)
{
return data_[index(i)];
}
const T& operator[](std::size_t i) const
{
return data_[index(i)];
}
void clear()
{
head_ = 0;
size_ = 0;
}
iterator begin()
{
return iterator(this, 0);
}
iterator end()
{
return iterator(this, size_);
}
const_iterator begin() const
{
return const_iterator(this, 0);
}
const_iterator end() const
{
return const_iterator(this, size_);
}
const_iterator cbegin() const
{
return const_iterator(this, 0);
}
const_iterator cend() const
{
return const_iterator(this, size_);
}
private:
std::size_t next(std::size_t i) const
{
return (i + 1) % capacity_;
}
std::size_t index(std::size_t i) const
{
return (head_ + i) % capacity_;
}
T* data_;
std::size_t capacity_;
std::size_t head_;
std::size_t size_;
public:
class iterator : public std::iterator<std::random_access_iterator_tag, T>
{
public:
iterator()
: buffer_(0), pos_(0)
{
}
iterator(CircularBuffer* buffer, std::size_t pos)
: buffer_(buffer), pos_(pos)
{
}
T& operator*() const
{
return (*buffer_)[pos_];
}
T* operator->() const
{
return &(*buffer_)[pos_];
}
iterator& operator++()
{
++pos_;
return *this;
}
iterator operator++(int)
{
iterator tmp(*this);
++(*this);
return tmp;
}
iterator& operator--()
{
--pos_;
return *this;
}
iterator operator--(int)
{
iterator tmp(*this);
--(*this);
return tmp;
}
iterator operator+(std::ptrdiff_t n) const
{
return iterator(buffer_, pos_ + n);
}
iterator operator-(std::ptrdiff_t n) const
{
return iterator(buffer_, pos_ - n);
}
std::ptrdiff_t operator-(const iterator& other) const
{
return static_cast<std::ptrdiff_t>(pos_) -
static_cast<std::ptrdiff_t>(other.pos_);
}
iterator& operator+=(std::ptrdiff_t n)
{
pos_ += n;
return *this;
}
iterator& operator-=(std::ptrdiff_t n)
{
pos_ -= n;
return *this;
}
T& operator[](std::ptrdiff_t n) const
{
return (*buffer_)[pos_ + n];
}
bool operator==(const iterator& other) const
{
return buffer_ == other.buffer_ && pos_ == other.pos_;
}
bool operator!=(const iterator& other) const
{
return !(*this == other);
}
bool operator<(const iterator& other) const
{
return pos_ < other.pos_;
}
bool operator>(const iterator& other) const
{
return other < *this;
}
bool operator<=(const iterator& other) const
{
return !(other < *this);
}
bool operator>=(const iterator& other) const
{
return !(*this < other);
}
private:
CircularBuffer* buffer_;
std::size_t pos_;
};
class const_iterator
: public std::iterator<std::random_access_iterator_tag, const T>
{
public:
const_iterator()
: buffer_(0), pos_(0)
{
}
const_iterator(const CircularBuffer* buffer, std::size_t pos)
: buffer_(buffer), pos_(pos)
{
}
const T& operator*() const
{
return (*buffer_)[pos_];
}
const T* operator->() const
{
return &(*buffer_)[pos_];
}
const_iterator& operator++()
{
++pos_;
return *this;
}
const_iterator operator++(int)
{
const_iterator tmp(*this);
++(*this);
return tmp;
}
const_iterator& operator--()
{
--pos_;
return *this;
}
const_iterator operator--(int)
{
const_iterator tmp(*this);
--(*this);
return tmp;
}
const_iterator operator+(std::ptrdiff_t n) const
{
return const_iterator(buffer_, pos_ + n);
}
const_iterator operator-(std::ptrdiff_t n) const
{
return const_iterator(buffer_, pos_ - n);
}
std::ptrdiff_t operator-(const const_iterator& other) const
{
return static_cast<std::ptrdiff_t>(pos_) -
static_cast<std::ptrdiff_t>(other.pos_);
}
const_iterator& operator+=(std::ptrdiff_t n)
{
pos_ += n;
return *this;
}
const_iterator& operator-=(std::ptrdiff_t n)
{
pos_ -= n;
return *this;
}
const T& operator[](std::ptrdiff_t n) const
{
return (*buffer_)[pos_ + n];
}
bool operator==(const const_iterator& other) const
{
return buffer_ == other.buffer_ && pos_ == other.pos_;
}
bool operator!=(const const_iterator& other) const
{
return !(*this == other);
}
bool operator<(const const_iterator& other) const
{
return pos_ < other.pos_;
}
bool operator>(const const_iterator& other) const
{
return other < *this;
}
bool operator<=(const const_iterator& other) const
{
return !(other < *this);
}
bool operator>=(const const_iterator& other) const
{
return !(*this < other);
}
private:
const CircularBuffer* buffer_;
std::size_t pos_;
};
};Example
#include <iostream>
int main()
{
CircularBuffer<int> buffer(3);
buffer.push_back(10);
buffer.push_back(20);
buffer.push_back(30);
std::cout << buffer.front() << std::endl; // 10
std::cout << buffer.back() << std::endl; // 30
buffer.pop_front();
buffer.push_back(40);
for (std::size_t i = 0; i < buffer.size(); ++i)
std::cout << buffer[i] << " ";
// Output: 20 30 40
return 0;
}The key idea is that head_ points to the logical first element, while size_ tracks how many elements are currently stored. Physical positions wrap using % capacity_, so insertion/removal from the ends is O(1) and indexing is also O(1).
Iterator usage is STL-like:
CircularBuffer<int> buffer(5);
buffer.push_back(10);
buffer.push_back(20);
buffer.push_back(30);
CircularBuffer<int>::iterator it;
for (it = buffer.begin(); it != buffer.end(); ++it)
std::cout << *it << " ";
// 10 20 30Because the iterator uses a logical position rather than a raw pointer, iteration works correctly even when the buffer wraps around physically:
Physical storage:
[40][50][30][ ][ ]
^
head
Logical iteration:
30 -> 40 -> 50
One C++98 caveat: std::iterator was deprecated much later and removed in C++20, but it is valid C++98. If you want the class to be compatible with both C++98 and modern C++, I would avoid inheriting from std::iterator and define the iterator typedefs directly.
There was a problem hiding this comment.
Maybe just put a 'todo' to refactor this when we drop C++98 compatibility and keep it simple for now.
There was a problem hiding this comment.
Circular Buffer class does not exist in c++20 either. So needs impl anyway.
There was a problem hiding this comment.
Some things can be simplified in C++20.
There was a problem hiding this comment.
Would agree with Caball, then we could source a properly made version of the circular buffer like this one
| TScorch scorch; | ||
| scorch.location = location; | ||
| scorch.radius = radius; | ||
| scorch.scorchType = type < 0 || (Int)type >= SCORCH_MARKS_IN_TEXTURE ? SCORCH_1 : type; |
There was a problem hiding this comment.
This test is new. Maybe make it an assert instead?
3 participants
Merge by rebase
std::dequeinstead of a fixed-size arrayThe
std::dequeis preferable because 1) it allows for a dynamic number of max scorches (if we were to increase in the future, and make quality level dependent); 2) performance.Todo