Trying to use command pattern breaks my code

[jhbardwell]

I'm trying to make a small game engine in SDL2 based around enum switch state machines because they make things easier to organize, interpret, and less messy. At least they should, in theory, once I understand them better.

I've got the GUI mostly working. It renders and the button pressing to switch GUI states works as expected with the exception of one major bug I'm trying to address in a separate question regarding removing functionality from the invisible button(s). And one minor bug related to the issue at hand: linking a button press to the exit command doesn't actually break the loop, yet. It's there for when I get these kinks ironed out and can set up a working "Exit" button.

When I try to introduce a command pattern to decouple the input handling from the gui state changes, the code completely breaks. No error or output messages whatsoever. Nothing renders at all. I can't figure out what's wrong, and moving the command enum switch hither and yon does nothing.

Broken Code Changes using "Menu" Button as example

enum CommandState { INIT_MENU };
CommandState commandState;

//Menu Button
if (mouseOverRect(mouse_x, mouse_y, menubutton))
{
    commandState = INIT_MENU;
    useMenu_Clip = MENU_DEFAULT;
}

switch (commandState)
{
    case INIT_MENU:
    guiState = GUI_MENU;
    break;
}

Original Somewhat Working Code

  //Variables

int Screen_Width  = 720;
int Screen_Height = 407;

const int LONGBUTTON_HEIGHT = 128;
const int LONGBUTTON_WIDTH = 256;

int mouse_x;
int mouse_y;

struct Graphic
{
	std::string name;
	std::string type;
	std::string flavortext;
	int x;
	int y;
	int h;
	int w;
};

//long button states
enum MenuButtonState { MENU_DEFAULT, MENU_HOVER, MENU_PRESSED, MENU_TOTAL };
enum NewGameButtonState { NEW_DEFAULT, NEW_HOVER, NEW_PRESSED, NEW_TOTAL };

//command states
enum CommandState { INIT_QUIT);
CommandState commandstate;

//gui states
enum GUIState { GUI_MENU, GUI_PLAY};
GUIState guiState;

//Initialization
SDL_Window* window = NULL;										
SDL_Renderer* renderer = NULL;
SDL_Rect* clip = NULL;

//Log Error Function
void logSDLError(std::ostream &os, const std::string &msg)
{
	os << msg << " error: " << SDL_GetError() << std::endl;
}

//Mouse Over Rectangle Function
bool mouseOverRect(int x, int y, Graphic &graphic)
{
	if ((x <= graphic.x + graphic.w) && (x > graphic.x) &&
		(y <= graphic.y + graphic.h) && (y > graphic.y))
		return true;
	else
		return false;
}

// Loads an image into a texture on the rendering device
SDL_Texture* loadTexture(const std::string &file, SDL_Renderer *renderer)
{
	SDL_Texture *texture = IMG_LoadTexture(renderer, file.c_str());
	if (texture == nullptr)
	{
		logSDLError(std::cout, "Cannot load texture.");
	}
	return texture;
}

//Draw an SDL_Texture to an SDL_Renderer at position x, y and size h, w
void renderTexture(SDL_Texture* texture, SDL_Renderer* renderer, Graphic &graphic,
	SDL_Rect *clip = nullptr)
{
	SDL_Rect dest;
	dest.x = graphic.x;
	dest.y = graphic.y;
	dest.h = graphic.h;
	dest.w = graphic.w;

	SDL_RenderCopy(renderer, texture, clip, &dest);
}

// Main Function
int main(int, char**)
{
	//Start up SDL
	if (SDL_Init(SDL_INIT_EVERYTHING) != 0)
	{
		logSDLError(std::cout, "Cannot initialize SDL video.");
	}

	//Setup window and renderer
	SDL_Window *window = SDL_CreateWindow("Dragon Hunt", SDL_WINDOWPOS_CENTERED,
			SDL_WINDOWPOS_CENTERED, Screen_Width, Screen_Height, SDL_WINDOW_RESIZABLE);
	if (window == nullptr)
	{
		logSDLError(std::cout, "Cannot create window.");
	}
	SDL_Renderer *renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC);
	if (renderer == nullptr)
	{
		logSDLError(std::cout, "Cannot create renderer.");
	}

	Graphic menubutton;
	menubutton.name = "Main Menu";
	menubutton.type = "longbutton";
	menubutton.x = 0;
	menubutton.y = Screen_Height - (Screen_Width / 10);
	menubutton.h = Screen_Width / 10;
	menubutton.w = Screen_Width / 5;

	Graphic newbutton;
	newbutton.name = "New Game";
	newbutton.type = "longbutton";
	newbutton.x = 0;
	newbutton.y = 5;
	newbutton.h = Screen_Width / 10;
	newbutton.w = Screen_Width / 5;

	//Load long button spritesheet texture
	SDL_Texture* longbutton_image = loadTexture("longbuttonSpriteSheet.png", renderer);
	if (longbutton_image == nullptr)
	{
		logSDLError(std::cout, "Cannot load long button spritesheet.");
	}

	//Menu Button Setup
	SDL_Rect menu_clips[MenuButtonState::MENU_TOTAL];
	for (int i = 0; i < MenuButtonState::MENU_TOTAL; i++)
	{
		menu_clips[i].x = i * LONGBUTTON_WIDTH;
		menu_clips[i].y = 0;
		menu_clips[i].w = LONGBUTTON_WIDTH;
		menu_clips[i].h = LONGBUTTON_HEIGHT;
	}
	int useMenu_Clip = MENU_DEFAULT;

	// New Game Button Setup
	SDL_Rect new_clips[NewGameButtonState::NEW_TOTAL];
	for (int i = 0; i < NewGameButtonState::NEW_TOTAL; i++)
	{
		new_clips[i].x = i * LONGBUTTON_WIDTH;
		new_clips[i].y = 1 * LONGBUTTON_HEIGHT;
		new_clips[i].w = LONGBUTTON_WIDTH;
		new_clips[i].h = LONGBUTTON_HEIGHT;
	}
	int useNew_Clip = NEW_DEFAULT;

	// Initial GUI State
	guiState = GUI_MENU;

	//Event Handler
	SDL_Event e;
	//For tracking if we want to quit
	while (!INIT_EXIT) 
	{
		while (SDL_PollEvent(&e))
		{
			switch (e.type)
			{
			case SDL_MOUSEBUTTONUP:
				mouse_x = e.button.x;
				mouse_y = e.button.y;

				//Menu Button
				if (mouseOverRect(mouse_x, mouse_y, menubutton))
				{
					guiState = GUI_MENU;
					useMenu_Clip = MENU_DEFAULT;
				}

				//New Game Button
				if (mouseOverRect(mouse_x, mouse_y, newbutton))
				{
					guiState = GUI_PLAY;
					useNew_Clip = NEW_DEFAULT;
				}
			}
		}

		switch (guiState)
		{
		case GUI_MENU:
			SDL_RenderClear(renderer);
			renderTexture(longbutton_image, renderer, newbutton, &new_clips[useNew_Clip]);
		SDL_RenderPresent(renderer);
		break;

		case GUI_PLAY:
			SDL_RenderClear(renderer);
			renderTexture(longbutton_image, renderer, menubutton, &menu_clips[useMenu_Clip]);
		SDL_RenderPresent(renderer);
		break;
		}

		switch (commandState)
		{
		case INIT_QUIT:
			SDL_QUIT;
			break;
		}
	}
}

[mbozzi]

Maybe this example will be of use

#include <SDL2/SDL.h>
#include <SDL2/SDL_image.h>

SDL_Window* window = nullptr;
SDL_Renderer* renderer = nullptr;

struct user_input { int pointer_x = 0, pointer_y = 0; unsigned int mouse_state; bool made_selection_this_frame; };

[[nodiscard]] static bool constexpr within_aabb(int a, int b, int x, int y, int w, int h)
{ return (a > x && a < x + w) && (b > y && b < y + h); }

[[nodiscard]] static bool button(user_input ip, int x, int y, int w, int h)
{
	bool clicked = false; 
	if (within_aabb(ip.pointer_x, ip.pointer_y, x, y, w, h))
	{
		if (ip.made_selection_this_frame) // clicked
			clicked = true;
		if (ip.mouse_state & SDL_BUTTON(SDL_BUTTON_LEFT)) // active
			SDL_SetRenderDrawColor(renderer, 0x80, 0x80, 0x80, 0xff);
		else //hovered
			SDL_SetRenderDrawColor(renderer, 0xcc, 0xcc, 0xcc, 0xff);
	}
	else // inactive
		SDL_SetRenderDrawColor(renderer, 0xff, 0xff, 0xff, 0xff);


	SDL_Rect const button_rect{ x, y, w, h };
	SDL_RenderFillRect(renderer, &button_rect);

	return clicked;
}

