Skip to content

Conversation

@cclauss
Copy link
Collaborator

@cclauss cclauss commented Nov 4, 2019

Fixes #101

Copy link
Collaborator

@StoicLoofah StoicLoofah left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this! I have to admit that I'm not really familiar with real_length, but looking at the speed factors, which can be fractional, it seems like floats can certainly exist here

"LotV": {"Slower": 0.2, "Slow": 0.4, "Normal": 0.6, "Fast": 0.8, "Faster": 1.0},

@cclauss
Copy link
Collaborator Author

cclauss commented Nov 4, 2019

So you want me to default to 0.6 instead of 1?

@StoicLoofah
Copy link
Collaborator

Sorry for being unclear: I missed actual question in the context =P

Why did you decide to use integer division instead of regular division? I'm not really sure how real/important the fractional parts are

@cclauss
Copy link
Collaborator Author

cclauss commented Nov 5, 2019

The current code wraps the calculation in int() and this PR uses // because
all(int(a / b) == a // b for a, b in ((1, 0.6), (1, 1.2), (10, 0.6), (100, 0.6), (0.6, 1))) # -> True

@StoicLoofah
Copy link
Collaborator

OH! I missed that int() in the original code. You definitely did that right. Also, I think using 1.0 as the default speed is probably more correct than 0.6. So everything you initially did everything correct =P

If you revert that, I will approve and merge. Thanks (and apologies) for bearing with me!

@cclauss
Copy link
Collaborator Author

cclauss commented Nov 5, 2019

#102 (review) So you want to default to faster, not normal?

@StoicLoofah
Copy link
Collaborator

Yes, let's default to faster. It will at least be ocrrect for alphastar and is the de facto default these for LotV ladder, right?

@StoicLoofah StoicLoofah merged commit 9d97aef into ggtracker:upstream Nov 5, 2019
@StoicLoofah
Copy link
Collaborator

Thanks!

@cclauss cclauss deleted the patch-4 branch November 5, 2019 08:33
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.

Unable to parse alphastar replays

2 participants