Refactor lighthouse #4

NikolaJelic · 2025-06-17T22:38:47Z

Create Lighthouse class
Add texture rendering with placeholder texture

- Create Lighthouse class - Add texture rendering with placeholder texture

assets/images/lighthouse.png

src/lighhouse.hpp

src/lighthouse.cpp

src/lighhouse.hpp

karnkaul · 2025-06-18T04:22:55Z

src/lighthouse.cpp

+	m_texture = asset_loader.load_texture("images/lighthouse.png");
+	m_sprite.texture = &m_texture.value();
+}
+void Lighthouse::rotate_towards_cursor(glm::vec2 const& cursor_pos) {


Nit: add empty line between functions

src/lighthouse.cpp

src/lighhouse.hpp

karnkaul

Need to fix incorrect unit vector

src/lighthouse.cpp

- rotate lighthouse image so it better follows the cursor - adjust whitespace - fix unit vector typo

src/enemy.cpp

Add new util/random

- add targeted movement - change spawn point generation

karnkaul · 2025-06-20T19:09:12Z

src/enemy.cpp

+	glm::vec2 direction = glm::normalize(m_target_pos - m_sprite.transform.position);
+	glm::vec2 movement = direction * m_move_speed * dt.count();


Nit: locals should be const unless being mutated after construction

karnkaul · 2025-06-20T19:09:20Z

src/enemy.cpp

+	float radius = std::max(screen_size.x, screen_size.y) / 2.0f;
+	float angle = util::random_range(0.0f, 2.0f * std::numbers::pi_v<float>);


karnkaul · 2025-06-20T19:10:33Z

src/enemy.hpp


 	void render(le::Renderer& renderer) const;
-	void move();
+	void move(kvf::Seconds dt);


Change the name to tick() , and don't use the identifier move in C++, it might conflict with std::move(), despite being in different namespaces. Also looks weird (use translate() or something instead).

karnkaul · 2025-06-20T19:11:02Z

src/enemy.hpp


  private:
-	glm::vec2 generate_spawn_pos(glm::vec2 screen_size);
+	static glm::vec2 generate_spawn_pos(glm::vec2 screen_size);


Instead of a static member function make this a free function in an unnamed namespace in the cpp file

karnkaul · 2025-06-20T19:11:17Z

src/enemy.hpp

 	glm::vec2 m_target_pos;
+	float m_move_speed;


Uninitialized primitives (use {})

karnkaul · 2025-06-20T19:11:29Z

src/game.cpp

@@ -1,10 +1,11 @@
 #include <game.hpp>
 #include <glm/gtx/norm.hpp>
 #include <le2d/context.hpp>
+#include "glm/ext/vector_float2.hpp"


Why is this needed?

karnkaul · 2025-06-20T19:13:56Z

src/enemy.hpp

+namespace miracle {
+class Enemy {
+  public:
+	explicit Enemy(gsl::not_null<le::ServiceLocator const*> services, glm::vec2 target_pos, float move_speed);


I'd suggest moving target_pos and move_speed params into a struct and take an instance of that in the constructor. A few benefits:

Constructor signature doesn't change even if the struct members evolve over time

Much more readable at call site: {.move_speed = 50.0f, .target_pos = {1.0f, 2.0f}} vs (50.0f, {1.0f, 2.0f})

Callers can memoize params into their own objects that can be reused

karnkaul · 2025-06-20T19:14:24Z

src/enemy.hpp

+	glm::vec2 m_target_pos;
+	float m_move_speed;


Uninitialized members (use {})

- move spawn point position generation to the 'util::random' namespace - add default initialization to primitives - remove redudant includes - add 'const' to local variables where appropriate - rename 'move' function to avoid naming conflicts

karnkaul · 2025-06-21T15:38:42Z

src/enemy.cpp

@@ -0,0 +1,27 @@
+#include <enemy.hpp>
+#include <algorithm>
+#include "glm/ext/vector_float2.hpp"


Why do these random glm headers keep popping up in your PR?

karnkaul · 2025-06-21T15:40:24Z

src/util/random.hpp

+// Returns a random coordinate on the specified radius
+[[nodiscard]] inline auto get_random_location_on_radius(float radius) -> glm::vec2 {
+	float const angle = random_range(0.0f, 2.0f * std::numbers::pi_v<float>);
+	return {radius * std::cos(angle), radius * std::sin(angle)};


std::cos requires <cmath>

karnkaul · 2025-06-21T15:40:38Z

src/util/random.hpp

@@ -0,0 +1,44 @@
+#pragma once
+#include <glm/ext/vector_float2.hpp>


Refactor lighthouse

7cef3ab

- Create Lighthouse class - Add texture rendering with placeholder texture

NikolaJelic requested a review from karnkaul June 17, 2025 22:38

NikolaJelic self-assigned this Jun 17, 2025

karnkaul reviewed Jun 18, 2025

View reviewed changes

karnkaul requested changes Jun 18, 2025

View reviewed changes

src/lighthouse.cpp Outdated Show resolved Hide resolved

Refacotr PR requests

d87ec64

- rotate lighthouse image so it better follows the cursor - adjust whitespace - fix unit vector typo

NikolaJelic requested a review from karnkaul June 18, 2025 20:06

Add very basic enemy spawn and render

e4360a0

karnkaul requested changes Jun 18, 2025

View reviewed changes

src/enemy.cpp Outdated Show resolved Hide resolved

src/enemy.cpp Outdated Show resolved Hide resolved

karnkaul requested changes Jun 18, 2025

View reviewed changes

src/enemy.cpp Outdated Show resolved Hide resolved

karnkaul and others added 4 commits June 19, 2025 18:22

Add util/random, demo in app (#5)

972e408

Merge branch 'main' into nikola/enemy

c7ab5f6

Add new util/random

Refactor Enemy

30c3153

- add targeted movement - change spawn point generation

Include <numbers> to fix compilation on Win

c1b20c7

NikolaJelic requested a review from karnkaul June 20, 2025 17:27

karnkaul requested changes Jun 20, 2025

View reviewed changes

Refactor small issues

3e86574

- move spawn point position generation to the 'util::random' namespace - add default initialization to primitives - remove redudant includes - add 'const' to local variables where appropriate - rename 'move' function to avoid naming conflicts

NikolaJelic marked this pull request as draft June 21, 2025 13:48

NikolaJelic added 2 commits June 21, 2025 15:51

Add missing namespace

8ec1158

Change function signature

5862bef

karnkaul requested changes Jun 21, 2025

View reviewed changes

		glm::vec2 direction = glm::normalize(m_target_pos - m_sprite.transform.position);
		glm::vec2 movement = direction * m_move_speed * dt.count();

		float radius = std::max(screen_size.x, screen_size.y) / 2.0f;
		float angle = util::random_range(0.0f, 2.0f * std::numbers::pi_v<float>);

		@@ -0,0 +1,44 @@
		#pragma once
		#include <glm/ext/vector_float2.hpp>

Refactor lighthouse #4

Are you sure you want to change the base?

Refactor lighthouse #4

Conversation

NikolaJelic commented Jun 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

karnkaul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!