When using components in a relationship pair, data is mixed if writing to first or second for the same pair

[jpeletier]

Describe the bug
When both elements of a pair are components and different entities are set with data in either first or second, data is put in the same column and considered the same in queries. Flecs detects when components are of different size and aborts, but if both happen to be of the same size, execution continues but data is reinterpreted.

To Reproduce

// the example assumes sizeof(int) == sizeof(float); true in GCC x64 / Linux.

  struct A {
    int value;
  };

  struct B {
    float value;
  };

  assert( sizeof(A) == sizeof(B) );

  auto e1 = ecs.entity("E1");
  auto e2 = ecs.entity("E2");

  auto a = ecs.component<A>();
  auto b = ecs.component<B>();

  e1.set<A>(b, {.value = 3});
  e2.set_second<B>(a, {.value = 4.0});

  std::cout << "first query (A,B) with 'B' containing data:" << std::endl;
  ecs.query_builder<const B>()
    .term_at(0).first(a).second(b)
    .build().each([](flecs::entity e, const B &b) {

    std::cout << "e=" << e.name().c_str() << " value = " << b.value << std::endl;

  });

  std::cout << "second query (A,B), with A containing data:" << std::endl;
  ecs.query_builder<const A>()
    .term_at(0).second(b)
    .build().each([](flecs::entity e, const A &a) {
    
    std::cout << "e=" << e.name().c_str() << " value = " << a.value << std::endl;
  
});

Output is:

first query (A,B) with 'B' containing data:
e=E1 value = 4.2039e-45 <-- int reinterpreted as float
e=E2 value = 4
second query (A,B), with A containing data:
e=E1 value = 3
e=E2 value = 1082130432 <-- float reinterpreted as int.

Expected behavior
I was expecting the following at first:

first query (A,B) with 'B' containing data:
e=E2 value = 4
second query (A,B), with 'A' containing data:
e=E1 value = 3

But really, I think that if data has been already set on the first element of a given pair, the pair should acquire the type of the first element during the lifetime of the world; and therefore any subsequent set must be also on the first element. If the condition is not satisfied, the program should abort as may be this is a programming error:

  e1.set<A>(b, {.value = 3}); // ok, first time we see (A,B)
  e2.set_second<B>(a, {.value = 4.0}); // crash now, does not make sense.

Perhaps this is a non-issue. Nevertheless I believe it should at least be documented somewhere, as a caveat or possible pitfall and wanted to let everyone know just in case :-)

EDIT:

ecs_get_type_info(ecs, ecs_pair(a, b)); resolves the tie and returns A as the underlying type for this particular example, therefore I would actually expect:

  e2.set_second<B>(a, {.value = 4.0});

to crash outright, since the underlying type for the pair is A, this operation should not be allowed.

[copygirl]

I thought this was documented somewhere. For a relationship pair:

• If the relationship has the EcsTag component, it doesn't hold any value.
• If the relationship is a component itself, that's its type.
• If the relationship is not a component, but its target is, that is its type.
• Otherwise it doesn't have a type, and doesn't hold a value.

There is no type check in place because I don't know if that's possible in Flecs. Components only store size+alignment so if you were to read/write it as the incorrect type, which you're doing here, then that's really just a programmer error?

[jpeletier]

Yes, what you mention is documented here: https://www.flecs.dev/flecs/md_docs_2Relationships.html#relationship-components

I understand this is a programmer error, but no warning or error is given, other than receiving garbage data in queries, which is confusing.

[copygirl]

From what I understand, Flecs can figure out the type of a pair given two type parameters, so in theory it could be possible to assert that, which would require using a set<A, B>(value) function (which I'm unsure if that exists). Using set<A> and set_second<B> doesn't give the compiler enough information to check if what you're doing is correct. (And then there's the fact that, regardless of this, adding EcsTag will cause the relationship to not have ANY type, which is done at runtime, so even if it figured out a type during compile time, that could still be incorrect.)

And I also don't believe it would be possible to get any sort of compile time type checking in a query through the chained .term_at(0).first(a).second(b) functions.

[jpeletier]

Well, I was not expecting a compile-time error, but at least some sort of runtime-error / abort.

Actually, I found out that:

  auto ti = ecs_get_type_info(ecs, ecs_pair(a, b));
  std::cout << "name:" << ti->name << std::endl;

Prints name: A, therefore Flecs already understands that for this specific pair in my example, where both types are components the underlying data type should be A. Therefore:

 e2.set_second<B>(a, {.value = 4});

should crash.

Perhaps set should check when setting a pair with first:

 assert( ecs_get_type_info(ecs, ecs_pair(first, second))->component == first );

and set_second should check:

 assert( ecs_get_type_info(ecs, ecs_pair(first, second))->component == second );

at runtime.

[copygirl]

You're right. It may be worth considering runtime debug asserts for this.

[Indra-db]

@copygirl @jpeletier

Something worth considering, what the Rust API does for flecs,

I panic when calling a bunch of functions like

  ecs.query_builder<const B>()
    .term_at(0).first(a).second(b)

when referencing a generic/templated parameter

this because rust has to be type safe by default

code in question:

fn check_term_access_validity<'a>(term: &impl TermBuilderImpl<'a>) {
    if term.current_term_index() < term.count_generic_terms()
        && term.current_term_ref_mode() != TermRefMode::Src
    {
        panic!("This function should only be used on terms that are not part of the generic type signature. ")
    }
}


/// * C++ API: `term_builder_i::first`
fn set_first<T: ComponentId>(&mut self) -> &mut Self {
    check_term_access_validity(self);
    self.set_first_id(T::id(self.world()))
}

count_generic_terms is the amount of template parameters supplied.

query<Position> // 1 count_generic_terms
query<Position, Velocity> // 2 count_generic_terms
query<Position,Velocity,Mass> // 3 count_generic_terms

so the if check ensures you don't use .term_at with several functions that could invalidate the type itself. This is fine because there's ways provided by the Rust API (and C++ API) to provide alternatives, for example in your use case, you could do

ecs.query_builder<const Pair<A,B>>()

which would select the correct type for you with the same functionality

[Indra-db]

this of course comes with a performance penalty, but in this case, for building systems and queries, it really shouldn't matter.

[SanderMertens]

@jpeletier the behavior that determines which type a pair assumes is documented here: https://www.flecs.dev/flecs/md_docs_2Relationships.html#relationship-components

I think this can be caught early by an assert. Will take a look.

[jpeletier]

Yes, I am aware of that. Every now and then come back to re-read it! :-)

I wonder if the solution is too expensive, I can't think of anything other than:

set should check when setting a pair with first:

 assert( ecs_get_type_info(ecs, ecs_pair(first, second))->component == first );

and set_second should check:

 assert( ecs_get_type_info(ecs, ecs_pair(first, second))->component == second );

But that implies bringing over type info at a performance penalty. Is there any bitmask wizardry that could return the pair's type?

[SanderMertens]

Unfortunately not, the asserts you're suggesting would be the right way to do it. It would add performance overhead in debug mode, though it shouldn't be prohibitive.

[Indra-db]

@jpeletier the rust API does the same

pub fn set_id<T>(self, data: T, id: impl IntoId) -> Self
{
    let world = self.world.world_ptr_mut();
    let id = *id.into();
    let data_id = T::id(self.world);
    let id_data_id = unsafe { sys::ecs_get_typeid(world, id) };
    if data_id != id_data_id {
        panic!("Data type does not match id type. For pairs this is the first element occurrence that is not a zero-size
    }

pub fn set_first<First>(self, first: First, second: impl Into<Entity>) -> Self
{
   let world_ptr = self.world.world_ptr_mut();
   let first_id = First::id(self.world);
   let second_id = *second.into();
   let pair_id = ecs_pair(first_id, second_id);
   let data_id = unsafe { sys::ecs_get_typeid(world_ptr, pair_id) };
   if data_id != first_id {
       panic!("First type does not match id data type. For pairs this is the first element occurrence that is not a zero-
   }

[Indra-db]

most (hopefully all) of these safety regards, have been solved in the rust API, since rust requires it, so feel free to take a peek there or ask me

[SanderMertens]

Fixed!