// Main Function
int main()
{
	SDL_Init(SDL_INIT_VIDEO);
	window = SDL_CreateWindow("Dragon Hunt", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 800, 600, 0);
	renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC);

	user_input ip;
	for (SDL_Event e;;)
	{
		SDL_SetRenderDrawColor(renderer, 0x00, 0x00, 0x00, 0xff);
		SDL_RenderClear(renderer);

		ip.made_selection_this_frame = false;
                // todo: check that it was the left mouse button that was released in this if statement
		while (SDL_PollEvent(&e)) if (e.type == SDL_MOUSEBUTTONUP)  ip.made_selection_this_frame = true;
		ip.mouse_state = SDL_GetMouseState(&ip.pointer_x, &ip.pointer_y);
		
		if (button(ip, 20, 20, 100, 40))
		{
			SDL_ShowSimpleMessageBox(0x00, "Button clicked", "You clicked the demo button.", window);
			break;
		}

		SDL_RenderPresent(renderer);
	}

	return 0;
}

[jhbardwell]

I'm sorry, I just started C++ a few months ago. I understand a little of your code? I think I got the gist except for how struct user_input actually works and what [[no discard]] is doing. It looks like you're replacing the input enum switch with what will become a long chain of if else statements as I tacked on more and more buttons? I'm not sure though. In the "real" code, I'm up to 45 buttons at the moment each with three or four different button states depending on the type of button, which are each in turn affected by three different mouse events. The enums keep things neat, organized, and easy to parse. At least, for me.

Unfortunately, the part you changed is the part that works. Unless the buttons are on top of each other, or suffer that invisible-yet-still-functional problem due to the UI rendering/UI logic decoupling issue, I can hover over and click the buttons just fine. Each button state will then "animate" as it switches from one state to the other and/or trigger init commands.

As a loop, Event Poll(mouse state init) >>Mouse input enum switch (Button state init)>> Button enum switch (GUI state init) >> GUI enum switch (render image init) mostly works. Introducing a command pattern is what throws in the monkey wrench. Event Poll(mouse state init) >>Mouse input enum switch (Button state init)>> Button enum switch (Command state init)>> Command enum switch (GUI state init)>> GUI enum switch (render image init) does not work. It should work. An oft repeated phrase in this business, I'm sure. It uses exactly the same code logic as the working version. I am trying to figure out why adding that particular curlicue makes my code break.

I wanted to include both working and broken code versions, but I passed over the limit, hence that awkward working code/broken changes bit. Even the pared down version of this loop is getting way too big. Chopping the code into function based cpp with a small core loop running the show is definitely on my wish list.

[mbozzi]

I think I got the gist except for how struct user_input actually works

user_input aggregates the things the user did that are interesting. In the button example the interesting things are
a.) what was the location and state of the mouse
b.) did the user release the left mouse button during this frame

...what is [[nodiscard]] doing.

In this case [[nodiscard]] indicates that the result of the function should not be discarded. The compiler produces a diagnostic if the result is ignored anyway.
Very few language features produce such a substantial improvement to product quality with so little effort.
See:
https://en.cppreference.com/w/cpp/language/attributes/nodiscard

I'm up to 45 buttons at the moment each with three or four different button states depending on the type of button, which are each in turn affected by three different mouse events.

I'd like to look at some code, preferably the real, broken version. This might help me understand what the problem is. If it is too large to post, consider using a service like pastebin
https://pastebin.com/

The example is contrived but it tries to illustrate two points:
1. The user can never interact with a button that isn't being drawn.
2. The button does not enact game logic in response to clicks.

The purpose of the event loop is to collect all the input that must be accounted for in the next frame. It doesn't make any sense to handle game logic while you're still aggregating input. Finish figuring out what the user did and only then process it once per frame.

linking a button press to the exit command doesn't actually break the loop, yet. It's there for when I get these kinks ironed out and can set up a working "Exit" button.

What I understand is there is effectively a boolean named still_playing (for example) that controls the main loop. To toggle it indirection is being added in the form of the command pattern, where
- if the exit button is pressed an exit command is issued;
- if an exit command is processed still_playing is assigned false
A better process to implement is the trivial process where
- if the exit button is pressed still_playing is assigned false.