john-chapman / im3d

Immediate mode rendering and 3d gizmos.
MIT License
1.18k stars 63 forks source link

Default IM3D_MALLOC/IM3D_FREE macros should be defined in `im3d.h` #62

Closed ChemistAion closed 1 year ago

ChemistAion commented 1 year ago

Otherwise they are not visible outside, IMHO defining them first in a custom im3d_config.h is not elegant :sweat_smile:

Use case: im3d as a static-lib

ChemistAion commented 1 year ago

Please consider this: https://github.com/ocornut/imgui/blob/6b2e03c5b1bc4f99dbd86d10e5fd12af9e3fe1c2/imgui.h#L1772-L1779

john-chapman commented 1 year ago

I don't think I understand the use case - these macros are only used internally so there's no need for the defaults to be externally visible.

ChemistAion commented 1 year ago

I am wrapping im3d into im3d_impl_win32 and im3d_impl_dx12 similarly to: https://github.com/ocornut/imgui/tree/master/backends

This req allows to be consistent with im3d in such a wrapper (even defaults with malloc/free does not trigger ctor/dtor), e.g.:

bool Im3d_ImplDX12_Init(ID3D12Device* device, int num_frames_in_flight, DXGI_FORMAT rtv_format, ID3D12DescriptorHeap* heap)
{
    Im3d::AppData& ad = Im3d::GetAppData();
    IM3D_ASSERT(ad.m_rendererData == nullptr && "The im3d already initialized at platform backend!");
    //  IM3D_ASSERT(device);
    //  IM3D_ASSERT(num_frames_in_flight > 0);
    //  IM3D_ASSERT(heap);

    Im3d_ImplDX12_Data* data = (Im3d_ImplDX12_Data*)IM3D_MALLOC(sizeof(Im3d_ImplDX12_Data));
    ad.m_rendererData = (void*)data;

    data->device_ = device;
    data->heap_ = heap;
    data->num_frames_in_flight_ = num_frames_in_flight;
    data->frame_index_ = UINT_MAX;
    data->rtv_format_ = rtv_format;
    data->frame_resources_ = (Im3d_ImplDX12_RenderBuffers*)IM3D_MALLOC(num_frames_in_flight * sizeof(Im3d_ImplDX12_RenderBuffers));

    for (int index = 0; index < num_frames_in_flight; ++index)
    {
        Im3d_ImplDX12_RenderBuffers* render_buffer = &data->frame_resources_[index];

        render_buffer->index_buffer_ = nullptr;
        render_buffer->vertex_buffer_ = nullptr;
        render_buffer->index_buffer_size_ = 0;
        render_buffer->vertex_buffer_size_ = 0;
    }

    (...)

    return true;
}

btw: IM3D_ASSERT is visible from im3d.h :innocent:

ChemistAion commented 1 year ago

...finally, let say that we could close it since I will "break" im3d internals for these wrappers and use local new/delete scheme

john-chapman commented 1 year ago

It wouldn't break the internals, but it would be a breaking change for users who have set up the im3d_config.h file. The purpose of that is to have a single, central place for users to configure the library and then receive updates by overwriting the other library files. No, it's not elegant but it is very simple!

It's common for users to extend Im3d by adding stuff to im3d_config.h - so in your case adding IM3D_NEW(),IM3D_DELETE(), etc. macros there would be in line with the typical way of extending the library.

ChemistAion commented 1 year ago

Yes, I understand. Let's proceed then with the current approach for extending the library :rocket: