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

New dice-rolling app: InfiniDice! #1326

Merged
merged 83 commits into from
Jan 23, 2024
Merged

Conversation

yusufmte
Copy link
Contributor

InfiniDice is a dice-rolling applet where you can roll random numbers from d2 to d99. The functionality is:

  • Set the number of "sides" of the dice
  • Roll to randomly generate a result number in the interval from 1 to the number of sides

It starts at d2 (ie coinflip), and when you first open the app, a roll is made, so you get a "free" coinflip. Then you can change the number of sides and use it as a d6, d20, etc. The color of the result alternates with each roll (so that you can be sure it was actually rolled even when the result is the same).

Tested on PineTime hardware with InfiniTime 1.10.0, and found to be in good working order. See video:

infinidice.mp4

I often find myself looking for dice and think this could come in handy for tabletop players :) Any testing, feedback, or suggestions on optimization would be appreciated!

@joseph58tech
Copy link

I believe this has been implemented before, check #565

@minacode
Copy link
Contributor

I like the simple interface and think they solve different problems. Maybe we could have both apps in a world where we are able to choose which apps we want.

@NeroBurner NeroBurner added the new app This thread is about a new app label Sep 13, 2022
Copy link
Contributor

@NeroBurner NeroBurner left a comment

Choose a reason for hiding this comment

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

lovely app ❤️

src/displayapp/screens/Dice.h Outdated Show resolved Hide resolved
src/displayapp/screens/Dice.h Outdated Show resolved Hide resolved
src/displayapp/screens/Dice.h Outdated Show resolved Hide resolved
src/displayapp/screens/Dice.h Outdated Show resolved Hide resolved
@FintasticMan
Copy link
Member

One feature that I think is quite nice to have in a dice app, is being able to specify a number of dice, rather than just always rolling one die. I don't think that for this you would need to show the results of all the dice individually, just the final result.

For the rest I think this app looks good! 👍

@minacode
Copy link
Contributor

#565 is doing exactly that already.

@FintasticMan
Copy link
Member

You're right, it does do that. But it hasn't been updated in quite a while, and so doesn't merge nicely with some of the changes in InfiniTime. The author of that PR also doesn't seem to be very active, so it's uncertain as to whether it will be updated.

@minacode
Copy link
Contributor

This problem is fundamental for InfiniTime. We need to fix the app problem.
Shameless plug: #1296

@yusufmte
Copy link
Contributor Author

yusufmte commented Sep 13, 2022

Thank you @NeroBurner kindly for your improvements on the code ❤️ Thank you everyone for your comments and feedback as well.

@FintasticMan :

One feature that I think is quite nice to have in a dice app, is being able to specify a number of dice, rather than just always rolling one die.

I definitely appreciate your sentiment, rolling multiples is a frequently needed and useful function of dice! 👍 However I tend to agree with @minacode , there's a value to keeping the interface simple and intuitive, and #565 already implements multi-roll in a lovely way. But I might be thinking about ways to implement that smoothly without crowding the UI too much.

@yusufmte
Copy link
Contributor Author

yusufmte commented Sep 15, 2022

Implemented rolling multiple dice in 52e4af5

(Thanks for the suggestion @FintasticMan ! 👍)

The number of dice can range from 1-9 (default 1), and the sides can range from d2-d99 (default d2). The individual rolls are shown as well. Here's the new look:

infinidice_on_infinisim
infinidice_on_pinetime

As before, any feedback or further suggestions are much appreciated. I'll try to keep it up to date with the official develop branch in the meantime. Thanks everyone :)

@yusufmte yusufmte requested a review from NeroBurner September 15, 2022 18:55
@yusufmte
Copy link
Contributor Author

yusufmte commented Jan 10, 2024

Rebased to 1.14! (Very excited about the customizable user app selection implemented in this version of Infinitime.)

I also want to bump this PR in general. It has been ready for a long time, and continuing to work great in 1.14. The shake-to-roll function implemented by @Poohl has been working super well also. This has probably been my most frequently used of the PineTime user apps, and have used it in a few different circles with very good success. Sometimes I have taken the PineTime off the band and passed it around during a tabletop game for players to roll. There have been numerous other PRs for dice apps in the past, so there is definitely interest in such an app from the PineTime community.

I know there are a lot of PRs to review and a lot of more important work to do for Infinitime in general -- all on a volunteer basis, which I recognize and greatly appreciate! However, can the maintainers weigh in on the plans for this at some point? @JF002 Are there plans to add it to the 1.15 milestone? Thanks! :)

Copy link
Member

@FintasticMan FintasticMan left a comment

Choose a reason for hiding this comment

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

I really like this app! A couple of concerns: do we really need the C++ random generator, is it that much better than C's rand? Could you try using rand with the bias elimination implemented in #881, and see what the difference in flash usage is? If the difference isn't too big, or if you think that rand really isn't good enough, I'd be very happy to use the better C++ generator.

Another concern is how well the shaking works when lower to sleep is also enabled. Could you check if there are any issues there? (I'm also not a big fan of the hack of enabling shake to wake if it's disabled to get it to work, but I don't think that needs to be solved now)

Thank you so much for this app, it's very nice, and I apologise for how long it's taking.

src/displayapp/screens/Dice.cpp Outdated Show resolved Hide resolved
src/displayapp/screens/Dice.cpp Outdated Show resolved Hide resolved
src/displayapp/screens/Dice.cpp Outdated Show resolved Hide resolved
src/displayapp/screens/Dice.h Outdated Show resolved Hide resolved
src/displayapp/apps/CMakeLists.txt Show resolved Hide resolved
Copy link
Member

@FintasticMan FintasticMan left a comment

Choose a reason for hiding this comment

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

Just a couple of things.

src/displayapp/screens/Dice.h Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@yusufmte
Copy link
Contributor Author

@FintasticMan thank you for your kind words & thoughtful feedback/review, I really appreciate it!! ❤️ No apology necessary, you folks have been doing amazing work with InfiniTime and I know you have a ton on your plate!

I really like this app! A couple of concerns: do we really need the C++ random generator, is it that much better than C's rand? Could you try using rand with the bias elimination implemented in #881, and see what the difference in flash usage is?

I noticed that rand() is used for RNG in other apps like Paddle and Twos, and so I did initially use it in earlier versions of the Dice. However, there are a few significant flaws with rand() (detailed below) that make it inferior to the C++11 <random> lib. I think that doesn't matter for the other apps mentioned, where the experience is fine as long as it generally feels random-- but for the Dice, since the whole point of the app is randomness, it feels important for the randomness to be very fair / high quality.

(1) According to official docs, rand() is considered deprecated for most needs as it does not guarantee quality of the randomness of the sequence produced. More details here:

There are no guarantees as to the quality of the random sequence produced. In the past, some implementations of rand() have had serious shortcomings in the randomness, distribution and period of the sequence produced (in one well-known example, the low-order bit simply alternated between 1 and 0 between calls). rand() is not recommended for serious random-number generation needs. It is recommended to use C++11's random number generation facilities to replace rand(). (since C++11)

(2) As you mentioned, there is the possibility of modulo bias with rand() % x (where is RAND_MAX is not divisible by x, then generating smaller numbers is more likely). With rand(), it's possible to correct for it with a few extra lines of code, but feels more elegant to instead use the intended standard library function, which picks integers uniformly distributed on a specified closed interval.

(3) The <random> library has the very neat std::seed_seq function used here, which mixes multiple sources of randomness together (as opposed to srand(), which takes in just 1 seed), and makes seeds that are distributed over the whole 32-bit range even when the inputs are close or poorly distributed. This is nice here particularly because the PineTime has a few possible sources of randomness that can work together in tandem; the dice app mixes the system tick count and the X/Y/Z components of the PineTime's sensitive motion controller (credit to @w4tsn #1199 for the motion controller idea!), making seed collision vanishingly unlikely.

Another concern is how well the shaking works when lower to sleep is also enabled. Could you check if there are any issues there? (I'm also not a big fan of the hack of enabling shake to wake if it's disabled to get it to work, but I don't think that needs to be solved now)

That's a great point; I am not sure as I haven't previously used lower to sleep on my own PineTime -- I will test this shortly and report back here soon!

@FintasticMan
Copy link
Member

Thank you for the thorough explanation. I think it shows you've put a lot of thought into this and I agree that using rand() is not well-suited for this application. Let's stick with the C++ random classes you've already implemented then!

@yusufmte
Copy link
Contributor Author

Another concern is how well the shaking works when lower to sleep is also enabled. Could you check if there are any issues there? (I'm also not a big fan of the hack of enabling shake to wake if it's disabled to get it to work, but I don't think that needs to be solved now)

Tested current version on PineTime hardware, it's working well! The lower-to-sleep & raise-to-wake continues to work inside & outside the Dice app when these are enabled, and the shake-to-roll works as well.

Copy link
Member

@FintasticMan FintasticMan left a comment

Choose a reason for hiding this comment

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

Awesome! This looks good to me

@yusufmte
Copy link
Contributor Author

Awesome, thanks again!!! Let me know if anything else needed prior to merge!

@NeroBurner NeroBurner added this to the 1.15.0 milestone Jan 15, 2024
@NeroBurner NeroBurner merged commit a40168a into InfiniTimeOrg:main Jan 23, 2024
7 checks passed
@NeroBurner
Copy link
Contributor

@yusufmte thank you for your hard work and endurance!

We can finally say: Alea iacta est

@yusufmte
Copy link
Contributor Author

@yusufmte thank you for your hard work and endurance!

We can finally say: Alea iacta est

Thank you friend, and many thanks to all who contributed and reviewed along the way! 🎉 Alea iacta est, indeed! :)

lasnikr pushed a commit to lasnikr/LasnikrInfiniTime that referenced this pull request Mar 5, 2024
Add new App `Dice.h` to randomly roll the dice(s).
The number of dice can range from 1-9 (default 1), and the sides can
range from d2-d99 (default d2).

To have a haptic feedback we make Dice vibrate on roll.

Regarding the use of C++ `<random>` library:
There are known problems with `rand()` and `srand()` (see https://en.cppreference.com/w/cpp/numeric/random/rand)
and the `<random>` library is preferred for this reason. The function used from
`<random>` also avoids a very rare bias that would occur using `rand()` and modulo,
when `RAND_MAX` is not a multiple of `d` and the initially generated number falls in
the last "short" segment. This commit also updates the seed to derive entropy
(via `seed_seq`) from a mix of the system tick count and the x,y,z components of the
PineTime motion controller -- taking inspiration from and with credit to @w4tsn
(InfiniTimeOrg#1199)

Thanks for suggestions:
* in Dice, when rolling 1d2, also show "HEADS" or "TAILS" -- suggestion by @medeyko
* ui adjustments and result realignment -- suggestion by @Boteium

---------

Co-authored-by: NeroBurner <[email protected]>
Co-authored-by: Riku Isokoski <[email protected]>
Co-authored-by: Paul Weiß <[email protected]>
Co-authored-by: FintasticMan <[email protected]>
tmaklin pushed a commit to tmaklin/InfiniTime that referenced this pull request Jul 30, 2024
Add new App `Dice.h` to randomly roll the dice(s).
The number of dice can range from 1-9 (default 1), and the sides can
range from d2-d99 (default d2).

To have a haptic feedback we make Dice vibrate on roll.

Regarding the use of C++ `<random>` library:
There are known problems with `rand()` and `srand()` (see https://en.cppreference.com/w/cpp/numeric/random/rand)
and the `<random>` library is preferred for this reason. The function used from
`<random>` also avoids a very rare bias that would occur using `rand()` and modulo,
when `RAND_MAX` is not a multiple of `d` and the initially generated number falls in
the last "short" segment. This commit also updates the seed to derive entropy
(via `seed_seq`) from a mix of the system tick count and the x,y,z components of the
PineTime motion controller -- taking inspiration from and with credit to @w4tsn
(InfiniTimeOrg#1199)

Thanks for suggestions:
* in Dice, when rolling 1d2, also show "HEADS" or "TAILS" -- suggestion by @medeyko
* ui adjustments and result realignment -- suggestion by @Boteium

---------

Co-authored-by: NeroBurner <[email protected]>
Co-authored-by: Riku Isokoski <[email protected]>
Co-authored-by: Paul Weiß <[email protected]>
Co-authored-by: FintasticMan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new app This thread is about a new app
Projects
None yet
Development

Successfully merging this pull request may close these issues.