Update NO_REPEAT_RANDOM to use Sample Without Replacement (2nd Try) by pedker · Pull Request #853 · profezzorn/ProffieOS

pedker · 2025-09-17T05:44:27Z

This is a second try at this PR, attempting to address its raised issues.

The storage used to manage the Sample-Without-Replacement algorithm is now a one-dimensional boolean array on each Effect, created statically of size NO_REPEAT_RANDOM_BUFFER_SIZE_BITS. Each bit of the array is used to manage an individual Sound File, and the array's size determines the maximum number of Sound Files each Effect can support using SWR. If an Effect has more Sound Files than available memory, then the Effect will default back to using the existing NO_REPEAT_RANDOM logic.

To meet the requirement of using less than one byte of memory per File, the value of NO_REPEAT_RANDOM_BUFFER_SIZE_BITS should be tuned relative to the ratio of Files to Effects used by Fonts on average. For example, setting it to 16 would only require the ratio to be 2:1, A value of 24 would require 3:1, and a value of 32 would require 4:1.

2nd attempt

profezzorn · 2025-09-21T10:18:21Z

sound/effect.h

 #ifdef KILL_OLD_PLAYERS
    killable_ = false;
+#endif
+#ifdef NO_REPEAT_RANDOM


If we're going to have this code (which I'm not entirely convinced it is a good idea.) then we should use a new define to enable it. Maybe the NO_REPEAT_RANDOM_BUFFER_SIZE_BITS define?

profezzorn · 2025-09-21T10:19:16Z

sound/effect.h


 #ifdef NO_REPEAT_RANDOM
+  // Storage for random sampling without replacement.
+  bool available_[NO_REPEAT_RANDOM_BUFFER_SIZE_BITS];


An array of bool does not take one bit per bool.
In most cases, a bool takes one byte.

Ah, crap! I incorrectly assumed that a boolean took up one bit in memory. I will have to look into the feasibility of refactoring to instead use a single integer with bitmasking.

This means that available_ will change to be either of type uint16_t or uint32_t, and the size will no longer be configurable. The current version of the define macro will be renamed/reused as the enable define, mentioned above. 32 bits is close to too much memory, considering average file counts. 16 bits is way below the budget, but a max of 16 files per Effect may feel a bit constraining. I am leaning towards 16 bits and will procced that way unless told otherwise.

You could use this class:

ProffieOS/common/arg_parser.h

Line 128 in 1a27ca6

class BitSet {

profezzorn · 2025-09-21T10:19:38Z

sound/effect.h

 #ifdef NO_REPEAT_RANDOM
+  // Storage for random sampling without replacement.
+  bool available_[NO_REPEAT_RANDOM_BUFFER_SIZE_BITS];
+  bool available_mark_ = false;


This needs a better name, or a comment that explains what it is for.

profezzorn · 2025-09-21T10:25:27Z

sound/effect.h

+        for (uint8_t i = 0; i < total_files; ++i) {
+          const int16_t file = sub_files_ ? i / sub_files_ : i;
+          const int16_t subfile = sub_files_ ? i % sub_files_ : 0;
+          n += available_[i] == available_mark_ && (last_ != file || (sub_files_ && last_subid_ != subfile));


Use an if statement here, it will make the code easier to read.

profezzorn · 2025-09-21T10:26:22Z

sound/effect.h

+          n += available_[i] == available_mark_ && (last_ != file || (sub_files_ && last_subid_ != subfile));
+        }
+        if (!n) {
+          available_mark_ = !available_mark_;


Using available_mark_ this way doesn't save anything if you have to iterate over the whole array to check how many elements are available anyways.

It lowers the worst case from 3N to 2N, since the alternative is to iterate over the array one more time to mark everything as available. I agree though, it's not really much of a save since it's still all O(N). The refactor will scrap this anyway, since if available_ is becoming an integer I can just set it back to 0 or max

Marking everything as available only has to be done every Nth time though, so the average time spent ends up being constant.

pedker · 2025-10-02T06:44:58Z

Successfully converted this to use a BitSet, though I had to add a few functions to the BitSet class, mainly to support some subset-based operations. Also, I set the memory size to 32 because BitSets denominate by increments of 32-bit ints, so setting the size to 16 would just waste the other 16 bits.

Something else to note is that I have not been able to test the random_subid() portion of this code, because I cannot get that function to fire under normal circumstances. It's supposed to be called by GetFollowing()...

profezzorn · 2025-10-02T07:09:37Z

common/arg_parser.h

    }
    return ret;
  }
+  size_t popcount_subset(size_t first, size_t last) const {


I'm wondering if it would be easier (and faster) to do all this subset stuff with ands and ors.

Basically something like:

Bitset subset = ~(Bitset::All() << last) & (Bitset::All() << first); size_t popcount_subset = (subset & bitset).popcount();

profezzorn · 2025-10-02T07:12:50Z

sound/effect.h

+        uint8_t n = available_.popcount();
+        if (!n) {
+          available_.fill(total_files);
+          n = available_.popcount();


n = total_files; would be faster

profezzorn · 2025-10-02T07:18:46Z

common/arg_parser.h

      bits_[i] = 0;
    }
  }
+  void fill(size_t bit = SIZE) {


This should be a static function that returns a bitset. It also needs a comment to explain what it does.

profezzorn · 2025-10-02T07:23:48Z

common/arg_parser.h

    }
  }
+  void fill(size_t bit = SIZE) {
+    for (size_t i = 0; i < NELEM(bits_); i++) {


The loop should only iterate over all bits_[] where it can assign 0xFFFFFFFF, then assign the final value after the loop.

pedker · 2025-10-11T08:53:19Z

Needed to add some bitwise operator overloads to support the proposed changes, figured I'd take a shot at making them all while I'm there (except XOR). I'm considering renaming some or all of the instances of subset to instead be range, but I'm on the fence.

profezzorn · 2025-10-11T09:35:53Z

common/arg_parser.h

+    ret >>= bits;
+    return ret;
+  }
+  void operator<<=(int bits) {


We should probably improve these while we're at it, something like:

uint32_t get_word(int word) { if (word < 0 || word >= (int) NELEM(bits_)) return 0; return bits_[word]; } uint32_t get32(int pos) { uint64_t tmp = get_word(1 + (pos >> 5)); tmp <<= 32; tmp |= get_word(pos >> 5); return tmp >> (pos & 31); } BitSet<SIZE> operator>>(int bits) const { BitSet<SIZE> ret; for (int i = 0; i < SIZE; i++) ret.bits_[i]= get32(i*32 + bits); return ret; }

<< would be basically the same, but with a - inside the get32() call.

pedker · 2025-10-16T13:00:04Z

I've implemented the suggestions for the shifters. It compiles, but I currently do not have time to thoroughly test it as I am out of town soon. I will do thorough testing when I return, but on paper this looks pretty good to me

pedker · 2025-10-25T12:39:47Z

Identified a crashing issue and put up a fix. Unfortunately, before I could verify the fix and finish testing, I accidentally ripped the USB port right off of the ProffieBoard of my saber. So, for now, my ability to work on this has been crippled. I'm not sure how long it will take to remedy, as I may have to contact support, try to DIY fix/replace the board on my own, or wait until I can afford to pay for a replacement solution.

pedker · 2025-11-08T11:35:29Z

I have replaced my soundboard! I've done more extensive testing now, everything is behaving as expected now. I was also able to test calling random_subid() and it now works as I expect.

profezzorn · 2025-11-08T18:44:04Z

common/arg_parser.h

+  }
+  void operator<<=(int bits) {
+    if (!bits) return;
+    const size_t n = NELEM(bits_);


This looks like a complicated way to write:

for (int i = NELEM(bits_)-1; i >= 0; i--) {

Fair, I was going to but was afraid of moving away from using size_t

pedker · 2025-11-09T00:34:34Z

I also caught something I did wrong when attempting to address an edge case in fill(), the real issue was that the N input is not 0-indexed, but was being used to access the BitSet without accounting for 0-indexing

NO_REPEAT_RANDOM using Sample Without Replacement

85eed22

2nd attempt

pedker marked this pull request as ready for review September 17, 2025 05:47

profezzorn reviewed Sep 21, 2025

View reviewed changes

Use BitSet instead of Boolean Array

d245579

profezzorn reviewed Oct 2, 2025

View reviewed changes

Add Bitwise Operands to BitSet

3c1970f

profezzorn reviewed Oct 11, 2025

View reviewed changes

Optimize Bitshift Operands (untested)

d4cae8d

Fix Loop in operator>>=

4e35982

Fix Bounds Errors in BitSet, Fix Subset Range in random_subid()

1966c66

profezzorn reviewed Nov 8, 2025

View reviewed changes

pedker added 2 commits November 8, 2025 15:09

Simply Reverse Loop in BitSet

ba51d03

Account for 0-Indexing in fill()

5ad0dd8

Conversation

pedker commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedker commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedker commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedker commented Oct 16, 2025

Uh oh!

pedker commented Oct 25, 2025

Uh oh!

pedker commented Nov 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedker commented Nov 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pedker commented Oct 11, 2025 •

edited

Loading