Add RhythmGame sample #75

dturner · 2018-03-29T16:53:45Z

Adding a new sample which shows how to create a simple musical game. Please read the README before reviewing:

https://github.com/google/oboe/tree/rhythmgame/samples/RhythmGame

The main files which need reviewing are:

samples/RhythmGame/README.md
samples/RhythmGame/src/main/cpp/*

mnaganov

Sorry for a delay. This is a big change.

mnaganov · 2018-04-02T17:42:12Z

samples/RhythmGame/src/main/cpp/Game.cpp

+    mMixer.addTrack(mClap);
+    mMixer.addTrack(mBackingTrack);
+
+    mClapEvents.push(0);


Please provide a comment why we are doing this.

mnaganov · 2018-04-02T17:43:39Z

samples/RhythmGame/src/main/cpp/Game.cpp

+    mClapWindows.push(144000);
+
+    AudioStreamBuilder builder;
+    builder.setFormat(AudioFormat::I16);


Since setters of AudioStreamBuilder return a pointer to it, it's possible to chain these calls.

True but chaining the calls makes the code less maintainable because it's slightly more difficult to add and remove stream properties. Is there any advantage, other than not prefixing with builder.?

Omitting 'builder' removes clutter, so the code is easier to read. If you choose the first parameter to be something that is always needed to be set, and use a line per chained call, then adding / removing setters is no more difficult than in your version. Compare:

builder.setA();
builder.setB();
builder.setC();

builder.setA()
->setB()
->setC();

[Personally I would prefer setters that return "*this", so it's always "." prefix, but I guess we are stuck with what we have.]

But finally, up to you—there is no style guide for this.

When you add a setter you need to remove the trailing ; from the last line (in this case ->setC(); so there is some marginal extra overhead. Also, I find the difference between the initial . and subsequent ->s to be more visually cluttering (and slightly more confusing: "oh wait, a set method returns a pointer?") than the multiple instances of builder.. Not that bothered either way, but will probably leave as is.

Sure. I agree that returning a pointer from the setters hurts the looks of client code.

mnaganov · 2018-04-02T17:44:39Z

samples/RhythmGame/src/main/cpp/Game.cpp

+
+    builder.openStream(&mAudioStream);
+
+    mAudioStream->setBufferSizeInFrames(mAudioStream->getFramesPerBurst() * 2);


Are you deliberately skipping checks for stream validity here?

I was deliberately skipping them when I live coded this example, however I shouldn't in this official sample. Fixed.

mnaganov · 2018-04-02T17:45:37Z

samples/RhythmGame/src/main/cpp/Game.cpp

+    if (mClapWindows.pop(nextClapWindowFrame)){
+
+        int64_t frameDelta = nextClapWindowFrame - mCurrentFrame;
+        int64_t timeDelta = convertFramesToMillis(frameDelta, 48000);


48000 should be some game-wide constant since you even mention it in the project description as the current limitation.

mnaganov · 2018-04-02T17:46:12Z

samples/RhythmGame/src/main/cpp/Game.cpp

+    if (mUiEvents.pop(r)) {
+        renderEvent(r);
+    } else {
+        SetGLScreenColor(GREY);


This should be a constant in case anyone would like to customize the color scheme.

mnaganov · 2018-04-02T18:27:28Z

samples/RhythmGame/src/main/cpp/utils/logging.h

+#include <android/log.h>
+#include <vector>
+
+#define LOGD(...) ((void)__android_log_print(ANDROID_LOG_DEBUG, "rhythm_game", __VA_ARGS__))


Create a define for the tag name.

mnaganov · 2018-04-02T18:28:36Z

samples/RhythmGame/src/main/java/com/donturner/rhythmgame/GameSurfaceView.java

+
+    @Override
+    public void surfaceDestroyed(SurfaceHolder holder) {
+        super.surfaceDestroyed(holder);


Does it make sense to run our code first, and then the base class code in this case?

Yes, it probably does. Changed.

mnaganov · 2018-04-02T18:29:05Z

samples/RhythmGame/src/main/java/com/donturner/rhythmgame/GameSurfaceView.java

+        super(context);
+
+        // Create an OpenGL ES context
+        /*if(BuildConfig.FLAVOR.equals("GL_3"))


Please remove commented out code for clarity.

mnaganov · 2018-04-02T18:29:22Z

samples/RhythmGame/src/main/java/com/donturner/rhythmgame/RendererWrapper.java

@@ -0,0 +1,41 @@
+/*
+ * Copyright 2017 Google Inc.


2018, AOSP?

mnaganov · 2018-04-02T18:30:45Z

samples/RhythmGame/test/testLockFreeQueue.cpp

+ *  - read/writes correct after wraparound (takes too long to run)
+ *  - Constructing queue with non power of 2 capacity results in assert failure (needs death test)
+ *  - Constructing queue with signed type results in assert failure (needs death test)
+ *  - Multi-threaded read/writes (complicated to test)


MT is essential to test here, otherwise there is a big risk of running into very confusing problems at run time only.

mnaganov

Looks good, just a few more comments.

Please decide what to do with your queue. It seems that you are planning to replace it with the one from Oboe.

mnaganov · 2018-04-05T17:54:51Z

samples/RhythmGame/src/main/cpp/Game.h

+// This defines the size of the tap window in milliseconds. For example, if defined at 100ms the
+// player will have 100ms before and after the centre of the tap window to tap on the screen and
+// be successful
+constexpr int kWindowCenterOffset = 100;


Please add "Ms" suffix.

mnaganov · 2018-04-05T17:55:07Z

samples/RhythmGame/src/main/cpp/Game.h

+#include "utils/LockFreeQueue.h"
+#include "utils/UtilityFunctions.h"
+
+constexpr int kSampleRate = 48000; // Fixed sample rate, see README


Please add "Hz" suffix.

mnaganov · 2018-04-05T17:56:00Z

samples/RhythmGame/src/main/cpp/audio/RenderableAudio.h



 #include <cstdint>

 class RenderableAudio {

 public:
+    virtual ~RenderableAudio(){};


nit: It is recommended to put "= default" instead of using empty body.

Done. nits = better code 👍

mnaganov · 2018-04-05T17:56:37Z

samples/RhythmGame/src/main/cpp/Game.cpp

+    mClapWindows.push(144000);
+
+    AudioStreamBuilder builder;
+    builder.setFormat(AudioFormat::I16);


Sure. I agree that returning a pointer from the setters hurts the looks of client code.

Updating various READMEs

ead0672

dturner requested review from mnaganov and philburk March 29, 2018 16:53

mnaganov reviewed Apr 2, 2018

View reviewed changes

Addressing review comments

0738494

mnaganov reviewed Apr 5, 2018

View reviewed changes

dturner added 3 commits April 6, 2018 12:34

Adding concurrency test

45996c2

Solving concurrency problem

ab38cf2

Fixing variable names

553af95

mnaganov approved these changes Apr 9, 2018

View reviewed changes

dturner merged commit 7e69661 into master Apr 10, 2018

dturner deleted the rhythmgame branch April 10, 2018 11:58

Hyyzt mentioned this pull request Aug 18, 2022

the drumthumper have a crash #1608

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RhythmGame sample #75

Add RhythmGame sample #75

dturner commented Mar 29, 2018

mnaganov left a comment

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 3, 2018

dturner Apr 4, 2018

mnaganov Apr 5, 2018

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 2, 2018

dturner Apr 3, 2018

mnaganov Apr 2, 2018

dturner Apr 6, 2018

mnaganov left a comment

mnaganov Apr 5, 2018

dturner Apr 6, 2018

mnaganov Apr 5, 2018

dturner Apr 6, 2018

mnaganov Apr 5, 2018

dturner Apr 6, 2018

mnaganov Apr 5, 2018


		builder.openStream(&mAudioStream);

		mAudioStream->setBufferSizeInFrames(mAudioStream->getFramesPerBurst() * 2);

Add RhythmGame sample #75

Add RhythmGame sample #75

Conversation

dturner commented Mar 29, 2018

mnaganov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaganov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment