Skip to content

Add pair overload to get_ref #1190

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 21, 2024
Merged

Add pair overload to get_ref #1190

merged 3 commits into from
Apr 21, 2024

Conversation

TBlauwe
Copy link
Contributor

@TBlauwe TBlauwe commented Apr 19, 2024 •
edited
Loading

First time creating a pull request, I'm sorry in advance if I did something the wrong way !

This pull request adds a pair overload to get_ref to support pair types, so that a ref to the actual type is returned rather than a ref to the pair.

Here is a repro :

struct A {};
struct B {int value {0};};
using AB = flecs::pair<A, B>;
using SharedAB = flecs::pair<A, flecs::ref<B>>;

int main(int argc, char **argv)
{
    flecs::world world;
    auto a = world.entity("a");
    auto b = world.entity("b");
    a.add<AB>();
    b.set<SharedAB>({a.get_ref<AB>()});// This doesn't work. 
                                       // Returns flecs::ref<flecs::pair<A, flecs::ref<B>>>
    b.set<SharedAB>({a.get_ref<A, B>()}); // A workaround
                                          // Returns flecs::ref<B>
}

Changes were made to the normal get_ref to dissambiguate with the newly added overload.

@SanderMertens
Copy link
Owner

Looks good! Can you add a test that makes sure the right pair gets added?

The CI failure is not due to this PR, investigating at the moment.

@TBlauwe
Copy link
Contributor Author

TBlauwe commented Apr 21, 2024

Done ! Although I may have misunderstood you. Should I also add these ?

// In Entity.cpp
auto e = world.entity().set<PositionTag>({10, 20});
test_bool((e.has<Position, Tag>()), true);
test_bool((e.has<Tag, Position>()), false);
// ....
auto e = world.entity().set<TagPosition>({10, 20});
test_bool((e.has<Position, Tag>()), false);
test_bool((e.has<Tag, Position>()), true);

If so, can I add these two using in cpp_api.h ?

// In cpp_api.h
using PositionTag = flecs::pair<Position, Tag>;
using TagPosition = flecs::pair<Tag, Position>;

@copygirl
Copy link
Contributor

copygirl commented Apr 21, 2024 •
edited
Loading

Would it make more sense for the test and reader for the test to mix setting <PositionTag> with getting via <Position, Tag> (for example), both to make sure they mean the same thing to the library, and for the reader to see these are equivalent?

@SanderMertens
Copy link
Owner

LGTM! The test looks good as is, using shouldn't change the behavior here.

@SanderMertens SanderMertens merged commit 7b6b108 into SanderMertens:master Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TBlauwe @SanderMertens @copygirl