[core] operator== hook #1455

jpeletier · 2024-11-29T13:45:03Z

Note, requires #1454 to be merged first.

Building on the previous PR, this one adds a hook for operator==. It will detect this operator in a component definition and bind a hook to it that can be used to test components for equality.

If this operator is not defined but the compare operator is, Flecs will automatically derive this operator.

    class Vec2D {
    public:
        float x, y;
    
        Vec2D(float x = 0.0f, float y = 0.0f) : x(x), y(y) {}
    
        // Function to calculate the modulus (magnitude) of the vector
        float modulus() const {
            return std::sqrt(x * x + y * y);
        }
    
        // Overload operator< to compare vectors based on their modulus
        bool operator<(const Vec2D& other) const {
            return this->modulus() < other.modulus();
        }
    
        // Overload operator== to compare vectors with a precision tolerance of 0.001
        bool operator==(const Vec2D& other) const {
            return std::fabs(this->x - other.x) < 0.001 && std::fabs(this->y - other.y) < 0.001;
        }
    };

    flecs::world ecs;

    auto vtype =
        ecs.component<Vec2D>();  // will automatically register the above hooks.

    Vec2D a(1, 2);
    Vec2D b(2, 3);
    Vec2D c(1.00001, 1.9999999);

    const ecs_type_info_t *ti = ecs_get_type_info(ecs, vtype);

    assert(ti->hooks.equals(&a, &b, ti) == false); // ok!
    assert(ti->hooks.equals(&a, &c, ti) == true);  // ok!
    assert(ti->hooks.cmp(&b, &a, ti) > 0);         // ok!

SanderMertens · 2025-01-07T22:32:16Z

include/flecs/addons/cpp/component.hpp

