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

[PLS] Planeshift - Striped Collation #12960

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

tiera3
Copy link
Contributor

@tiera3 tiera3 commented Oct 2, 2024

This also includes code to enable Striped Collation.

tiera3 added 3 commits October 2, 2024 11:23
Uses stripe widths of 2-5 inclusive with equal chances for each.
If data can be obtained, a later improvement would be to give different weightings to different stripe widths.
@tiera3
Copy link
Contributor Author

tiera3 commented Oct 2, 2024

The Travis Build is not loading on my screen, so at this point, I wont be able to track down the error.

@@ -15,7 +15,7 @@
public class Rotater<T> {

private final List<T> items;
private int position;
private int position, stripeLen = 0, stripeWidth, stripeDepth;
Copy link
Contributor

Choose a reason for hiding this comment

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

The code to implement striped collation almost certainly needs to be a separate class, perhaps both implementing a common interface.

You're going to have a hard time getting this to work without a dev environment for testing.

tiera3 added 3 commits October 2, 2024 13:27
Found error - had failed to change one of the classnames from my test version to xmage version.
Also made nextWidth private. (No need for it to be public.)
@tiera3
Copy link
Contributor Author

tiera3 commented Oct 2, 2024

Travis started co-operating and thus I was able to fix the simple errors.


Currently working on a version with the majority of the code for striped collation moved into CardRun instead.

@tiera3
Copy link
Contributor Author

tiera3 commented Oct 2, 2024

Okay. All done. Sorry that took so long.

Rotate.java line 47 - whilst an if statement should be sufficient. I put in a while statement for added safety.

There should probably be some sort of test to check when a cardRun is created with a stripeLen, that items.size % stripeLen ==0 (ie no incomplete rows).

Copy link
Contributor Author

@tiera3 tiera3 left a comment

Choose a reason for hiding this comment

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

moved the majority of the code for Striped Collation into cardRun.
didn't want a completely separate class, because still wanted it to be usable by BoosterStructure.
Some code had to be added to Rotator.java to allow for:
a) different offsets when iterating, and
b) ability to check if on leading edge of sheet.

FYI - I did run external tests to check that it selected cards in the correct order, for both sequential and striped collation.

@tiera3 tiera3 requested a review from xenohedron October 2, 2024 06:29
@xenohedron
Copy link
Contributor

I'm going to refactor the hierarchy. I don't think the current inheritance structure is appropriate.

Please hold off on other striped collation PRs until that's done. When I finish my framework, I will ask you to check over it. Once it is in place, you can make one PR with individual commits for each set that needs striped collation.

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

Successfully merging this pull request may close these issues.

2 participants