Suggestion: simplify c99 callback interface

[cloudwu]

bgfx use an adapter class CallbackC99 for callback interface now. I don't think we should maintain c99 callback interface the same memory layout with C++ callback interface.

Because of only one bgfx instance allowed in one process, we don't need bgfx_callback_interface_t* _this in callback function.

I suggest that we can define callback functions directly in bgfx_callback_interface_t , and remove bgfx_callback_vtbl_s like :

typedef struct bgfx_callback_interface_s
{
    void (*fatal)(const char* _filePath, uint16_t _line, bgfx_fatal_t _code, const char* _str);
    void (*trace_vargs)(const char* _filePath, uint16_t _line, const char* _format, va_list _argList);
    void (*profiler_begin)(const char* _name, uint32_t _abgr, const char* _filePath, uint16_t _line);
    void (*profiler_begin_literal)(const char* _name, uint32_t _abgr, const char* _filePath, uint16_t _line);
    void (*profiler_end)(bgfx_callback_interface_t* _this);
    uint32_t (*cache_read_size)(uint64_t _id);
    bool (*cache_read)(uint64_t _id, void* _data, uint32_t _size);
    void (*cache_write)(uint64_t _id, const void* _data, uint32_t _size);
    void (*screen_shot)(const char* _filePath, uint32_t _width, uint32_t _height, uint32_t _pitch, const void* _data, uint32_t _size, bool _yflip);
    void (*capture_begin)(uint32_t _width, uint32_t _height, uint32_t _pitch, bgfx_texture_format_t _format, bool _yflip);
    void (*capture_end)();
    void (*capture_frame)(const void* _data, uint32_t _size);
} bgfx_callback_interface_t;

It we need an additional user object, passing a void *ud by bgfx_init through bgfx_callback_interface_t would be better ( more C style) :

typedef struct bgfx_callback_interface_s
{
    void *ud;   // an user object
    void (*atexit)(void *ud);   // It should be call from CallbackC99::~CallbackC99()
    void (*fatal)(void *ud, const char* _filePath, uint16_t _line, bgfx_fatal_t _code, const char* _str);
    void (*trace_vargs)(void *ud, const char* _filePath, uint16_t _line, const char* _format, va_list _argList);
    ...
} bgfx_callback_interface_t;

We should add the atexit callback function for destruction, and we have no change to destroy bgfx_callback_interface_t object when we use C99 interface now.

[bkaradzic]

I don't see benefit of doing this.

[bkaradzic]

I forgot to answer on this, because I was trying to figure out what's advantage, and couldn't figure out any. I assume it simplifies something but not sure what?!

[cloudwu]

For C , it’s hard to implement a callback interface. We should simulate a C++ like class. But there is no advantage for keeping a C++ class memory layout .

And for c++ callback interface , we can define a destructor to delete this class , but there is no way to do that for current c interface.

When I implement bgfx lua binding, I found the only way to offer a callback interface is using a static/global struct , because I have no chance to free it.

https://github.com/cloudwu/lua-bgfx/blob/master/luabgfx.c#L353

My version copy all the callback function pointers into adapter class, and not holding the interface struct pointer in bgfx_init_t struct . It’s more stable , we can put bgfx_callback_interface_t on stack just like bgfx_init_t , and a little better performence by removing vtbl ( m_interface->vtbl->api to m_interface.api ).

Another advantage is that we can define callback only we need, and leave NULL for the callback we don’t care.

btw, If we want to keep ABI as COM, we should not define destructor in CallbackI https://github.com/bkaradzic/bgfx/blob/master/include/bgfx/bgfx.h#L419 , and add a new Release method. And then , We can remove adaptor CallbackC99 .

[cloudwu]

There is an example. If we want to hook trace_vargs , we should define all callback functions:

static bgfx_callback_vtbl_t vtbl = {
    cb_fatal,
    cb_trace_vargs,
    cb_profiler_begin,
    cb_profiler_begin_literal,
    cb_profiler_end,
    cb_cache_read_size,
    cb_cache_read,
    cb_cache_write,
    cb_screen_shot,
    cb_capture_begin,
    cb_capture_end,
    cb_capture_frame,
};

static bgfx_callback_interface_t cb = { &vtbl };

bgfx_init_t init;
bgfx_init_ctor(&init);

init.callback = &cb;

If bgfx add a new callback in future, we should add it too.

By my version, we can simplify these code:

bgfx_callback_interface_t cb;
memset(&cb, 0, sizeof(cb));
cb.trace_vargs = cb_trace_vargs;

bgfx_init_t init;
bgfx_init_ctor(&init);

init.callback = &cb;

We don't need change code when bgfx add new callback or change the callback we don't care.

Another example for user defined allocator.

static void* cb_alloc(bgfx_allocator_interface_t* _this, void* _ptr, size_t _size, size_t _align, const char* _file, uint32_t _line) {
    struct bgfx_my_allocator * alloc = (struct bgfx_my_allocator *)_this;
    return my_alloc(alloc->ud, _ptr, _size, _align, _file, _line);
}

struct bgfx_my_allocator {
    bgfx_allocator_interface_t base;
    void *ud;
};

static bgfx_callback_vtbl_t alloc_vtbl = {
    cb_alloc,
};

static struct bgfx_my_allocator cb = {
    { &alloc_vtbl },
    NULL,   // ud
};

bgfx_init_t init;
bgfx_init_ctor(&init);

init.allocator = &cb;

By my version,

bgfx_init_t init;
bgfx_init_ctor(&init);

bgfx_allocator_interface_t alloc = {
    NULL,   // ud
    NULL,   // destructor to free ud
    my_alloc,
};

init.allocator = &alloc;

[bkaradzic]

If we want to hook trace_vargs , we should define all callback functions:

Yes, this is equivalent to C++. I don't see this as an issue, you define all or none.

[cloudwu]

It’s not an issue, but I think c api can do it better.

The actually issue is that simulate a c++ object ( vtbl , virtual functions, etc ) in c is troublesome and meaningless . C has no syntactic sugar for virtual class or struct destructor , we must define two struct and maintain their lifetime .

So, my opinion is that offering callback functions only in c api rather than a c++ like object. The second example shows how it can simplify the user defined allocator.

[bkaradzic]

Then bgfx_init_t would need to have:

    bgfx_callback_t*  callback;
    void* callbackUserData;
    bgfx_allocator_t* allocator;
    void* allocatorUserData;

instead of:

    bgfx_callback_interface_t*  callback;
    bgfx_allocator_interface_t* allocator;

[cloudwu]

A minor issue is that bgfx_init is not equivalent to bgfx::init now, because we can define a destructor for callback object in c++ but we can’t do it in c.

[cloudwu]

We can put void * callbackUserdata in bgfx_callback_t , see my pr .