Skip to content
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

[meta] Getter interface for primitive opaque types #1403

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jpeletier
Copy link
Contributor

This PR adds support for reading opaque types that map to primitives without having to invoke a serializer that may output an unwanted type.

The opaque interface now has "get" callbacks that allow to define a function that will retrieve the value from the opaque.

Opaques are now transparent 😅 to cursors.

  struct MyOpaque {
    int value = 0;
  };

  auto my_component = ecs.component<MyOpaque>();
  my_component.opaque(flecs::I32)
      // will be called when setting an int:    
      .assign_int([](MyOpaque *data, int64_t value) {
             data->value = value;
      })
      // **new** will be called when trying to read as an int:
      .get_int([](const MyOpaque *data) -> int64_t {
            return data->value;
      });

  MyOpaque instance;

  flecs::cursor cur(ecs, my_component, &instance);

  cur.set_int(79);
  std::cout << "value = " 
      << cur.get_int()  // **new** this now will work.  
      << std::endl;  // value = 79

@SanderMertens
Copy link
Owner

SanderMertens commented Dec 1, 2024

How does this work when you have a type with multiple members? Wouldn't it be possible to implement this with the existing ensure_member/ensure_element callbacks?

@jpeletier
Copy link
Contributor Author

How does this work when you have a type with multiple members?

This PR only deals with reading an opaque as a primitive type. PR #1405 is dedicated to collection types and introduces get_member for that purpose (read-only version of ensure_member). It could be just one PR, but wanted to do it in two steps for clarity.

In the example above, the opaque is implemented as struct, perhaps the example is contrived. But note that in the example, the as_type is flecs::I32 meaning it wants to be seen as an int. There is no member with a name to query for, and the only chance to read the value before this PR is to invoke serialize() and hope the value callback provides a flecs:I32.

Background: https://discord.com/channels/633826290415435777/1295740423293108298 where you considered this as missing in the API.

In essence, this PR only provides the symmetric side of the assign_ collection of handlers. It makes ecs_get_* work with the remaining opaques, (ecs_get_string already did some work to be able to read a string out of an opaque)

The idea here is that if an opaque, of whatever as_type implements, for example, get_int it means the opaque is somehow convertible to int, like if it implemented operator int(). Conversely, when it implements assign_int it is as if it implemented an assign operator, e.g., MyOpaque& operator=(int x). That is at least how I understand it.

Wouldn't it be possible to implement this with the existing ensure_member/ensure_element

I consider ensure_xx as writing. The semantics of the word ensure, as I have understood in other parts of Flecs is "create the item if it did not exist but give me a pointer to something I can write to". Also, it requires "write access", that is a non-const pointer. Can you read with ensure_xxx ? yes, but the developer that wrote the opaque implementation did not intend ensure to be used for reading and may even create an element for you to read, altering the underlying data. Implementation of ensure actually grows vectors if necessary in order to accomodate the requested item. If I only want to read, why alter the vector?

As a summary, this just helps making the API symmetrical.

@SanderMertens
Copy link
Owner

SanderMertens commented Dec 2, 2024

The semantics of the word ensure, as I have understood in other parts of Flecs is "create the item if it did not exist but give me a pointer to something I can write to".

That's correct- so would it be correct to say that this functionality could be implemented with just a get_member and get_element?

Never mind, I see that this is just for primitive types. Makes sense now. Let me think for a bit on the interface (whether I prefer this or a single untyped get callback).

@jpeletier
Copy link
Contributor Author

Here is my full analysis, for your review:

option 1: single, untyped get call:

  1. (optional) We could just reuse get_element, ignoring the element index parameter, or reuse get_member ignoring the member parameter.
  2. API would not be symmetrical, because in the other side you have all the assign_ methods, one for each primitive type family (integers/unsigned, floats, bool, string...)
  3. When calling get_element this way, or otherwise having an untyped get, you need to know the type you are dealing with in advance. This means looking at the as_type to understand what type si to be expected.
  4. You lose type conversion capacity. Perhaps the opaque stores an integer, but is able to return a float version if asked.

option 2: Using a per-type get_ call (this PR):

  1. API is symmetrical, assign vs get.
  2. Opaque developer can provide different ways of accesing the data, with conversion. The caller can choose the most convenient. This practice happens on the assign side, for example, cursor.c:986 (ecs_meta_set_int()), where the most appropriate handler is chosen.

Neither of the above solves the problem when the underlying opaque data does not match the as_type, especially when a string is requested. Therefore I propose the following third option, similar to serialize:

option 3: single, untyped get call with callback:

// Define a struct to manage the call to opaque's `get` callback.
// Similar to ecs_serializer_t
struct ecs_opaque_getter_t {
    const ecs_world_t *world;
    void *ctx;
    union { // Ignored for primitives
        const char *member; // In case it is a struct
        ecs_size_t elem; // In case it is a vector/array
    };
    void (*value)(ecs_opaque_getter_t *getter, const void *item);
};

The get handler for the opaque is thus defined as:

void (* get)(ecs_opaque_getter_t * getter, const void* data);

And in C++, usage would look as follows:

struct  MyOpaque {
	float  value; // whatever internal implementation
};

ecs.component<MyOpaque>()
    .opaque(flecs::I32) // promise to behave as `int`.
    .get([](const ecs_opaque_getter_t *getter, const MyOpaque *data) {
        // `as_type` is flecs::I32, a primitive, so we ignore the member/elem field
        // Convert internal representation to int:
        int vint = int(data->value);  
        // Invoke the callback with an int*, as promised.
        getter->value(getter, &vint);
        // Chance to cleanup, in this case `vint` just goes out of scope.
    });

Here is a different example, where the as_type is flecs::String:

struct  MyOpaque {
	float  value; // whatever internal implementation
};

ecs.component<MyOpaque>()
    .opaque(flecs::String) // promise to behave as `string`.
    .get([](const ecs_opaque_getter_t *getter, const MyOpaque *data) {
        // `as_type` is flecs::String, a primitive, so we ignore the member/elem field
        // Convert internal representation to string:
        int len = snprintf(NULL, 0, "%f", data->value);
        char *str = malloc(len + 1);
        snprintf(str, len + 1, "%f", data->value);
        // Invoke the callback with a const char**, as promised:
        getter->value(getter, &str);
        // Chance to cleanup:
        free(str);
    });

It would be nice to have the opposite, set, and remove all the assign set of handlers, but this would require more work, since assign_* is already used in a number of places.

In my opinion, the as_type represents an interface contract for the opaque, and the external user should not expect the opaque to be able to process other input or deliver any output different from the as_type it promised to behave as: the opaque implementer should not have to provide many assign_ handlers.

Please let me know what you think to move this PR forward in the best possible direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants