Replace Newton Dynamics with Jolt Physics #287

Lamakaio · 2024-05-29T13:24:36Z

I am making this PR to track the work on replacing Newton Dynamics with Jolt Physics.
If it's not the right way to track things, feel free to remove it.

Add JoltPhysics to the repo as a submodule
Make it compile with the project
Write the interface to Jolt, similar to ospnewton
Write the Session for ospjolt, similar to newton
Change scenarios to use Jolt instead of Newton
Fix remaining bugs
Find out what I forgot in this list

jonesmz · 2024-05-29T15:23:26Z

Really great start to this! I applaud your communication and consistency thereof!

jonesmz · 2024-05-29T15:24:46Z

.gitmodules

@@ -37,3 +37,6 @@
 [submodule "3rdparty/freetype"]
 	path = 3rdparty/freetype
 	url = https://github.com/freetype/freetype
+[submodule "3rdparty/JoltPhysics"]
+	path = 3rdparty/JoltPhysics
+	url = [email protected]:jrouwe/JoltPhysics.git


If you didn't already do this, please make sure you track an "official" release by git-tag, if that exists for Jolt.

For all the other submodules, except the Magnum graphics related ones, I Just track whatever the latest official release tag is.

For magnum, they just kind of stopped doing releases, so i just periodically bump to the latest commit.

It should be tracking v5.0.0 ? hopefully

The Jolt submodule is set to use a git+ssh protocol, and this only works if your client is setup with a valid GitHub SSH credential (e.g. it cannot be cloned anonymously this way)

Like the other dependencies I'm assuming this should be using the HTTPS URI of https://github.com/jrouwe/JoltPhysics.git so that it can be cloned anonymously

jonesmz · 2024-05-29T15:25:26Z

src/adera/activescene/VehicleBuilder.cpp

@@ -201,15 +201,15 @@ VehicleData VehicleBuilder::finalize_release()

        // reserve node-to-machine partitions
        rPerNodeType.nodeToMach.data_reserve(rPerNodeType.m_connectCountTotal);
-        for (NodeId node : rPerNodeType.nodeIds.bitview().zeros())
+        for (NodeId node : rPerNodeType.nodeIds)


I don't disagree with this change, but I'm not really sure how related to the Jolt physics change it is.

Maybe this should be in it's own stand-alone PR?

It is ! Sorry it is not the cleanest, I have two other PRs open and this is one of them. I kinda figured I would start with the most advanced version of the code I could, as this PR will take me quite some time anyway. I'll just rebase whenever it is merged.

Lamakaio · 2024-06-06T18:11:16Z

I am mostly finished here, but I get some weird behavior in Release mode only. Valgrind doesn't tell me anything, so I'm not sure where to look. If I could get some additional testing / a review it would be great.

Jolt produces compilation warnings on Windows with Wall, so I disabled warnings in Jolt (at least, hopefully, I don't have MSVC to test it locally).
It still fails on some builds only, and I'm a bit at loss as to why.

Currently I left all the Newton code up, and examples simply have to include Jolt.h or Newton.h to switch physics engine. I'm not sure how relevant that is, though.

src/ospjolt/activescene/joltinteg.h

Capital-Asterisk

Nice work! I'd assume the Newton submodule should be kept around for benchmarking? Fairly simple to just make a copy of each test scenario. I don't mind this being merged in a non-final state as long as it mostly works; I'd like to figure out how to get around doing planet terrain colliders.

src/ospjolt/activescene/joltinteg.h

Capital-Asterisk · 2024-06-07T01:43:35Z

src/ospjolt/activescene/joltinteg.h

+	va_end(list);
+
+	// Print to the TTY
+	std::cout << buffer << std::endl;


This can be OSP_LOG_*, though I'm wondering if spdlog or fmt has a built-in function for va_list

src/ospjolt/activescene/joltinteg.h

Capital-Asterisk · 2024-06-07T01:57:08Z

src/ospjolt/activescene/joltinteg.h

+	static constexpr ObjectLayer NON_MOVING = 0;
+	static constexpr ObjectLayer MOVING = 1;
+	static constexpr ObjectLayer NUM_LAYERS = 2;
+};


This is not important right now, though I'm wondering if we're going to need to dynamically reassign these and filters at runtime eventually.

Capital-Asterisk · 2024-06-07T03:16:55Z

src/ospjolt/activescene/joltinteg_fn.cpp