@@ -397,14 +442,14 @@ struct component : untyped_component {
    template <typename Func>
    component<T>& on_add(Func&& func) {
        using Delegate = typename _::each_delegate<typename std::decay<Func>::type, T>;
-        flecs::type_hooks_t h = get_hooks();
+        flecs::type_hooks_t h = this->get_hooks();


Nit: remove this-> so it's consistent with set_hooks

SanderMertens · 2025-01-07T22:32:32Z

include/flecs/addons/cpp/component.hpp

@@ -429,14 +474,48 @@ struct component : untyped_component {
    component<T>& on_set(Func&& func) {
        using Delegate = typename _::each_delegate<
            typename std::decay<Func>::type, T>;
-        flecs::type_hooks_t h = get_hooks();
+        flecs::type_hooks_t h = this->get_hooks();


Nit: see above

SanderMertens · 2025-01-07T22:34:24Z

include/flecs/addons/cpp/component.hpp

+
+    /** Register operator compare hook. */
+    using untyped_component::op_compare;
+    component<T>& op_compare() {


I think I made up my mind about using on_X naming. It makes more sense to me, since it's still a hook that will be invoked when the core needs to do a comparison, so it is "on compare", "on equals".

SanderMertens · 2025-01-07T22:38:21Z

src/addons/meta/rtt_lifecycle.c

-        dtor_hook_required,
-        move_hook_required,
-        copy_hook_required);
+        ctor_hook_required ? flecs_rtt_struct_ctor : NULL,


Can we keep the existing design where we just pass in booleans to flecs_rtt_init_default_hooks_struct?

Fixed.
This change made sense in some internal refactor. Not anymore.

SanderMertens · 2025-01-07T22:39:22Z

src/addons/meta/rtt_lifecycle.c


    ecs_type_hooks_t hooks = *ecs_get_hooks_id(world, component);
+
+    hooks.ctor = ctor_hook_required && !(flags & ECS_TYPE_HOOK_CTOR_ILLEGAL) ? 


Why was this code added? It doesn't seem related to equals/compare

This is due to the new flags checks in ecs_set_hooks_id(). It now requires that hooks are set to NULL if they are to be illegal: The PR enables the commented-out asserts in there:

/* TODO: enable asserts once RTT API is updated */

... and also unquarantines several tests in ComponentLifecycle.c.

SanderMertens · 2025-01-07T22:40:19Z

src/addons/meta/rtt_lifecycle.c


    if (hooks.lifecycle_ctx_free) {
        hooks.lifecycle_ctx_free(hooks.lifecycle_ctx);
-        hooks.lifecycle_ctx_free = NULL;
+        hooks.lifecycle_ctx_free = flecs_rtt_free_lifecycle_nop;


Why do we need a nop? Can we keep the existing approach?

The existing approach didn't actually work as intended. Specifically, ecs_set_hooks_id does not allow you to unset lifecycle_ctx_free once it's been set.

This issue went unnoticed in other cases (e.g., for ctor, dtor, etc.) because if any field in a struct required a runtime-generated hook, the struct would always continue requiring hooks as new fields were added. In those cases, the lifecycle context wouldn't need to be removed.

However, handling cmp and equals hooks is different. Initially, a struct might be comparable because its first field is comparable, and so an autogenerated compare hook is added. But if a later field is not comparable, the entire struct becomes uncomparable, even if further comparable fields are added later. This means the lifecycle context must be removed, unless the struct coincidentally still requires other hooks (e.g., ctor or dtor).

Since ecs_set_hooks_id doesn't support unsetting lifecycle_ctx_free, I opted to replace it with a no-op function.

I could change ecs_set_hooks_id to do what I want and avoid the nop, but that would break a pattern in there, so it is better to have this conversation and decide.

So that's why 😉

SanderMertens · 2025-01-07T22:40:44Z

src/addons/meta/rtt_lifecycle.c

    }

-    if (ctor_hook_required || dtor_hook_required || move_hook_required ||
-        copy_hook_required) {
+    if (hooks.ctor || hooks.dtor || hooks.move ||


Why did the existing approach change here?

Same as above, due to the refactor required to comply with ecs_set_hooks_id() strict flags check.

SanderMertens · 2025-01-07T22:41:52Z

src/world.c

@@ -1464,9 +1539,10 @@ void ecs_set_hooks_id(
    }

    if(ti->hooks.flags & ECS_TYPE_HOOK_MOVE_DTOR_ILLEGAL) {
-        ti->hooks.ctor_move_dtor = flecs_move_ctor_illegal;
+        ti->hooks.move_dtor = flecs_move_ctor_illegal;


Why did this change?

This is unrelated to this PR, I just spotted this bug and fixed it along the way before I forgot: The wrong hook was being set to the flecs_move_ctor_illegal illegal handler.

SanderMertens · 2025-01-07T22:43:03Z

test/cpp/src/ComponentLifecycle.cpp

+    int16_t value;
+};
+
+int compare(flecs::world& ecs, flecs::entity_t id, const void *a, const void *b) {


Can you make these functions static

ComponentLifecycle tests first version, pending tests and cpp change default compare behavior remove flag fix clang operator== warning. always set default hook. fix wfloat-equal warning fix string comparer refactor rtt tests fix (const char *const *) add compare test file

…ests

r

Allow in cpp to generate equals hook from compare hook

…ing function

jpeletier force-pushed the equals-hook branch 7 times, most recently from fa1a619 to 2c416ea Compare December 5, 2024 17:01

jpeletier force-pushed the equals-hook branch 2 times, most recently from b4af2c5 to 9fb4d95 Compare December 23, 2024 11:13

SanderMertens requested changes Jan 7, 2025

View reviewed changes

jpeletier added 20 commits January 8, 2025 10:07

restored

869b4a4

ComponentLifecycle tests first version, pending tests and cpp change default compare behavior remove flag fix clang operator== warning. always set default hook. fix wfloat-equal warning fix string comparer refactor rtt tests fix (const char *const *) add compare test file

make compare == NULL mean illegal

08a2a62

Enable ecs_set_hooks_id asserts and unquarantine ComponentLifecycle t…

137dadd

…ests

add primitive compare tests

8f3b445

complete tests by adding more basic testing first

c223c6c

some comments in primitive comparer tests

9eb7a62

Rename Compare.c to PrimitiveCompare.c

838f795

fix cpp compare tests

3009c60

extend disable float == warning

1bc809d

more tests

511fa98

Finish tests

6de8ef4

fixt test-windows compilation error due to const array size

886e4df

r

Rename comp to cmp for clarity

6ff7df3

add equals hook type

6425012

Implement equals hook. Add tests to PrimitiveCompare

e82a7ee

Equals hook lifecycle traits

1f64ead

Added equals test to RttCompare

177e974

renamed function in RuntimeTypes test

114c84d

Add equals hook illegal test.

377d1e9

Allow in cpp to generate equals hook from compare hook

added op_equals, op_compare. Tests.

6362cb1

jpeletier added 5 commits January 8, 2025 10:07

fix quirks (1)

ab85734

fix cast error c/c++

626976c

support enums

8b0b1c2

fix flag

7d61b63

Add cmp/equals support for custom enum types

07b2680

jpeletier force-pushed the equals-hook branch from 9fb4d95 to 07b2680 Compare January 8, 2025 12:31

rename op_compare/op_equals. Refactor rtt_lifecycle struct hooks sett…

d52c651

…ing function

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] operator== hook #1455

[core] operator== hook #1455

jpeletier commented Nov 29, 2024

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025

SanderMertens Jan 7, 2025

jpeletier Jan 8, 2025


		ecs_type_hooks_t hooks = *ecs_get_hooks_id(world, component);

		hooks.ctor = ctor_hook_required && !(flags & ECS_TYPE_HOOK_CTOR_ILLEGAL) ?

[core] operator== hook #1455

Are you sure you want to change the base?

[core] operator== hook #1455

Conversation

jpeletier commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment