Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add physics component #29

Merged
merged 3 commits into from
Jan 30, 2024
Merged

add physics component #29

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4952586
add physics component. give physics to player and use it to update th…
swagween Jan 30, 2024
ae4abd3
update velocity clamping, stronger typing, more accurate physics inte…
swagween Jan 30, 2024
f47724e
make constants members of Physics class. reset physics position after…
swagween Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions DogTales/src/components/physics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#include <src/components/physics.hpp>

namespace component {

Physics::Physics(glm::vec2 friction, float gravity, float mass) : m_friction(friction), m_gravity(gravity), m_mass(mass) {}

void Physics::tick(bave::Seconds const dt) {
static constexpr bave::Seconds ft{0.005}; // 5ms

if (dt.count() > tick_limit) { return; } // return for unexpected dt values, particularly during the beginning of the state

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@swagween swagween Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

would this fix work?

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

Copy link
Member

Choose a reason for hiding this comment

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

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


void Physics::integrate(float dt) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there must be a better way to constrain the velocity

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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


m_position += m_velocity * dt;
}

} // namespace component
28 changes: 28 additions & 0 deletions DogTales/src/components/physics.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#pragma once
#include <bave/app.hpp>
#include <bave/graphics/sprite.hpp>

namespace component {

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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!


class Physics {

public:

glm::vec2 m_position{};
glm::vec2 m_velocity{};
glm::vec2 m_acceleration{};

glm::vec2 m_friction{};
float m_gravity{};
float m_mass{};
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@karnkaul karnkaul Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I realized it's the m_residue being private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@karnkaul karnkaul Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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


Physics(glm::vec2 friction, float gravity, float mass = 1.f);

void tick(bave::Seconds const dt);
void integrate(float dt);
};

} // namespace component
4 changes: 3 additions & 1 deletion DogTales/src/player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
Player::Player(glm::vec2 const world_space) : m_world_space(world_space) { m_sprite.set_size(size_v); }

void Player::tick(bave::Seconds const dt) {
m_sprite.transform.position += m_vel * dt.count();

m_physics.tick(dt);
m_sprite.transform.position = m_physics.m_position;

handle_wall_collision();
}
Expand Down
2 changes: 2 additions & 0 deletions DogTales/src/player.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once
#include <bave/app.hpp>
#include <bave/graphics/sprite.hpp>
#include "components/physics.hpp"

class Player {
static constexpr glm::vec2 speed_v{500.0f, 500.0f};
Expand All @@ -11,6 +12,7 @@ class Player {
bave::Sprite m_sprite{};

glm::vec2 m_vel{speed_v};
component::Physics m_physics{{0.999f, 0.999f}, -1000.f};

void handle_wall_collision();

Expand Down