Skip to content
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

Implementing circular stack for Command Stack #160

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

roman-koshchei
Copy link
Contributor

I added CircularStack, which is a fixed size stack that goes around if there is no more space to add new items.
It's related to the comment:

// @todo this is garbage for performance, as it has to copy the entire buffer when it's full
// could be solved by using linked lists instead of dynamic arrays (vectors)
// optimal solution may be to preallocate an array instead, and use a cursor to manage undo/redo <-- probably do this
// luckily we only store pointers so should be decent performance for now (as long as max_undo_steps doesn't grow too large)

I am sure we can make it even better by using only 1 stack instead of 2 for undo and redo commands. But I just wanted to make the initial step. I am not a game dev, but I like implementing data structures, so here it is.

Code is tested in my another project works fine. Currently, size type is hard-coded based on max_undo_steps type, but it can also be put into a template later.

Waiting for a feedback.

@PanosK92
Copy link
Owner

PanosK92 commented Oct 7, 2024

Thanks a lot for the PR!

Since the engine isn’t used for game development yet, there’s no need to worry too much about perfection. For example, I’m happy with how max_undo_steps is hardcoded, its nature doesn't require anything else, and simplicity is key, just as mentioned in the coding style\general advice.

Your work is a nice step forward, feel free to proceed. I’m ready to merge when you are.

@roman-koshchei
Copy link
Contributor Author

Thanks, it's ready to be merged.
I think putting size type into a template will be overkill and introduce unnecessary complexity at this point.

@PanosK92
Copy link
Owner

PanosK92 commented Oct 7, 2024

I agree, thanks!

@PanosK92 PanosK92 merged commit 4834f0a into PanosK92:master Oct 7, 2024
4 checks passed
@PanosK92
Copy link
Owner

PanosK92 commented Oct 7, 2024

I reached out on X, please check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants