Guitar Hero III Bot

I made a Guitar Hero III bot for PC. I was able to beat the song "Through the Fire and Flames" on Expert with the bot which can you see here.

If you watched the video, you can see that the bot misses a few notes, which isn't normal for a BOT to do. When you normally see bots playing Guitar Hero, they never miss a single note.

I feel like my code is very flawed and sluggish which hinders my bot's ability to play notes that require fast execution.

I'd like some help optimizing my code so that my bot can match the performance of other bots.

I commented almost all of my code to avoid any confusion but if I left out any information, or you'd like a better explanation of something, please let me know and I'll do my best to answer your question!

```
using System;
using System.Collections.Generic;
using System.Runtime.InteropServices;
using System.Threading;
using System.Windows.Forms;

namespace Testing_Grounds
{
public partial class Form1 : Form
{
// The text file "Timestamps and Notes.txt" is 3722 lines long.
//
//
// Ignore the line numbers! They are not part of the text file. (e.g., "25:")
//
//
// The numbers to the LEFT of the separator "|" is the timestamp.
// This is when the note should be played.
// The time is in ticks.
//
//
// The numbers to the RIGHT of the separator "|" is the note.
// Each note is 6 digits long containing 1s and 0s.
// The first 5 numbers in a note show which keys need to be held down to play the note.
// the LAST number in a note is irrelevant and It's always a 1.
// 1 means, hold down that key.
// 0 means, ignore that key.
// Since this is Guitar Hero, we have 5 buttons that we use to play notes. (Green, Red, Yellow, Blue, and Orange) [G] [R] [Y] [B] [O]
// So, if we were given the note "101011", then we would hold down the Green, Yellow, and Orange keys. (Remember, we ignore

That huge comment block at the top should be an actual comment block:

/*
 * The text file "Timestamps and Notes.txt" is 3722 lines long.
 *
 * Ignore the line numbers! They are not part of the text file. (e.g., "25:")
 *
 * ...
 *
*/

This would make the whole block collapsible, so you or anyone looking at the code wouldn't need to scroll down two entire screens worth of comments just to get to the actual code.

These key constants should be an enum:

private const ScanCodeShort GreenFretKey = ScanCodeShort.KEY_V; // V key
private const ScanCodeShort RedFretKey = ScanCodeShort.KEY_C; // C key
private const ScanCodeShort YellowFretKey = ScanCodeShort.KEY_X; // X key
private const ScanCodeShort BlueFretKey = ScanCodeShort.KEY_Z; // Z key
private const ScanCodeShort OrangeFretKey = ScanCodeShort.LSHIFT; // Left SHIFT key
private const ScanCodeShort StarPowerKey = ScanCodeShort.SPACE; // SPACEBAR
private const ScanCodeShort WhammyKey = ScanCodeShort.KEY_W; // W key
private const ScanCodeShort StrumDownKey = ScanCodeShort.OEM_2; // '/?' key

[Flags]
private enum GuitarKeys
{
    GreenFretButton = ScanCodeShort.KEY_V,
    RedFredButton = ScanCodeShort.KEY_C,
    YellowFretButton = ScanCodeShort.KEY_X,
    BlueFretButton = ScanCodeShort.KEY_Z,
    OrangeFretButton = ScanCodeShort.LSHIFT,
    StarPowerButton = ScanCodeShort.SPACE,
    WhammyBarButton = ScanCodeShort.KEY_W,
    StrumButton = ScanCodeShort.OEM_2 // '?' key
}

Notice the only comment that's actually useful is the last one. The [Flags] attribute indicates that the values can be combined and that you can use bitwise logic and the .HasFlag method to determine the combination of values a GuitarKeys represent; by using a set of constants like you did, you lose this ability and need to resort to conditionals and logical operators instead.

// When button1 is clicked, execute this code.
private void button1_Click(object sender, EventArgs e)
{
    // Store all of the lines in the text file "Timestamps and Notes.txt" into the variable named "lines".
    var lines = System.IO.File.ReadAllLines("Timestamps and Notes.txt");

The comments are overkill. Bad comments state the obvious and tell nothing the code doesn't already say. Good comments say why, and let the code say what. If mere implementation details such as the name of the text file needs to change, you made yourself two places to maintain it in: in the actual code, and in the comment above it. Everywhere. There's so many comments everywhere, it's distracting, and makes the code actually harder to follow because of the constant context-switching.

Your noteDigits have pre-determined and specific significance, that you're referring to with magic numbers - one has to resort to reading the comments to figure out what's what. Make another enum:

private enum NotePosition
{
    Green,
    Red,
    Yellow,
    Blue,
    Orange
}

And then map them to actual keys:

private static readonly IDictionary NoteButtonMap =
    new Dictionary
{
    { NotePosition.Green, GuitarKeys.GreenFretButton },
    { NotePosition.Red, GuitarKeys.RedFretButton },
    { NotePosition.Yellow, GuitarKeys.YellowFretButton },
    { NotePosition.Blue, GuitarKeys.BlueFretButton },
    { NotePosition.Orange, GuitarKeys.OrangeFretButton }
};

And now you can take this repeated chunk:

if (noteDigits[0].ToString() == "1")
{
    nInputs.Add(new INPUT
    {
        type = InputType.KEYBOARD,
        U =
        {
            ki =
            {
                wScan = GreenFretKey
            }
        }
    });
}

And extract it into its own function, as Matt suggested:

private void AddInput(char[] notes, NotePosition notePosition, IList inputs)
{
    if (notes[notePosition] != '1') { return; }

    inputs.Add(new INPUT
    {
        type = InputType.KEYBOARD,
        U =
        {
            ki =
            {
                wScan = NoteButtonMap[notePosition]
            }
        }
    });
}

Being a char[], each index in the notes array corresponds to a char. Why waste cycles in the conversion to a String when you can compare it to a char?

About this sleep:

Thread.Sleep(6);

That 6ms is probably below the clock resolution (MSDN):


The system clock ticks at a specific rate called the clock resolution. The actual timeout might not be exactly the specified timeout, because the specified timeout will be adjusted to coincide with clock ticks.

Per this SO answer, the thread time slice on Windows is 15ms, so it's not clear what that arbitrary 6 really does.

Same here:

Thread.Sleep(1);

You definitely need a function that returns an INPUT[], and since that function will have pre-determined outputs you could reduce the object creation/destruction overhead by keeping an INPUT[] instance around somewhere in static context.

The DllImport declarations should be in a NativeMethods static class, not in the form's code-behind.

Actually..

StackExchange Code Review Q#133131, answer score: 7