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

Deprecated SpiralLoop cleanup. #1019

Merged
merged 1 commit into from
Oct 30, 2020
Merged

Deprecated SpiralLoop cleanup. #1019

merged 1 commit into from
Oct 30, 2020

Conversation

DaEgi01
Copy link
Contributor

@DaEgi01 DaEgi01 commented Oct 26, 2020

Removed the SpiralLoop method.
Adjusted unit- and performance tests.
Performance tests should use a mono path that should be more common for CSL.

Removed the SpiralLoop method.
Adjusted unit- and performance tests.
performance tests should use a mono path that should be more common for CSL.
@DaEgi01 DaEgi01 added the code cleanup Refactor code, remove old code, improve maintainability label Oct 26, 2020
@krzychu124 krzychu124 self-requested a review October 26, 2020 01:07
Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

for (int i = 0; i < 10; i++) {
var coords = spiral.GetCoords(radius);
for (int j = 0; j < coords.Count; j++) {
var coord = coords[j];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afraid this will be optimized out by the compiler but okay, if you're testing the generation code not the loop code.

Copy link
Contributor Author

@DaEgi01 DaEgi01 Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the perf tests were there to confirm that a) the implementation is more performant than the previous one and b) that the spiral class does not generate any garbage in the GetCoords call that is used in the gameloop.
I left the test there to verify b) and as a baseline should anyone wish to create an even better implementation.

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@DaEgi01 DaEgi01 merged commit 0b7fecf into master Oct 30, 2020
@DaEgi01 DaEgi01 deleted the oldspiralcleanup branch October 30, 2020 00:01
@originalfoo originalfoo added this to the 11.6.0 milestone Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Refactor code, remove old code, improve maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants