TheOpenSpaceProgram / osp-magnum

A spaceship game
https://www.openspaceprogram.org/
MIT License
216 stars 32 forks source link

Replace osp::BitVector_t with lgrn::IdSetStl #285

Closed Lamakaio closed 6 months ago

Lamakaio commented 6 months ago

This pull request replaces osp::BitVector_t with lgrn::IdSetStl as outlined in issue #282. It depends on #281 and #284 at the moment.

All the functional refactoring should be done, but for some reason both this branch and #281 fail to compile on my (admittedly janky) hardware, and I still need to do a pass to check formatting and variable names.

Capital-Asterisk commented 6 months ago

Delightfully surprised to see this addressed rather quickly, thanks!

May I ask what the build issue might be? If it's related to building SDL2, then there exists the -DOSP_USE_SYSTEM_SDL=ON option that can be enabled to use SDL2 through a system package manager or other means.

Lamakaio commented 6 months ago

I solved the build issue. Somehow my computer tends to crash during builds (not only this project) and it corrupted some compilation artifacts or something, entirely on my end anyway.

However, I am now having some issues on assertion fail due to insufficient size for the IdSets (trying to insert bit 64 on set of size 64).

In some places in the code, sets are resized like that (in testapp/sessions/shapes.cpp)

rPhys.m_hasColliders.resize(rBasic.m_activeIds.vec().capacity());

The resize function looks like that (in id_set_htl.hpp)

void resize(std::size_t n)
    {
        vec().resize(lgrn::div_ceil(n, Base_t::bitview().int_bitsize()), 0);
    }

To me, it looks like we're using the capacity of an uint64 vector as the number of individual bits in the IdSet, and that is why it is too small.

However, I cannot figure out why it would work before if that was the case, as my changes should not have changed that behavior.

Anyway, I get assertion fails on insert on the sets that are resized this way (after the refactor).

jonesmz commented 6 months ago

Somehow my computer tends to crash during builds (not only this project) and it corrupted some compilation artifacts or something, entirely on my end anyway.

Try limiting parallelism in the build.

`cmake --build path/to/build --parallel numberParallelJobs``

Capital-Asterisk commented 6 months ago

In some places in the code, sets are resized like that (in testapp/sessions/shapes.cpp)...

You forgot the ints() call in the example you posted:

rPhys.m_hasColliders.ints().resize(rBasic.m_activeIds.vec().capacity());

rBasic.m_activeIds is an IdRegistryStl.

Both IdRegistryStl and IdSetStl are internally just an std::vector<uint64_t> wrapped in a different interface. ints() and vec() obtains the internal vector.

The code above just resizes one vector to the capacity of the other. Of course this is kind of bad for leaking implementation details, but it's arguably a little faster.

This should work:

rPhys.m_hasColliders.resize(rBasic.m_activeIds.capacity());

I merged #281; maybe worth rebasing this PR

Lamakaio commented 6 months ago

Thank you, that makes a lot of sense !

I fixed it and rebased the PR (and removed the commit from #284 that I had left as well)