add player controller. add debug window for testing. by swagween · Pull Request #33 · cpp-gamedev/DogTales

swagween · 2024-02-19T18:31:40Z

added a PlayerController class and gave one to m_player. Also added a small debug menu with ImGui for testing.

karnkaul · 2024-02-19T19:05:40Z

DogTales/dog/dogtales.cpp

+				if (ImGui::BeginTabItem("Player")) {
+					bave::im_text("Controller States");
+					ImGui::Separator();
+					ImGui::Text("Test (Press 'T'): %.2f", m_player.get_controller_state("test"));


Suggested change

ImGui::Text("Test (Press 'T'): %.2f", m_player.get_controller_state("test"));

bave::im_text("Test (Press 'T'): {:.2f}", m_player.get_controller_state("test"));

karnkaul · 2024-02-19T19:06:40Z

DogTales/dog/player.cpp


 void Player::draw(bave::Shader& shader) const { m_sprite.draw(shader); }

+float const Player::get_controller_state(std::string key) {


Returning a float const is redundant, compiler/linter should be warning about this.

should I just make it float?

karnkaul · 2024-02-19T19:07:16Z

DogTales/dog/player.cpp

+	try {
+		return m_player_controller.key_map[key];
+	} catch (std::out_of_range const& oor) { return -1.f; }


We don't want to risk exceptional paths in hot code like this.

what should i do instead? just return the element? or use if()?

First of all the member function should be const. Once that's done key_map[key] will not compile anyway. So you'd want to use key_map.find(key) and return a default value / nullopt if the key isn't present.

karnkaul · 2024-02-19T19:08:01Z

DogTales/dog/player.hpp

 	void draw(bave::Shader& shader) const;
+
+	float const get_controller_state(std::string key);


Taking a std::string_view would optimize calls made with literals.

karnkaul · 2024-02-19T19:09:34Z

DogTales/dog/player/PlayerController.hpp

+
+	void tick(bave::Seconds dt, const bave::App& app);
+
+	std::unordered_map<std::string, float> key_map{};


This shouldn't be public, as anybody can modify it. Also, use an annoyingly long form to enable string_view lookups:

#include <bave/core/string_hash.hpp> // ... std::unordered_map<std::string, float, bave::StringHash, std::equal_to<>> key_map{};

okay, fixed!

karnkaul · 2024-02-19T19:20:40Z

Also, player::PlayerController is a bit much, one of the prefixes is sufficient. 😅

…dd actual keymaps, and update debug menu accordingly.

karnkaul · 2024-02-20T13:55:48Z

DogTales/dog/player/PlayerController.cpp

+std::optional<float> PlayerController::get_controller_state(std::string_view key) const {
+	if (auto search = key_map.find(key); search != key_map.end()) {
+		return search->second;
+	} else {
+		return -1.f;
+	}
+}


Two points:

Since this always returns a float, there's not much point in wrapping it in std::optional. Even in the user code above you don't check if the returned optional has a value, just call .value() anyway. Returning a float will eliminate that redundancy.

The default returned value should be 0.0f not -1.0f, because that implies: eg "pulling left all the time" if interpreted as a horizontal direction for a key that isn't mapped. 😄

Nitpick: else is redundant if all paths in the if above it return.

karnkaul · 2024-02-20T13:56:56Z

DogTales/dog/player/PlayerController.hpp

+	std::optional<float> get_controller_state(std::string_view key) const;
+
+  private:
+	std::unordered_map<std::string, float, bave::StringHash, std::equal_to<>> key_map{};


Nitpick: non-public members have the m_ prefix everywhere else.

karnkaul · 2024-02-20T14:02:53Z

DogTales/dog/player/PlayerController.cpp

+void PlayerController::tick(bave::Seconds dt, bave::App const& app) {
+
+	auto const& key_state = app.get_key_state();
+
+	bool left = key_state.is_pressed(bave::Key::eA) || key_state.is_pressed(bave::Key::eLeft);
+	bool right = key_state.is_pressed(bave::Key::eD) || key_state.is_pressed(bave::Key::eRight);
+	bool up = key_state.is_pressed(bave::Key::eW) || key_state.is_pressed(bave::Key::eUp);
+	bool down = key_state.is_pressed(bave::Key::eS) || key_state.is_pressed(bave::Key::eDown);
+
+	key_map["move_x"] = 0.f;
+	key_map["move_x"] = left && !right ? -1.f : key_map["move_x"];
+	key_map["move_x"] = right && !left ? 1.f : key_map["move_x"];
+	key_map["move_x"] = right && left ? 0.f : key_map["move_x"];
+
+	key_map["move_y"] = 0.f;
+	key_map["move_y"] = down && !up ? -1.f : key_map["move_y"];
+	key_map["move_y"] = up && !down ? 1.f : key_map["move_y"];
+	key_map["move_y"] = up && down ? 0.f : key_map["move_y"];
+
+	key_map["jump"] = key_state.is_pressed(bave::Key::eW) || key_state.is_pressed(bave::Key::eUp) ? 1.f : 0.f;
+}


A few things to refactor here.

Don't call key_map["move_x"] a dozen times - it will perform the hash, comparison, and lookup each time. Instead get a reference once and operate on the reference throughout the function.

The left / right logic is unnecessarily complicating things, instead just append the delta to a temp float, and clamp it between -1.0f and 1.0f at the end.

This is still pretty hard-coded and there's no indirection for which key corresponds to which game-action. I think at this level it's better for the logic to be generic: the controller should cycle through its mappings and update the key_map according to the current key_state, without knowing how many mappings there are / what they are.

karnkaul · 2024-02-20T14:18:35Z

DogTales/dog/player/PlayerController.cpp

Filename should be in snake_case like others.

karnkaul · 2024-02-20T14:55:47Z

@swagween I've created a PR targeting this branch, feel free to merge it or adopt the changes yourself and close it.

add player controller. add debug window for testing.

ee94aee

swagween requested a review from karnkaul February 19, 2024 18:33

karnkaul requested changes Feb 19, 2024

View reviewed changes

swagween added 2 commits February 20, 2024 14:37

make key_map access const. fix type errors. remove test key_map and a…

ba0eb54

…dd actual keymaps, and update debug menu accordingly.

fix formatting

69c1e01

swagween requested a review from karnkaul February 20, 2024 13:40

karnkaul requested changes Feb 20, 2024

View reviewed changes

karnkaul reviewed Feb 20, 2024

View reviewed changes

DogTales/dog/player/PlayerController.cpp Outdated

Copy link

Member

karnkaul Feb 20, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

Filename should be in snake_case like others.

Refactor PlayerController. (#35)

b08e372

karnkaul approved these changes Feb 25, 2024

View reviewed changes

	ImGui::Text("Test (Press 'T'): %.2f", m_player.get_controller_state("test"));
	bave::im_text("Test (Press 'T'): {:.2f}", m_player.get_controller_state("test"));


		void Player::draw(bave::Shader& shader) const { m_sprite.draw(shader); }

		float const Player::get_controller_state(std::string key) {

		void draw(bave::Shader& shader) const;

		float const get_controller_state(std::string key);


		void tick(bave::Seconds dt, const bave::App& app);

		std::unordered_map<std::string, float> key_map{};

Conversation

swagween commented Feb 19, 2024

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!

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!

karnkaul commented Feb 19, 2024

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!

karnkaul Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karnkaul Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karnkaul commented Feb 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

karnkaul Feb 20, 2024 •

edited

Loading

karnkaul Feb 20, 2024 •

edited

Loading