-
-
Notifications
You must be signed in to change notification settings - Fork 460
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
Implement a world-local component id caching API and make use of it in cpp components #1036
base: master
Are you sure you want to change the base?
Conversation
2ecb8d5
to
d3a55a2
Compare
@SanderMertens This is almost finished, could you update the unit tests afterwards to your preference? |
Sure no problem! A few things before this can get merged:
ecs_time_t t = {};
t = ecs_time_measure(&t);
flecs::entity e = world.entity();
for (int i = 0; i < 1000 * 1000; i ++) {
e.add<Foo>(); // noop after the first call, so less time spent measuring internals
}
printf("time spent = %f\n", ecs_time_measure(&t)); Make sure to measure once the majority of tests are passing. |
db46049
to
8f01b1e
Compare
I haved benchmarked my approach with your provided code and interestingly it seems slightly faster for some reason (let me know if you get the same results):
The tests still fail on some platforms, probably due to this code, which needs proper handling of the new logic: // To support member<uintptr_t> and member<intptr_t> register components
// (that do not have conflicting symbols with builtin ones) for platform
// specific types.
if (!flecs::is_same<i32_t, iptr_t>() && !flecs::is_same<i64_t, iptr_t>()) {
// TODO
const entity_t id = flecs::_::cpp_type<iptr_t>::id_explicit(world, nullptr, true, flecs::Iptr);
(void)id;
ecs_assert(id == flecs::Iptr, ECS_INTERNAL_ERROR, NULL);
// Remove symbol to prevent validation errors, as it doesn't match with
// the typename
ecs_remove_pair(world, flecs::Iptr, ecs_id(EcsIdentifier), EcsSymbol);
}
if (!flecs::is_same<u32_t, uptr_t>() && !flecs::is_same<u64_t, uptr_t>()) {
// TODO
const entity_t id = flecs::_::cpp_type<uptr_t>::id_explicit(world, nullptr, true, flecs::Uptr);
(void)id;
ecs_assert(id == flecs::Uptr, ECS_INTERNAL_ERROR, NULL);
// Remove symbol to prevent validation errors, as it doesn't match with
// the typename
ecs_remove_pair(world, flecs::Uptr, ecs_id(EcsIdentifier), EcsSymbol);
} Could you take a look at it? For me it is very unclear what the logical difference of |
fa04345
to
8f01b1e
Compare
Hmm, almost all C++ tests are still failing/segfaulting:
I'd recommend installing bake locally so you can run the test suite without having to push to CI, that should make debugging a lot easier. Re the difference between id/id_explicit, see the comments: // Obtain a component identifier for explicit component registration.
static entity_t id_explicit(world_t *world = nullptr,
const char *name = nullptr, bool allow_tag = true, flecs::id_t id = 0,
bool is_component = true, bool *existing = nullptr) // Obtain a component identifier for implicit component registration. This
// is almost the same as id_explicit, except that this operation
// automatically registers lifecycle callbacks.
// Additionally, implicit registration temporarily resets the scope & with
// state of the world, so that the component is not implicitly created with
// the scope/with of the code it happens to be first used by.
static id_t id(world_t *world = nullptr, const char *name = nullptr,
bool allow_tag = true) |
@SanderMertens I have tried installing bake locally but sadly it had errors building the tests, which I have fixed now. Overall it seems like the debugging from an IDE with bake is not possible since it does not seem to support IDE integration, which I would prefer for easier debugging. Otherwise I have to attach a debugger manually which is time-consuming. I have tried setting up the tests with CMake for better IDE integration, which is kind a complicated because it links with bake, but got it to compile: https://github.com/Naios/flecs/commits/cmake_test . Sadly the tests seem to have issues when being run from the CMake generated build files (test failures due to an assertion see below).
Also there is a crash in the call to Additionally, I have recognized that you run the tests in parallel from the same executable which is not ideal since it also makes debugging non-deterministic. It would be great if there is a flag to allow synchronous single-threaded deterministic execution of the tests as common C++ unit frameworks usually do (most of them run single-threaded with an option to opt-in for parallel execution that is done on a per-process level rather than a per-thread level). Overall I would have debugged this issues already, but the mentioned problems make it very difficult to do so. |
Running the tests is not hard, to run all C++ tests you can just do:
This runs each test in its own separate process, with multiple processes running in parallel To disable multithreading just specify you want to run on a single thread:
To debug a single test you can do
Also note how each test failure shows you the executable/command to run just that test, which allows you to run it in a debugger:
|
This is not what happens, each test is ran in its own process which ensures determinism & isolation. |
60a3a11
to
a016e60
Compare
1f78c8c
to
8374627
Compare
@SanderMertens I have fixed a few issues in this PR, and the failing unit tests look much more reasonable now. |
8374627
to
a3bd866
Compare
5ecdae8
to
cde4b4f
Compare
…n cpp components * Fixes potential conflicting component id issues when initializing different worlds with a different order. * Closes SanderMertens#1032
cde4b4f
to
da6ab6f
Compare
d99efe2
to
da0221e
Compare
438bde4
to
220852f
Compare
3b99938
to
ac6cdca
Compare
7874ea5
to
f479787
Compare
ace232e
to
b968586
Compare
301243a
to
4a5784f
Compare
34dc148
to
c9b99e0
Compare
As described in #1032
Note: This still requires support for copying the same components with different ids between worlds, for full correctness, but this will not be part of this PR.