-
Notifications
You must be signed in to change notification settings - Fork 34
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
Replace osp::BitVector_t with lgrn::IdSetStl #285
Conversation
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 |
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). |
Try limiting parallelism in the build. `cmake --build path/to/build --parallel numberParallelJobs`` |
You forgot the rPhys.m_hasColliders.ints().resize(rBasic.m_activeIds.vec().capacity());
Both 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 |
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
a76e382
into
TheOpenSpaceProgram:master
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.