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.

Sign up for GitHub

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

Filter beatmap sets based on conditional searches (eg: AR>9) #323

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Filter beatmap sets based on conditional searches (eg: AR>9) #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 33 additions & 9 deletions src/itdelatrisu/opsu/beatmap/BeatmapSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public class BeatmapSet implements Iterable<Beatmap> {
/** List of associated beatmaps. */
private final ArrayList<Beatmap> beatmaps;

/** Search filter */
private ArrayList<Beatmap> filteredBeatmaps = new ArrayList<>();
/**
* Constructor.
* @param beatmaps the beatmaps in this set
Expand All @@ -43,24 +45,35 @@ public BeatmapSet(ArrayList<Beatmap> beatmaps) {
}

/**
* Returns the number of elements.
* Returns the number of search results or total number.
Copy link
Owner

Choose a reason for hiding this comment

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

Returns the number of elements (possibly filtered).

*/
public int size() { return beatmaps.size(); }
public int size() { return filteredBeatmaps.size() > 0 ? filteredBeatmaps.size() : beatmaps.size(); }
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you conditionally return the unfiltered size here -- wouldn't that cause problems? (But check that it doesn't break things if you remove it.)

Copy link
Author

Choose a reason for hiding this comment

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

At the moment how it is written is that filtered list starts off empty, and then maps are added to the filter. If the filter list is empty then it is assumed no filter is applied so therefore the unfiltered size should be returned, else other parts of the program will think there's no maps at all.

I can change it so that instead filteredBeatmaps is a clone of beatmaps at first and then remove maps based on the criteria, that way it can always return filteredBeatmaps.size/get


/**
* Returns the true number of elements
Copy link
Owner

Choose a reason for hiding this comment

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

Returns the number of elements (unfiltered).

*/
public int trueSize(){return beatmaps.size(); }
Copy link
Owner

Choose a reason for hiding this comment

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

Would prefer a more meaningful name like unfilteredSize().


/**
* Returns the beatmap at the given index.
* @param index the beatmap index
* @throws IndexOutOfBoundsException if the index is out of range
*/
public Beatmap get(int index) { return beatmaps.get(index); }
public Beatmap get(int index) { return filteredBeatmaps.size() > 0 ? filteredBeatmaps.get(index) : beatmaps.get(index); }
Copy link
Owner

Choose a reason for hiding this comment

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

Again, why would we want the unfiltered get() here?


/**
* Removes the beatmap at the given index.
* @param index the beatmap index
* @return the removed beatmap
* @throws IndexOutOfBoundsException if the index is out of range
*/
public Beatmap remove(int index) { return beatmaps.remove(index); }
public Beatmap remove(int index) {
if(filteredBeatmaps.size() > 0){
beatmaps.remove(this.get(index));
return filteredBeatmaps.remove(index);
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't we also want to remove the beatmap from the unfiltered list too? (Wouldn't this cause the deleted beatmap to reappear when the filter is reset?)

}
return beatmaps.remove(index);
}

