-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Queue curve rework #1616
Merged
Merged
Queue curve rework #1616
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
heinezen
added
improvement
Enhancement of an existing component
lang: c++
Done in C++ code
code quality
Does not alter behavior, but beauty of our code
area: simulation
Involved in the game mechanics and simulation
labels
Jan 14, 2024
Depends on #1603 |
heinezen
force-pushed
the
feature/queue_rework
branch
from
January 14, 2024 20:09
66cc071
to
ba06e5d
Compare
heinezen
force-pushed
the
feature/queue_rework
branch
7 times, most recently
from
January 16, 2024 02:24
23bfb70
to
19a7fc2
Compare
heinezen
force-pushed
the
feature/queue_rework
branch
2 times, most recently
from
January 29, 2024 19:04
3a6eace
to
3f81f48
Compare
'front_start' was not a valid element when the queue was empty.
heinezen
force-pushed
the
feature/queue_rework
branch
from
February 13, 2024 17:03
3f81f48
to
4bef85b
Compare
TheJJ
previously approved these changes
Feb 13, 2024
TheJJ
approved these changes
Feb 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area: simulation
Involved in the game mechanics and simulation
code quality
Does not alter behavior, but beauty of our code
improvement
Enhancement of an existing component
lang: c++
Done in C++ code
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reworks the curve queue to make it more intuitive to use and more efficient in certain situations.
In the original design by @Tomatower , elements in a curved queue are mostly accessed by requesting all elements in a timespan (e.g. all elements between
t1 == 0
andt2 == 5
). You could then iterate through these elements to process them.front(t)
orpop(t)
access (like in "normal" queues) to retrieve/remove the front element at a timet
was not possible.However, there are a few problems with this in practice which made the queue unintuitive to use. If the elements are processed one at a time, the requirement for a timespan puts a burden on the calling code to track which time/elements have already been processed. If the queue is shared by multiple callers, this adds a lot of additional complexity. Furthermore, since popping is not possible, there is no real way to "empty" the queue for a specific timespan, except by tracking this status outside of the queue. Filtering a timespan was also an expensive operation with a complexity of O(n) vs. O(1) for most operations in
std::queue
.#1609 already introduced a few changes like the addition of
front(t)
,pop_front(t)
andempty(t)
to address some of these problems, but these only work under the assumption that time is always progressing forward, so they didn't fulfill the basic curve properties. This PR addresses this and also changes the operations logic to make more sense for people familiar withstd::queue
.Track lifespan of elements with
dead
timeLike
curve::UnorderedMap
,curve::Queue
now tracks the lifespan of elements with insertion time (alive
) and erasure time (dead
).dead
is now set when an element is removed bypop_front(t)
which makes the element invisible for pops at later times. Elements are also no longer erased from the queue if they are popped.Change timespan observed by
front
/pop_front
Previously, the operations
front(t)
andpop_front(t)
only considered elements that were inserted after or att
. However, this meant that e.g.front(2)
had no result even if the queue contained elementsA
andB
with insertion times0
and1
respectively. Since this was counter-intuitive, these operations will now consider elements inserted untilt
, i.e. before or att
. In other words, the front element is now the element with the lowest insertion time that is also still alive att
.Optimizations for common operations
front(t)
/pop_front(t)
/empty(t)
/insert(t)
have been optimized for the case that simulation time always progresses forwards, i.e. that the timet
always increases for each access, by caching the iterator to the last accessed element. This optimization should cover the normal use case of running a game. Complexity for these operations is then O(1) (vs. O(n) previously).