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

Random level/pack selection #405

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Ena-Shepherd
Copy link

  • Hitting LeftCtrl will select a random level on your current pack
  • LeftCtrl + Focus selects a random pack
  • Compatible with favorites

@Ena-Shepherd Ena-Shepherd marked this pull request as draft April 2, 2023 14:53
@Ena-Shepherd Ena-Shepherd marked this pull request as ready for review April 2, 2023 14:55
@vittorioromeo vittorioromeo self-requested a review April 5, 2023 08:50
Copy link
Owner

@vittorioromeo vittorioromeo 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 the PR. Not opposed to this feature, just a bit uncomfortable with the hardcoded keys.

Comment on lines 1020 to 1030
int randomLevel;

if (lvlDrawer->levelDataIds->size() == 1)
return;

randomLevel = ssvu::getRndI(0, lvlDrawer->levelDataIds->size());

if (randomLevel >= lvlDrawer->levelDataIds->size())
setIndex(0);
else
setIndex(randomLevel);
Copy link
Owner

Choose a reason for hiding this comment

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

Why are all these checks necessary? Wouldn't the following just always work, or am I missing something?

Suggested change
int randomLevel;
if (lvlDrawer->levelDataIds->size() == 1)
return;
randomLevel = ssvu::getRndI(0, lvlDrawer->levelDataIds->size());
if (randomLevel >= lvlDrawer->levelDataIds->size())
setIndex(0);
else
setIndex(randomLevel);
setIndex(ssvu::getRndI(0, lvlDrawer->levelDataIds->size()));

Ditto for the other two similar functions.

Copy link
Author

@Ena-Shepherd Ena-Shepherd Apr 5, 2023

Choose a reason for hiding this comment

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

Without the checks, line 1025 sometimes returns what seems to be undefined behavior like "-124852".
This creates an out of bounds error segfault and crashes the program.
Another exception to check is if the pack contains only 1 level, like tutorial pack, or the favorites list.

There is also 2 Keybinds intended to act as the combo keys, but i checked on both Shift Keys instead of Focus, so i will rework on this. (didn't find how to check on Focus directly).

Edit :
Also declared randomLevel and RandomCollection as separated ints in each lambda for better readability.
(3 nested function calls is a bit heavy)

Copy link
Owner

Choose a reason for hiding this comment

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

ssvu::getRndI(a, b) returns a number between a and b-1. So ssvu::getRndI(0, lvlDrawer->levelDataIds->size()) should always give a valid index.

The only case where it can go wrong is if lvlDrawer->levelDataIds->size() == 0, because it can underflow to a very large number. Maybe that's the only required check?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, absolutely.
I'll modify that, and make the keys reassignable and everything should be good :)

Choose a reason for hiding this comment

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

Is there any chance you could add a bind for "random level in random pack" too? It is very commonly used in osu, for example.

Copy link
Author

@Ena-Shepherd Ena-Shepherd Apr 5, 2023

Choose a reason for hiding this comment

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

Is there any chance you could add a bind for "random level in random pack" too? It is very commonly used in osu, for example.

Yes, if @vittorioromeo is ok with that

@Ena-Shepherd Ena-Shepherd marked this pull request as draft April 5, 2023 19:29
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.

3 participants