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

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

Merged
merged 3 commits into from
May 31, 2024

Conversation

Lamakaio
Copy link
Contributor

@Lamakaio Lamakaio commented May 28, 2024

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

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

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

jonesmz commented May 28, 2024

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

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 Lamakaio marked this pull request as ready for review May 29, 2024 09:10
@Lamakaio
Copy link
Contributor Author

Lamakaio commented May 29, 2024

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)

@@ -133,8 +133,8 @@ entt::any setup_scene(osp::Resources& rResources, osp::PkgId const pkg)

// Resize some containers to fit all existing entities
std::size_t const maxEnts = rScene.m_activeIds.vec().capacity();
rScene.m_matPhong.ints() .resize(maxEnts);
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this was oversized by 64x previously

@Capital-Asterisk Capital-Asterisk self-requested a review May 30, 2024 02:53
Copy link
Contributor

@Capital-Asterisk Capital-Asterisk left a comment

Choose a reason for hiding this comment

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

looks good!

@Capital-Asterisk Capital-Asterisk merged commit a76e382 into TheOpenSpaceProgram:master May 31, 2024
22 checks passed
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.

3 participants