Skip to content

Commit

Permalink
fix: Make sure it is sorted after removals that make bucket empty (#23)
Browse files Browse the repository at this point in the history
After discovering an issue with priority sorting in Flame, I dug down until I was able to narrow the issue down to the OrderedSet instance. In this specific situation, it appears the set does not maintain ordering between elements.

It seems that remove failed to remove empty buckets when it was the last element in the bucket, resulting in an invalid ordering between buckets in specific situations. I modified remove to delete empty sets instead.

Aside from making the new test pass, the code actually broke an existing test. On reading it, I believe the expected condition was incorrect. I fixed the test to the actual expected condition, allowing it to pass.
  • Loading branch information
misha committed Sep 11, 2023
1 parent e9cac9b commit d2cb63e
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 2 deletions.
3 changes: 2 additions & 1 deletion lib/ordered_set.dart
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,8 @@ class OrderedSet<E> extends IterableMixin<E> {
final result = bucket.remove(e);
if (result) {
_length--;
_backingSet.remove(<E>[]);
// If the removal resulted in an empty bucket, remove the bucket as well.
_backingSet.remove(<E>{});
_validReverseCache = false;
}
return result;
Expand Down
13 changes: 12 additions & 1 deletion test/ordered_set_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,18 @@ void main() {
expect(orderedSet.toList().join(), 'bd');
orderedSet.addAll([d, b, a, c]);
expect(orderedSet.removeAll([d, b]).join(), 'db');
expect(orderedSet.toList().join(), 'abc');
expect(orderedSet.toList().join(), 'ac');
});

test('sorts after remove', () {
final orderedSet = OrderedSet<int>();
// The initial elements must be in order.
orderedSet.addAll([1, 3, 4]);
expect(orderedSet.toList().join(), '134');
expect(orderedSet.remove(4), true);
expect(orderedSet.toList().join(), '13');
expect(orderedSet.add(2), true);
expect(orderedSet.toList().join(), '123');
});
});

Expand Down

0 comments on commit d2cb63e

Please sign in to comment.