+    case EShape::Cylinder:
+        //cylinder needs to be internally rotated 90° to match with graphics
+        joltShape = (Shape *) new CylinderShape(1.0f, 2.0f);
+        joltShape = (Shape *) new RotatedTranslatedShape(Vec3Arg::sZero(), Quat::sRotation(Vec3::sAxisX(), JPH_PI/2), joltShape);


just put new CylinderShape(1.0f, 2.0f) into the constructor

Capital-Asterisk · 2024-06-07T03:52:08Z

src/ospjolt/activescene/joltinteg_fn.cpp

+    Quat joltRotation(rawQuat[0], rawQuat[1], rawQuat[2], rawQuat[3]);
+
+    //scale the shape directly
+    pJoltShape->mShape = pJoltShape->mShape->ScaleShape(Vec3Arg(scale.x(), scale.y(), scale.z())).Get();


unnecessary comment + huh, that's a knockoff Rust Result? (not really)

I'm not sure if this is an absolute or relative scale. I have a feeling it's relative, which is not what we want to do here.

Or wouldn't this continuously accumulate more and more layers of ScaledShape each time orient_shape is called? eg: each time a compound shape is modified

create_primitive and orient_shape previously exists to fit around Newton Dynamic's model. The way to just 'make a shape given a position/rotation/scale' in Jolt is likely different.

Is the compound shape (collider) transform supposed to be absolute ? I assumed it was relative to the compound shape transform (mostly because the recursive function starts with identity transform)
That is, the final absolute transform needs to be "compound shape transform" * "relative subshape transform".

I do agree that there are better ways to handle this though. I'll scrap orient_shape and create_primitive to make something that only adds ScaledShape wrapper when it is needed (ie when we are not creating the shape in the same spot, and when the shape is not already scaled).

src/ospjolt/activescene/joltinteg.h

Capital-Asterisk · 2024-06-07T04:41:58Z

src/ospjolt/activescene/joltinteg_fn.cpp

+
+//TODO this is locking on all bodies. Is it bad ?
+//The easy fix is to provide multiple step listeners for disjoint sets of bodies, which can then run in parallel.
+//It might not be worth it considering this function should be quite fast.


For now this works, but maybe a custom Constraint is the way to go later on for ForceFactors?

Docs says all bodies are all locked within PhysicsStepListener, and bodies can be accessed directly.

src/testapp/sessions/jolt.cpp

jonesmz · 2024-06-07T07:00:45Z

Nice work! I'd assume the Newton submodule should be kept around for benchmarking? Fairly simple to just make a copy of each test scenario. I don't mind this being merged in a non-final state as long as it mostly works; I'd like to figure out how to get around doing planet terrain colliders.

I think keeping newton for a short amount of time might be worth doing, but we should definitely be removing it if jolt is working at a similar level or better. Newton doesn't compile on a bunch of targets, and it's code is a bonkers mess. I'd rather just be done with it.

jonesmz · 2024-06-07T07:01:51Z

src/ospjolt/activescene/forcefactors.h

@@ -0,0 +1,39 @@
+/**
+ * Open Space Program
+ * Copyright © 2019-2024 Open Space Program Project


Arguably the copyright date for this file only starts in 2024, not 2019.

I'm not a lawyer, so i'm not 100% certain how this should be done.

I copied some things straight from the forcefactor.h in ospnewton which starts in 2019, but honestly I have no idea either.

jonesmz · 2024-06-07T07:08:52Z

src/ospjolt/activescene/forcefactors.h

+// These determine which physics calculations are required for a certain
+// rigid body, such as gravity, thurst, or aerodynamics.
+// Forces are assignable at runtime in ACtxJoltWorld::m_factors
+using ForceFactors_t = std::array<uint64_t, 1u>;


Why use an array of size one?

Is this to provide a strong typedef? or is the goal here to generically support any number of items in the array, but start with only 1?

(not my code, so I can't be 100% sure)
I think the intent is to be able to have a bigger array, so that more than 64 forces can be supported. Currently some things break if the array is bigger though.

jonesmz · 2024-06-07T07:10:49Z

src/ospjolt/activescene/joltinteg.h

+static void TraceImpl(const char *inFMT, ...)
+{
+	// Format the message
+	va_list list;


I'm very, very, very, wary of C++ code using va_list / va_start.

What's driving the use of a C-style variadic function instead of a C++ template function?

Somehow, there don't seem to be another way.

Basically, I updated it to use spdlog, and the problem is, the spdlog function takes 2 arguments then variadics, and jolt expects 1 argument then variadics. So the only way seem to be to "unwrap" the variadics using vsnprintf, then call spdlog::trace.
I just looked into it and I'm pretty annoyed there isn't a simpler way.

Template function can't be used here, as Jolt expects a generic function pointer that can be used for any type of values.

jonesmz · 2024-06-07T07:11:50Z

src/ospjolt/activescene/joltinteg.h

+ */
+namespace Layers
+{
+	static constexpr ObjectLayer NON_MOVING = 0;


Is there a reason not to use an enum class for these constants? They look a lot like enum values?

Jolt expects "ObjectLayer" values, which is basically u16. Is there a way to make an enum which can be compared against another type easily in c++ ? For exemple in a switch statement

src/ospjolt/activescene/joltinteg.h

jonesmz · 2024-06-07T07:20:40Z

src/ospjolt/activescene/joltinteg.h

+#include <osp/activescene/basic.h>
+#include <osp/core/id_map.h>
+
+#include <Jolt/Jolt.h>


from https://github.com/jrouwe/JoltPhysics/blob/master/HelloWorld/HelloWorld.cpp

// The Jolt headers don't include Jolt.h. Always include Jolt.h before including any other Jolt header. // You can use Jolt.h in your precompiled header to speed up compilation. #include <Jolt/Jolt.h>

wow, what an absolutely bullshit decision.

jonesmz · 2024-06-07T07:24:08Z

src/ospjolt/activescene/joltinteg.h

+		case Layers::MOVING:
+			return true; // Moving collides with everything
+		default:
+			JPH_ASSERT(false);


Outside the scope of this PR, but we should probably make a more advanced ASSERT implementation for OSP that automatically does fun things like generating coredumps, generating back traces, and so on.

src/ospjolt/activescene/joltinteg.h

jonesmz · 2024-06-07T07:32:05Z

src/ospjolt/activescene/joltinteg_fn.cpp

+
+TransformedShapePtr_t SysJolt::create_primitive(ACtxJoltWorld &rCtxWorld, osp::EShape shape)
+{
+    Shape* joltShape;


std::unique_ptr

src/ospjolt/activescene/joltinteg_fn.h

src/testapp/scenarios.cpp

src/testapp/sessions/jolt.cpp

jonesmz · 2024-06-07T07:51:30Z

To fix the msvc analyzer workflow, we'll need to link against the debug build of the microsoft libraries when building the Debug config.

See: https://stackoverflow.com/questions/35793622/lnk2019unresolved-external-symbol-crtdbgreport-referenced-in-function-crt-rtc

That's controlled by https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html

but it doesn't seem to be working properly?

Lamakaio · 2024-06-12T21:13:51Z

This should be mostly done, except for the windows build issue, which I can't seem to find a solution to. If no new idea arise, I will probably setup the project on my windows machine to do some more in-depth testing. I'm still a bit unsure of some design decision (like the jolt / osp ids), but it's the best I could come up with that compiles.

Lamakaio · 2024-06-13T22:19:02Z

src/ospjolt/activescene/joltinteg.h

+    BroadPhaseLayer GetBroadPhaseLayer(ObjectLayer inLayer) const override
+    {
+        JPH_ASSERT(inLayer < Layers::NUM_LAYERS);
+        return mObjectToBroadPhase[inLayer];


Looks like this is covered by the assert?

The only constructor initializes the array up to Layers::NUM_LAYERS.

I've seen clang-tidy do similar things on custom asserts. Under the 'show paths' button, it says it's assuming that the condition inLayer < Layers::NUM_LAYERS is false, and doesn't know the program should just panic in that case.

There's specifically a function attribute to tag functions like that as "If you get here, assume everything after it is unreachable".

but i don't think there's an attribute like that for MSVC... (if someone knows about one, please tell me, as i really would like to use it at work...)

Note that these code scanners are scanning based on MSVC, not clang / gcc.

Github requires "Sarif" output to be able to ingest analyzer results, and clang / gcc don't support that format for their analyzer tools.

Lamakaio · 2024-06-16T11:26:00Z

I did one more pass on the code and rebased it. I feel like it's pretty much done on my end.

Capital-Asterisk · 2024-06-22T19:01:58Z

One thing I forgot to note, is that the rockets in the vehicle test scenario have a bit of a pendulum effect on them. Something is up with the center-of-mass or torque calculations. I don't mind this being fixed later.

does this PR still need WIP in the title?

jonesmz · 2024-06-23T12:05:06Z

I have no objection to merging this code as is.

@Lamakaio can you please do three (relatively) minor things so we can merge it?

Change the title to remove "wip".
Change the PR from draft to normal.
Rebase against master one last time, and this time squash/fixup your commits into a more "publicly facing" format. You don't need to squish everything into a single commit, just try to reduce the quantity, and maybe add some more elaborated commit messages if you feel like it.

Lamakaio · 2024-06-23T18:21:39Z

Absolutely ! I'll do that tomorrow.
I'll check the center of mass / inertia calculation quickly in case I missed something obvious.

…round)

Lamakaio · 2024-06-24T11:00:39Z

Should be all done. I also fixed the pendulum effect, there was a stray minus sign in the center of mass calculations.

(sorry for the multiple pushes I broke some things while rebasing and rewriting the history apparently...)

jonesmz · 2024-06-24T12:36:45Z

Ok, passing CI, approval from CapitalAsterisk and myself, and nice commit organization.

Lets merge it!

jonesmz reviewed May 29, 2024

View reviewed changes

Lamakaio force-pushed the jolt-physics branch from 8a3cf05 to f6ba0e0 Compare June 6, 2024 10:27

Lamakaio marked this pull request as ready for review June 6, 2024 18:07

github-advanced-security bot found potential problems Jun 6, 2024

View reviewed changes

src/ospjolt/activescene/joltinteg.h Fixed Show resolved Hide resolved

Capital-Asterisk approved these changes Jun 7, 2024

View reviewed changes

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/ospjolt/activescene/joltinteg.h Outdated Show resolved Hide resolved

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/ospjolt/activescene/joltinteg.h Outdated Show resolved Hide resolved

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/ospjolt/activescene/joltinteg.h Outdated Show resolved Hide resolved

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/ospjolt/activescene/joltinteg.h Outdated Show resolved Hide resolved

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/ospjolt/activescene/joltinteg.h Outdated Show resolved Hide resolved

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/ospjolt/activescene/joltinteg_fn.h Outdated Show resolved Hide resolved

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/testapp/scenarios.cpp Outdated Show resolved Hide resolved

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/testapp/scenarios.cpp Outdated Show resolved Hide resolved

jonesmz reviewed Jun 7, 2024

View reviewed changes

src/testapp/sessions/jolt.cpp Outdated Show resolved Hide resolved

Lamakaio force-pushed the jolt-physics branch from 8e9f44a to fc9945e Compare June 13, 2024 22:04

github-advanced-security bot found potential problems Jun 13, 2024

View reviewed changes

Lamakaio force-pushed the jolt-physics branch from fc9945e to a4617ea Compare June 16, 2024 11:23

Lamakaio mentioned this pull request Jun 17, 2024

GDExtension build prototype #294

Open

Lamakaio changed the title ~~WIP replace Newton Dynamics with Jolt Physics~~ Replace Newton Dynamics with Jolt Physics Jun 24, 2024

Lamakaio added 2 commits June 24, 2024 12:55

Adds JoltPhysics to the project

b85d983

Change the physics engine from Newton to Jolt (While keeping Newton a…

ef02e34

…round)

Lamakaio force-pushed the jolt-physics branch from a4617ea to 2e93d90 Compare June 24, 2024 10:58

Lamakaio force-pushed the jolt-physics branch from 2e93d90 to 08eac4d Compare June 24, 2024 11:25

Add optimization for many bodies + build fixes

27722e2

Lamakaio force-pushed the jolt-physics branch from 08eac4d to 27722e2 Compare June 24, 2024 11:47

jonesmz merged commit c70effd into TheOpenSpaceProgram:master Jun 24, 2024
22 checks passed

Replace Newton Dynamics with Jolt Physics #287

Replace Newton Dynamics with Jolt Physics #287

Conversation

Lamakaio commented May 29, 2024 • edited Loading

jonesmz commented May 29, 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

Lamakaio May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Lamakaio commented Jun 6, 2024 • edited Loading

Capital-Asterisk left a 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

jonesmz commented Jun 7, 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

jonesmz commented Jun 7, 2024

Lamakaio commented Jun 12, 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

Lamakaio commented Jun 16, 2024

Capital-Asterisk commented Jun 22, 2024

jonesmz commented Jun 23, 2024

Lamakaio commented Jun 23, 2024 • edited Loading

Lamakaio commented Jun 24, 2024 • edited Loading

jonesmz commented Jun 24, 2024

Lamakaio commented May 29, 2024 •

edited

Loading

Lamakaio May 29, 2024 •

edited

Loading

Lamakaio commented Jun 6, 2024 •

edited

Loading

Lamakaio commented Jun 12, 2024 •

edited

Loading

Lamakaio commented Jun 23, 2024 •

edited

Loading

Lamakaio commented Jun 24, 2024 •

edited

Loading