Replace OU library and volatile atomics with C++ standard equivalents by Kwizatz · Pull Request #90 · odedevs/ode

Kwizatz · 2026-04-15T23:20:26Z

Remove the OU (Objects Unlimited) library dependency and replace all volatile-based atomics with C++ std::atomic, using only standard C++ threading and synchronization primitives throughout.

Atomics migration (threading_atomics_provs.h, threadingutils.h):

Replace volatile + custom CAS/exchange intrinsics with std::atomic
Replace OU CAtomicsUnsafeReinit/CAtomicPtr with std::atomic
Replace custom cellatomic/atomicord32/atomicptr typedefs with std::atomic equivalents
Update all consumers: threaded_solver_ldlt.h, coop_matrix_types.h, fastl*.cpp/impl.h, fastvecscale, step.cpp, quickstep.cpp, threading_impl_templates.h, resource_control.h, objects.h, util.*

OU removal (odeou.h, odeou.cpp, odeinit.cpp, CMakeLists.txt):

Replace OU CEnumUnsortedElementArray/CEnumSortedElementArray with standalone C++ templates in odeou.h
Replace OU CSimpleFlags with a minimal bitflag class in odeou.h
Remove OU initialization/customization bridge from odeou.cpp
Remove dOU_ENABLED, dATOMICS_ENABLED, dTLS_ENABLED guards from odeinit.cpp
Remove ODE_WITH_OU CMake option and OU source/include references
Remove unused odeou.h includes from collision_kernel.cpp, misc.cpp, joints/amotor.cpp

TLS rewrite (odetls.h, odetls.cpp):

Replace OU CThreadLocalStorage with direct slot-based struct
Use Windows FLS (Fiber-Local Storage) API on Windows for reliable per-thread cleanup (works around MinGW thread_local destructor bug)
Use thread_local with RAII wrapper on POSIX platforms

Threading tests (tests/threading.cpp, tests/main.cpp):

Add 26 unit tests covering threading lifecycle, multi-threaded stepping, threaded-vs-single-threaded consistency, TLS allocation, concurrent collision, and island parallelism
Suppress non-fatal LCP solver messages during test runs

All 245 tests pass (219 existing + 26 new).

Remove the OU (Objects Unlimited) library dependency and replace all volatile-based atomics with C++ std::atomic, using only standard C++ threading and synchronization primitives throughout. Atomics migration (threading_atomics_provs.h, threadingutils.h): - Replace volatile + custom CAS/exchange intrinsics with std::atomic - Replace OU CAtomicsUnsafeReinit/CAtomicPtr with std::atomic<T> - Replace custom cellatomic/atomicord32/atomicptr typedefs with std::atomic equivalents - Update all consumers: threaded_solver_ldlt.h, coop_matrix_types.h, fastl*.cpp/impl.h, fastvecscale, step.cpp, quickstep.cpp, threading_impl_templates.h, resource_control.h, objects.h, util.* OU removal (odeou.h, odeou.cpp, odeinit.cpp, CMakeLists.txt): - Replace OU CEnumUnsortedElementArray/CEnumSortedElementArray with standalone C++ templates in odeou.h - Replace OU CSimpleFlags with a minimal bitflag class in odeou.h - Remove OU initialization/customization bridge from odeou.cpp - Remove dOU_ENABLED, dATOMICS_ENABLED, dTLS_ENABLED guards from odeinit.cpp - Remove ODE_WITH_OU CMake option and OU source/include references - Remove unused odeou.h includes from collision_kernel.cpp, misc.cpp, joints/amotor.cpp TLS rewrite (odetls.h, odetls.cpp): - Replace OU CThreadLocalStorage with direct slot-based struct - Use Windows FLS (Fiber-Local Storage) API on Windows for reliable per-thread cleanup (works around MinGW thread_local destructor bug) - Use thread_local with RAII wrapper on POSIX platforms Threading tests (tests/threading.cpp, tests/main.cpp): - Add 26 unit tests covering threading lifecycle, multi-threaded stepping, threaded-vs-single-threaded consistency, TLS allocation, concurrent collision, and island parallelism - Suppress non-fatal LCP solver messages during test runs All 245 tests pass (219 existing + 26 new).

oleh-derevenko · 2026-04-30T06:13:46Z

I did not check the actual changes yet, but, from the description alone, here is my initial commentary.

OU stands for "Oder's Utilities" (and not what your CoPilot suggested ;)). I know it because Oder is the nickname of mine.

The goal of this library was to provide some useful platform-dependent features not directly available in the C++ at that time, as well as a few handy general purpose utilities, in a form of C++ classes and functions. The library is lightweight and mostly header-only.

Now, if atomics are widely available natively in the language, of course, it is better to use the native implementation as it is more flexible, more robust, and it is maintained. But removing the OU just for the sake of removing the OU — it might be not so wise idea. ODE is a physics simulation library and, ideologically, it should be focused on physics and the math for it. If it needs something platform-dependent which is not natively available in the language, it is better to keep that abstracted and isolated in a sub-library.

Always, if you are making a breaking change you should consider what problem are you solving, what will the the gain, and if it is worth breaking things for it.

With the TLS replacement (whatever that replacement could be) I can hardly imagine what could be the gains. Like, TLS always requires initialization on all all the major platforms and the initialization can be failing — you can't save on that. The language-provided thread_local... — I'm not sure it could be used directly in the cases and in the way ODE needs is. And just replacing one platform-dependent implementation with another platform-dependent implementation — that should not be worth breaking things at all. Yes, the current implementation is lacking thread memory release on Windows, but with the way it is used in ODE that's not a problem at all.
But well... Let's see it.

As for the generic utilities, like CSimpleFlags/CAtomicFlags and the enumarrays... If you don't need them it does not automatically mean I don't need them. ;)
Like, as far as I remember it, the flags are not directly used in the ODE — just for auxiliary puposes. And these classes are "uglified" in the ODE/OU because of the extra namespace, and because I had chosen to use different names for each and every method instead of the method names I have in my private code where they are carefully selected for convenience. But with properly selected names, these classes are really worth including into the language standard and to be listed in the language books. I'd be eager to merge them in ODE code but I kept them "externally" in the OU just because of the same priciple: "ODE is a physics library and it is not it's specialization to be defining general utilities".
Well, again, let's first see what was done there and what can be done about it.

oleh-derevenko · 2026-05-03T20:04:53Z

+    std::atomic<uint32_t> *GetStatisticsIterationCountStorage() const { dSASSERT(sizeof(uint32_t) == membersize(dWorldQuickStepIterationCount_DynamicAdjustmentStatistics, iteration_count)); return reinterpret_cast<std::atomic<uint32_t>*>(&m_statistics->iteration_count); }
+    std::atomic<uint32_t> *GetStatisticsPrematureExitsStorage() const { dSASSERT(sizeof(uint32_t) == membersize(dWorldQuickStepIterationCount_DynamicAdjustmentStatistics, premature_exits)); return reinterpret_cast<std::atomic<uint32_t>*>(&m_statistics->premature_exits); }
+    std::atomic<uint32_t> *GetStatisticsProlongedExecutionsStorage() const { dSASSERT(sizeof(uint32_t) == membersize(dWorldQuickStepIterationCount_DynamicAdjustmentStatistics, prolonged_execs)); return reinterpret_cast<std::atomic<uint32_t>*>(&m_statistics->prolonged_execs); }
+    std::atomic<uint32_t> *GetStatisticsFullExtraExecutionsStorage() const { dSASSERT(sizeof(uint32_t) == membersize(dWorldQuickStepIterationCount_DynamicAdjustmentStatistics, full_extra_execs)); return reinterpret_cast<std::atomic<uint32_t>*>(&m_statistics->full_extra_execs); }


So, there is a problem with this.
You can't cast pointer to plain integral value to a pointer to std::atomic<> as the former is a plain type and the latter is a class. Even if on some platforms the binary representation of the class may match the plain value this is not required in general.
And this is a bad code and bad behavior in general as there is plain integer and there is no std::atomic there. You may do the reinterpret_cast like this only if you know that there ACTUALLY IS the class instance there. But here, there is not.

oleh-derevenko · 2026-05-03T20:25:42Z

+    std::atomic<uint32_t>           m_tagsTaken;
+    std::atomic<uint32_t>           m_gravityTaken;
+    std::atomic<uint32_t>           m_inertiaBodyIndex;


Since now you have introduced class instances as fields of the structure you may no longer allocate plain memory and call Initialize() on it. You have to execute in-place constructor and in-place destructor for the structure.

oleh-derevenko · 2026-05-03T20:26:12Z

    unsigned int                    m_m;
    unsigned int                    m_mfb;
-    volatile atomicord32            m_valid_findices;
+    std::atomic<uint32_t>           m_valid_findices;


Since now you have introduced class instances as fields of the structure you may no longer allocate plain memory and call Initialize() on it. You have to execute in-place constructor and in-place destructor for the structure.

oleh-derevenko · 2026-05-03T20:26:30Z

+    std::atomic<uint32_t>           m_ji_J;
+    std::atomic<uint32_t>           m_ji_jb;
+    std::atomic<uint32_t>           m_bi;
+    std::atomic<uint32_t>           m_Jrhsi;


Since now you have introduced class instances as fields of the structure you may no longer allocate plain memory and call Initialize() on it. You have to execute in-place constructor and in-place destructor for the structure.

oleh-derevenko · 2026-05-03T20:27:51Z

+    std::atomic<uint32_t>           *m_bi_links_or_mi_levels;
+    std::atomic<uint32_t>           *m_mi_links;
    dReal                           m_LCP_iteration_premature_exit_delta;
    dCallReleaseeID                 m_LCP_IterationSyncReleasee;
    unsigned int                    m_LCP_IterationAllowedThreads;
    dCallReleaseeID                 m_LCP_fcStartReleasee;
-    volatile atomicord32            m_ji_4a;
-    volatile atomicord32            m_mi_iMJ;
-    volatile atomicord32            m_bi_forceMaxAdj;
-    volatile atomicord32            m_bi_fc;
-    volatile atomicord32            m_LCP_fcPrepareThreadsRemaining;
+    std::atomic<uint32_t>           m_ji_4a;
+    std::atomic<uint32_t>           m_mi_iMJ;
+    std::atomic<uint32_t>           m_bi_forceMaxAdj;
+    std::atomic<uint32_t>           m_bi_fc;
+    std::atomic<uint32_t>           m_LCP_fcPrepareThreadsRemaining;
    unsigned int                    m_LCP_fcCompleteThreadsTotal;
-    volatile atomicord32            m_mi_Ad;
+    std::atomic<uint32_t>           m_mi_Ad;
    unsigned int                    m_LCP_iteration;
    unsigned int                    m_LCP_extra_num_iterations;
    unsigned int                    m_LCP_iterationThreadsTotal;
-    volatile atomicord32            m_LCP_iterationThreadsRemaining;
+    std::atomic<uint32_t>           m_LCP_iterationThreadsRemaining;
    dCallReleaseeID                 m_LCP_iterationNextReleasee;
-    volatile atomicord32            m_SOR_reorderHeadTaken;
-    volatile atomicord32            m_SOR_reorderTailTaken;
-    volatile atomicord32            m_SOR_bi_zeroHeadTaken;
-    volatile atomicord32            m_SOR_bi_zeroTailTaken;
-    volatile atomicord32            m_SOR_mi_zeroHeadTaken;
-    volatile atomicord32            m_SOR_mi_zeroTailTaken;
-    volatile atomicord32            m_SOR_reorderThreadsRemaining;
-    volatile atomicord32            m_cf_4b;
-    volatile atomicord32            m_ji_4b;
+    std::atomic<uint32_t>           m_SOR_reorderHeadTaken;
+    std::atomic<uint32_t>           m_SOR_reorderTailTaken;
+    std::atomic<uint32_t>           m_SOR_bi_zeroHeadTaken;
+    std::atomic<uint32_t>           m_SOR_bi_zeroTailTaken;
+    std::atomic<uint32_t>           m_SOR_mi_zeroHeadTaken;
+    std::atomic<uint32_t>           m_SOR_mi_zeroTailTaken;
+    std::atomic<uint32_t>           m_SOR_reorderThreadsRemaining;
+    std::atomic<uint32_t>           m_cf_4b;
+    std::atomic<uint32_t>           m_ji_4b;


Since now you have introduced class instances as fields of the structure you may no longer allocate plain memory and call Initialize() on it. You have to execute in-place constructor and in-place destructor for the structure.

oleh-derevenko · 2026-05-03T21:43:02Z

+        unsigned lo = 0, hi = static_cast<unsigned>(EnumMax);
+        while (lo < hi)
+        {
+            unsigned mid = lo + (hi - lo) / 2;
+            if (data[mid] < value) lo = mid + 1;
+            else hi = mid;
+        }


Why would not you use std::lower_bound() here if you are redesigning this class?

oleh-derevenko · 2026-05-03T21:46:26Z

+template<typename EnumType, EnumType EnumMax, typename ElementType>
+struct CEnumSortedElementArray


Here, the same commentaries, as in the unsorted case, apply.
Also, the original implementation used to validate that the array elements are ordered with respect to the comparison operator (i. e., that the array can safely be used for binary search) . And this also was one of the the major features of the class that has been lost.

oleh-derevenko · 2026-05-03T21:51:36Z

-    OTI_TRIMESH_TRIMESH_COLLIDER_CACHE,

-    OTI__MAX,
+struct OdeTlsSlot


Why would not you follow the naming convention of the related class in the same file? This is the Windows convention, btw, when classes start with "C".

oleh-derevenko · 2026-05-03T21:58:10Z

+    if (!tls)
+    {
+        tls = new OdeTlsData();
+        FlsSetValue(g_flsIndex, tls);


This function can fail. The API call failures have to be checked and handled.

oleh-derevenko · 2026-05-03T22:11:01Z

+            if (slots[i].trimeshCache)
+            {
+                COdeTls::FreeTrimeshCollidersCache(slots[i].trimeshCache);
+                slots[i].trimeshCache = nullptr;
+            }


This code appears four times within this .cpp file. Why would not you move it to be a method of OdeTlsSlot?

oleh-derevenko · 2026-05-03T22:17:16Z

Also, here is a patch to add missed std::memory_order_relaxed parameters in your changes.

0001-Changed-Adding-missed-std-memory_order_relaxed-param.patch

oleh-derevenko · 2026-05-03T22:38:19Z

+    ~OdeTlsDataWrapper() { data.cleanup(); }
+};
+
+static thread_local OdeTlsDataWrapper g_odeTlsWrapper;


Since TLS key allocation is always a function that can potentially fail, here the 'thread_local' will be initialized from the CRT startup code on dlopen() (in the dynamic library case) or form the main application startup code (in the static library case). As a result, instead of clear failure location in COdeTls::Initialize() the library user will have a failure from dlopen() or a failed CRT startup of their program in general. Even though a very unlikely, this is a quality decrease.

oleh-derevenko · 2026-05-04T07:14:05Z

 }

+// Legacy aliases — both self-threaded and multi-threaded paths now use std::atomic
+typedef dxStdAtomicsProvider dxFakeAtomicsProvider;


Since you have changed the plain type to std::atomic, you are now unable to support simple arithmetic without the atomicity. But, at least, you could create another variant of the same atomic provider with all the operations' memory order reduced to std::memory_order_relaxed.

oleh-derevenko · 2026-05-04T10:49:23Z

Also, pay attention that you were able to make this refactoring partly because I used custom typedefs of atomicord32 and atomicptr. Now, if you replace these typedefs with generic uint32_t and void * you will lose some information and it will be harder to find the respective values and perform one more refactoring like this. Always, if a type has some custom meaning behind it, it is better to use a typedef name, rather than the generic int/void *.

Kwizatz force-pushed the ou-to-cxx-threading branch from 5eb9838 to a36dc3a Compare April 15, 2026 23:39

Kwizatz force-pushed the ou-to-cxx-threading branch from a36dc3a to 52c4520 Compare April 16, 2026 20:27

oleh-derevenko self-requested a review April 29, 2026 19:30

oleh-derevenko requested changes May 3, 2026

View reviewed changes

oleh-derevenko reviewed May 3, 2026

View reviewed changes

oleh-derevenko requested changes May 4, 2026

View reviewed changes

		template<typename EnumType, EnumType EnumMax, typename ElementType>
		struct CEnumSortedElementArray

Uh oh!

Conversation

Kwizatz commented Apr 15, 2026

Uh oh!

oleh-derevenko commented Apr 30, 2026

Uh oh!

oleh-derevenko May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

oleh-derevenko commented May 3, 2026

Uh oh!

oleh-derevenko May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleh-derevenko May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleh-derevenko commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oleh-derevenko May 3, 2026 •

edited

Loading

oleh-derevenko May 3, 2026 •

edited

Loading

oleh-derevenko May 4, 2026 •

edited

Loading