add physics component #29

swagween · 2024-01-30T14:51:13Z

add a component namespace and create a Physics class. the player gets a physics component, and its position is updated based on the physics state.

…e sprite position.

swagween · 2024-01-30T14:54:14Z

DogTales/src/components/physics.cpp

+	m_acceleration.y += m_gravity * dt;
+	m_velocity = (m_velocity + (m_acceleration / m_mass) * dt) * m_friction;
+
+	m_velocity.x = m_velocity.x > max_velocity.x ? max_velocity.x : m_velocity.x;


there must be a better way to constrain the velocity

karnkaul

Minor changes like stronger typing, using glm::clamp, constructor vs no constructor, etc.

karnkaul · 2024-01-30T14:59:56Z

DogTales/src/components/physics.cpp

+	for (bave::Seconds t{}; t <= dt; t += ft) {
+		integrate(t.count());
+	}
+}


There's a subtle issue here: some residue of dt that's less than ft remains un-integrated on every tick. It would be better to accumulate that in a member and add it to the next dt.

would this fix work?

for (bave::Seconds t{ m_accumulator }; t <= dt; t += ft) { integrate(t); if (t + ft >= dt) { m_accumulator = dt - t; } }

I don't think so, something like this would be better:

void Physics::tick(bave::Seconds dt) { // ... for (dt += m_residue; dt > ft; dt -= ft) { integrate(ft); } m_residue = dt; }

karnkaul · 2024-01-30T15:00:42Z

DogTales/src/components/physics.cpp

+	}
+}
+
+void Physics::integrate(float dt) {


Should use bave::Seconds here too, it's ambiguous what unit dt is (seconds or milliseconds etc).

karnkaul · 2024-01-30T15:02:03Z

DogTales/src/components/physics.cpp

+	m_velocity.x = m_velocity.x > max_velocity.x ? max_velocity.x : m_velocity.x;
+	m_velocity.y = m_velocity.y > max_velocity.y ? max_velocity.y : m_velocity.y;
+	m_velocity.x = m_velocity.x < -max_velocity.x ? -max_velocity.x : m_velocity.x;
+	m_velocity.y = m_velocity.y < -max_velocity.y ? -max_velocity.y : m_velocity.y;


Suggested change

m_velocity.x = m_velocity.x > max_velocity.x ? max_velocity.x : m_velocity.x;

m_velocity.y = m_velocity.y > max_velocity.y ? max_velocity.y : m_velocity.y;

m_velocity.x = m_velocity.x < -max_velocity.x ? -max_velocity.x : m_velocity.x;

m_velocity.y = m_velocity.y < -max_velocity.y ? -max_velocity.y : m_velocity.y;

m_velocity = glm::clamp(m_velocity, -max_velocity, max_velocity);

karnkaul · 2024-01-30T15:03:15Z

DogTales/src/components/physics.hpp

+static constexpr glm::vec2 max_velocity{1000.f, 1000.f};
+static constexpr float tick_limit{0.03f}; //this should possibly go somewhere else and be globally accessible


Globals in headers should not be static: that will introduce new storage for the global in each translation unit (cpp file that includes the header). Just constexpr is fine here, though if you want to be pedantic, inline constexpr.

karnkaul · 2024-01-30T15:03:39Z

DogTales/src/components/physics.hpp

+static constexpr glm::vec2 max_velocity{1000.f, 1000.f};
+static constexpr float tick_limit{0.03f}; //this should possibly go somewhere else and be globally accessible


This represents time and should use chrono units (bave::Seconds).

I also think 30ms is too little, 100ms is ok as the death-spiral-bail-out value.

Suggested change

static constexpr glm::vec2 max_velocity{1000.f, 1000.f};

static constexpr float tick_limit{0.03f}; //this should possibly go somewhere else and be globally accessible

constexpr glm::vec2 max_velocity_v{1000.f};

constexpr bave::Seconds tick_limit_v{100ms};

karnkaul · 2024-01-30T15:07:12Z

DogTales/src/components/physics.hpp

+	glm::vec2 m_position{};
+	glm::vec2 m_velocity{};
+	glm::vec2 m_acceleration{};
+
+	glm::vec2 m_friction{};
+	float m_gravity{};
+	float m_mass{};


If all this is public, a constructor isn't needed, users can just do:

auto physics = Physics{ .m_friction = /* ... */, .m_gravity = /* ... */, .m_mass = /* ... */, };

As a style thing, this is one reason why I don't prefix m_ for public members.

However, shouldn't gravity be shared amongst all physics components, as opposed to being an individual member on each instance?

do you think friction, position, velocity etc. should be public? or should I write getters for them?

I agree intuitively about gravity, but being able to change it might make it easier to simulate light-weightedness (for certain enemies or drops, etc).

do you think friction, position, velocity etc. should be public? or should I write getters for them?

It depends on whether you want them to be "set once and then it's read-only" or "set at your discretion". The former will take the values in the constructor and provide getters (but not setters). Latter would get rid of the constructor and keep the members public.

this isn't working because it says a designator cannot be used with a non-aggregate type (even though I deleted the constructor). is my syntax wrong here?

component::Physics m_physics = {.friction{0.999f, 0.999f}, .gravity{-1000.f}, .mass{1.f}};

Is this a member variable? Try removing the = sign.
Alternatively, try component::Physics m_physics = component::Physics{...}.

yep, it's a member variable of Player. neither of those solutions worked either. here's the new physics.hpp if the issue lies there:

namespace component { constexpr glm::vec2 max_velocity_v{1000.f}; constexpr bave::Seconds tick_limit_v{100ms}; class Physics { bave::Seconds m_residue{}; public: glm::vec2 position{}; glm::vec2 velocity{}; glm::vec2 acceleration{}; glm::vec2 friction{}; float gravity{}; float mass{}; void tick(bave::Seconds const dt); void integrate(bave::Seconds dt); }; }

ok I realized it's the m_residue being private

so, I guess I need to define a constructor then, because m_residue being public feels pretty wrong

Right, all the fields of aggregate types must be public.
A constructor that uses default values will be best then, that way both of these would work:

auto custom = Physics{/* ... */}; auto default_ = Physics{};

…gration

karnkaul

Also recommend removing Player::m_vel and related logic since that's no longer used.

karnkaul · 2024-01-30T17:06:31Z

DogTales/src/components/physics.hpp

+constexpr glm::vec2 default_friction{0.999f};
+constexpr float default_gravity{-1000.f};
+constexpr float default_mass{1.f};
+
+constexpr glm::vec2 max_velocity_v{1000.f};
+constexpr bave::Seconds tick_limit_v{100ms};


I'd move these constants into class Physics (as static constexpr) since they are specific to it.

karnkaul · 2024-01-30T17:12:19Z

DogTales/src/player.cpp

+	m_sprite.transform.position = m_physics.position;

 	handle_wall_collision();


Since handle_wall_collision clamps the player sprite's position to the bottom of the screen, after that point m_physics.position and m_sprite.transform.position keep diverging indefinitely, effectively pinning the player to the ground no matter what (jumping from -1000 to -990 will still get clamped to -360 or whatever).

For time being I'd suggest setting m_physics.position = m_sprite.transform.position after the clamping, to synchronize them again.

karnkaul · 2024-01-30T17:13:59Z

DogTales/src/components/physics.hpp

-	glm::vec2 m_friction{};
-	float m_gravity{};
-	float m_mass{};
+	Physics(glm::vec2 fric = default_friction, float grav = default_gravity, float m = default_mass) : friction(fric), gravity(grav), mass(m) {}


Use longer names, doesn't matter if the init goes into the next line.

Suggested change

Physics(glm::vec2 fric = default_friction, float grav = default_gravity, float m = default_mass) : friction(fric), gravity(grav), mass(m) {}

Physics(glm::vec2 friction = default_friction, float gravity = default_gravity, float mass = default_mass) : friction(friction), gravity(gravity), mass(mass) {}

ah, I always thought the names had to be different otherwise the compiler would get confused. good to know, and fixed!

Haha yeah, somehow in the initializer list it can disambiguate just fine. Once you enter the constructor body you'll need explicit this->foo = foo.

… clamping sprite position.

karnkaul · 2024-01-30T17:38:16Z

DogTales/src/components/physics.hpp

+	static constexpr glm::vec2 default_friction{0.99f};
+	static constexpr float default_gravity{-1000.f};
+	static constexpr float default_mass{1.f};

-class Physics {
+	static constexpr glm::vec2 max_velocity_v{1000.f};
+	static constexpr bave::Seconds tick_limit_v{100ms};


Nitpick: some constants use a _v suffix (for "value", standard library convention), some don't.

karnkaul · 2024-01-30T17:38:41Z

DogTales/src/player.cpp

+	if (position.x < bounce_rect.top_left().x || position.x > bounce_rect.bottom_right().x) { m_physics.velocity.x *= -0.9f; }
+	if (position.y > bounce_rect.top_left().y || position.y < bounce_rect.bottom_right().y) { m_physics.velocity.y *= -0.9f; }


Just curious: what's with the 0.9... instead of 1.0? 😄

when it's 1.0, it bounces against the ground for a really long time. I think of the 0.9 as the object transferring 10% of its energy into the ground/wall. it's just a needless temporary cosmetic fix, since player collision will probably be completely static anyway :)

add physics component. give physics to player and use it to update th…

4952586

…e sprite position.

swagween requested a review from karnkaul January 30, 2024 14:51

swagween commented Jan 30, 2024

View reviewed changes

karnkaul requested changes Jan 30, 2024

View reviewed changes

update velocity clamping, stronger typing, more accurate physics inte…

ae4abd3

…gration

swagween requested a review from karnkaul January 30, 2024 17:03

karnkaul requested changes Jan 30, 2024

View reviewed changes

karnkaul linked an issue Jan 30, 2024 that may be closed by this pull request

implement gravity to the player (and possible other mobs?) #23

Closed

make constants members of Physics class. reset physics position after…

f47724e

… clamping sprite position.

swagween requested a review from karnkaul January 30, 2024 17:33

karnkaul approved these changes Jan 30, 2024

View reviewed changes

swagween merged commit d975f25 into main Jan 30, 2024
2 checks passed

swagween deleted the lana_dev branch January 30, 2024 18:32

		static constexpr glm::vec2 max_velocity{1000.f, 1000.f};
		static constexpr float tick_limit{0.03f}; //this should possibly go somewhere else and be globally accessible

		m_sprite.transform.position = m_physics.position;

		handle_wall_collision();

	Physics(glm::vec2 fric = default_friction, float grav = default_gravity, float m = default_mass) : friction(fric), gravity(grav), mass(m) {}
	Physics(glm::vec2 friction = default_friction, float gravity = default_gravity, float mass = default_mass) : friction(friction), gravity(gravity), mass(mass) {}

		if (position.x < bounce_rect.top_left().x \|\| position.x > bounce_rect.bottom_right().x) { m_physics.velocity.x *= -0.9f; }
		if (position.y > bounce_rect.top_left().y \|\| position.y < bounce_rect.bottom_right().y) { m_physics.velocity.y *= -0.9f; }

add physics component #29

add physics component #29

Conversation

swagween commented Jan 30, 2024

Choose a reason for hiding this comment

karnkaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swagween Jan 30, 2024 • edited Loading

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

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 Jan 30, 2024 • edited Loading

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 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

karnkaul left a comment • edited Loading

Choose a reason for hiding this comment

karnkaul Jan 30, 2024 • edited Loading

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

swagween Jan 30, 2024 •

edited

Loading

karnkaul Jan 30, 2024 •

edited

Loading

karnkaul Jan 30, 2024 •

edited

Loading

karnkaul left a comment •

edited

Loading

karnkaul Jan 30, 2024 •

edited

Loading