@Override
public Iterator<Beatmap> iterator() { return beatmaps.iterator(); }
Expand All @@ -78,7 +91,7 @@ public BeatmapSet(ArrayList<Beatmap> beatmaps) {
* @throws IndexOutOfBoundsException if the index is out of range
*/
public String[] getInfo(int index) {
Beatmap beatmap = beatmaps.get(index);
Beatmap beatmap = this.get(index);
Copy link
Owner

Choose a reason for hiding this comment

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

this isn't necessary here:

Beatmap beatmap = get(index);

float speedModifier = GameMod.getSpeedMultiplier();
long endTime = (long) (beatmap.endTime / speedModifier);
int bpmMin = (int) (beatmap.bpmMin * speedModifier);
Expand Down Expand Up @@ -116,13 +129,20 @@ public String toString() {
return String.format("%s - %s", beatmap.getArtist(), beatmap.getTitle());
}

/**
* Clears the search array so we correctly reset the set after changing a filter
Copy link
Owner

Choose a reason for hiding this comment

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

Clears the filter on the beatmap set.

*/
public void clearFilter(){
filteredBeatmaps.clear();}
Copy link
Owner

Choose a reason for hiding this comment

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

Fix the formatting here. :P


/**
* Checks whether the beatmap set matches a given search query.
* @param query the search term
* @return true if title, artist, creator, source, version, or tag matches query
*/
public boolean matches(String query) {
// search: title, artist, creator, source, version, tags (first beatmap)
filteredBeatmaps.clear();
Copy link
Owner

Choose a reason for hiding this comment

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

matches() shouldn't mutate any state (or if it does, that should be documented, since it's counterintuitive). It makes more sense to call clearFilter() outside of this method.

Beatmap beatmap = beatmaps.get(0);
if (beatmap.title.toLowerCase().contains(query) ||
beatmap.titleUnicode.toLowerCase().contains(query) ||
Expand Down Expand Up @@ -153,6 +173,8 @@ public boolean matches(String query) {
* @return true if the condition is met
*/
public boolean matches(String type, String operator, float value) {
filteredBeatmaps.clear();
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above.

boolean found = false;
for (Beatmap beatmap : beatmaps) {
// get value
float v;
Expand Down Expand Up @@ -180,11 +202,13 @@ public boolean matches(String type, String operator, float value) {
default: return false;
}

if (met)
return true;
}
if (met){
filteredBeatmaps.add(beatmap);
Copy link
Owner

Choose a reason for hiding this comment

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

You could write a (very similar) method filter(type, operator, value) that sets your filteredBeatmaps list. It's more redundant but still cleaner than this approach IMO.

found = true;
}

return false;
}
return found;
}

/**
Expand Down
26 changes: 24 additions & 2 deletions src/itdelatrisu/opsu/beatmap/BeatmapSetList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ public class BeatmapSetList {
/** Current list of nodes (subset of parsedNodes, used for searches). */
private ArrayList<BeatmapSetNode> nodes;

/** nodes in last search, used to reset conditional search matches */
private ArrayList<BeatmapSetNode> lastSearchNodes;

/** Set of all beatmap set IDs for the parsed beatmaps. */
private HashSet<Integer> MSIDdb;

Expand Down Expand Up @@ -100,12 +103,26 @@ private BeatmapSetList() {
* This does not erase any parsed nodes.
*/
public void reset() {
resetFiltered();
nodes = groupNodes = BeatmapGroup.current().filter(parsedNodes);
expandedIndex = -1;
expandedStartNode = expandedEndNode = null;
lastQuery = null;
}

/**
* Will clear the filter on any mapsets that matched the last search request
*/
private void resetFiltered(){
Copy link
Owner

Choose a reason for hiding this comment

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

For the sake of clarity, I think it's better to just call clearFilter() on everything in nodes, and not bother keeping the extra lastSearchNodes reference. It's not noticeably helping performance, and I don't want this class getting much more complex (it's already pretty confusing).

Alternatively, since lastSearchNodes is just pointing to nodes anyway, why not use a boolean flag instead?

//Reset any nodes we filtered maps for
if(lastSearchNodes != null){
for(BeatmapSetNode n : lastSearchNodes)
n.getBeatmapSet().clearFilter();
//This is most likely going to be referencing nodes therefore we don't want to clear it
lastSearchNodes = new ArrayList<>();
}
}

/**
* Returns the number of elements.
*/
Expand Down Expand Up @@ -172,7 +189,7 @@ else if (ePrev != null && ePrev.index == expandedIndex)
nodes.remove(index);
parsedNodes.remove(eCur);
groupNodes.remove(eCur);
mapCount -= beatmapSet.size();
mapCount -= beatmapSet.trueSize();
if (beatmap.beatmapSetID > 0)
MSIDdb.remove(beatmap.beatmapSetID);
for (Beatmap bm : beatmapSet) {
Expand Down Expand Up @@ -237,7 +254,7 @@ public boolean deleteSong(BeatmapSetNode node) {

// last song in group?
int size = node.getBeatmapSet().size();
if (size == 1)
if (node.getBeatmapSet().trueSize() == 1)
return deleteSongGroup(node);

// reset indices
Expand Down Expand Up @@ -446,6 +463,9 @@ public boolean search(String query) {
lastQuery = query;
LinkedList<String> terms = new LinkedList<String>(Arrays.asList(query.split("\\s+")));

//Reset any map sets we previously filtered maps in
resetFiltered();

// if empty query, reset to original list
if (query.isEmpty() || terms.isEmpty()) {
nodes = groupNodes;
Expand Down Expand Up @@ -480,6 +500,8 @@ public boolean search(String query) {
if (node.getBeatmapSet().matches(type, operator, value))
nodes.add(node);
}
//theres no need to store a normal search as beatmaps wont be filtered at all and therefore won't need to be reset
lastSearchNodes = nodes;
} else {
// normal term
String term = terms.remove();
Expand Down
2 changes: 1 addition & 1 deletion src/itdelatrisu/opsu/states/ButtonMenu.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ public void click(GameContainer container, StateBasedGame game) {
public void click(GameContainer container, StateBasedGame game) {
SoundController.playSound(SoundEffect.MENUHIT);
BeatmapSetNode node = ((ButtonMenu) game.getState(Opsu.STATE_BUTTONMENU)).getNode();
MenuState ms = (node.beatmapIndex == -1 || node.getBeatmapSet().size() == 1) ?
MenuState ms = (node.beatmapIndex == -1 || node.getBeatmapSet().trueSize() == 1) ?
MenuState.BEATMAP_DELETE_CONFIRM : MenuState.BEATMAP_DELETE_SELECT;
((ButtonMenu) game.getState(Opsu.STATE_BUTTONMENU)).setMenuState(ms, node);
game.enterState(Opsu.STATE_BUTTONMENU);
Expand Down