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.

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

box2d physics support within NovelRT #282

Closed
wants to merge 18 commits into from

Conversation

Pheubel
Copy link
Contributor

@Pheubel Pheubel commented Apr 10, 2021

This pull request aims to partially satisfy issue #84 (Add support for either Box2D or Bullet?).

Copy link
Member

@capnkenny capnkenny left a comment

Choose a reason for hiding this comment

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

Looks like a good start to me!

The primary issue here is that the components themselves have direct dependencies on Box2D - ultimately, the Box2D components you have created should be inheriting from an abstraction due to future plans of moving to a plugin-based system (that and as you already know from Discord, this will not be the only phsyics system we consider adding to the engine core 😄 )

Other than that and the few questions I have, I'd think you'd be good to keep going. Feel free to reach out to @RubyNova or myself about the abstraction in Discord.

{
b2Body* body;

static const PhysicsBody DeletedBodyState;
Copy link
Member

Choose a reason for hiding this comment

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

Is this something that is required to be stored per component? Or is this more like a constant and could be held by the system?

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 made this static const because i need to give a state that signals a deleted component tothe buffer and i did it the way i would do it in C#. Although it works, i am not sure if this is the best way to do it.

float accumilator;
float stepTime;

static const PhysicsWorld DeletedWorld;
Copy link
Member

Choose a reason for hiding this comment

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

Same here, do we want to refer to deleted instances in the component?


namespace NovelRT::Physics::Box2d
{
struct TestTransform
Copy link
Member

Choose a reason for hiding this comment

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

Is this just for testing since we don't have a proper Transform component yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, it is a bit of a crude example of how i would imagine the transform to look like, or at least, the needed bits. Once a proper transform component is implemented in NovelRT, this one can be replaced and removed.

Comment on lines +401 to +410
#if false
world.Step(delta.getSecondsFloat(), velocityIterations, positionIterations);
b2Vec2 position = body->GetPosition();
boxRect->transform().position = *reinterpret_cast<NovelRT::Maths::GeoVector2F*>(&position);
float angle = body->GetAngle();
char message[50];
sprintf(message,"%4.2f %4.2f %4.2f\n",position.x, position.y, angle);

console.log(message, NovelRT::LogLevel::Debug);
#else
Copy link
Member

Choose a reason for hiding this comment

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

What's this for? Testing outside of the ECS setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i forgot to remove it/clean it up. Whoops. Might be good to have an ECS'less example as well that can be toggled, but that could clutter up the main more, and truth be told, it is already quite filled up with a lot of general stuff. Maybe i should look into making a proper physics sample at one point.

@FiniteReality
Copy link
Contributor

@capnkenny I told Kat to just code directly what box2d exposes here, since Bullet will likely expose something very different. An interface or something like that probably won't work.

@capnkenny
Copy link
Member

@capnkenny I told Kat to just code directly what box2d exposes here, since Bullet will likely expose something very different. An interface or something like that probably won't work.

That's fair - tbh the last is more feedback that may come up from @RubyNova than myself after discussing the general path for the engine w.r.t ECS and moving towards plug-ins.
I agree with your sentiment (mostly) that an interface wouldn't work when comparing to Bullet. However, (another decision up to Matt) we may want to interface this in case we may want to allow usage of another 2d physics system instead (i.e Chipmunk2D, etc).

@FiniteReality
Copy link
Contributor

Ah; my understanding was that we were only really going to support Box2D and Bullet 😅

@RubyNova
Copy link
Member

RubyNova commented Apr 12, 2021

This has to be agnostic. I/we are not implementing multiple default physics ECS system implementations. It might not be able to be agnostic when compared to bullet, but dependencies shouldn't be leaking anyway, and default ECS support should be as universal as possible. There's multiple places where we need to resolve this, I don't want any more cases of it.

In addition to this - making a hard-coded dependency on box2d and/or bullet defies the entire point and structure of NovelRT.

@RubyNova
Copy link
Member

PR is dead. Closing.

@RubyNova RubyNova closed this Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants