add player controller. add debug window for testing. #33

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

@@ -27,6 +23,12 @@ void Player::tick(bave::Seconds const dt) {

 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add player controller. add debug window for testing. #33

add player controller. add debug window for testing. #33

swagween commented Feb 19, 2024

karnkaul Feb 19, 2024

swagween Feb 19, 2024

karnkaul Feb 19, 2024

swagween Feb 19, 2024

karnkaul Feb 20, 2024

karnkaul Feb 19, 2024

swagween Feb 19, 2024

karnkaul Feb 20, 2024

karnkaul Feb 19, 2024

swagween Feb 19, 2024

karnkaul Feb 19, 2024

swagween Feb 19, 2024

karnkaul commented Feb 19, 2024

karnkaul Feb 20, 2024

karnkaul Feb 20, 2024

karnkaul Feb 20, 2024

karnkaul Feb 20, 2024 •

edited

Loading

karnkaul Feb 20, 2024 •

edited

Loading

karnkaul commented Feb 20, 2024

	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"));

		@@ -27,6 +23,12 @@ void Player::tick(bave::Seconds const dt) {

		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{};

add player controller. add debug window for testing. #33

Are you sure you want to change the base?

add player controller. add debug window for testing. #33

Conversation

swagween commented Feb 19, 2024

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

karnkaul commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karnkaul Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

karnkaul Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

karnkaul commented Feb 20, 2024

karnkaul Feb 20, 2024 •

edited

Loading

karnkaul Feb 20, 2024 •

edited

Loading