KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
866 stars 227 forks source link

No definition for ktxTexture2_Destroy in ktx.h #911

Closed matthew-rister closed 4 months ago

matthew-rister commented 4 months ago

libktx documentation references ktxTexture2_Destroy, however, there is no definition for ktxTexture2_Destory in ktx.h. The only mention of this function is in a \copydoc doxygen command. Is this a bug or am I missing something?

This is problematic because without this method it seems like the only way to destroy a ktxTexture2 object is to cast it to ktxTexture which can't be done with a static_cast and therefore requires the unsafe use of reinterpret_cast to call ktxTexture_Destroy.

matthew-rister commented 4 months ago

This is also true for ktxTexture1_Destroy. I see both are defined in texture1.c and texture2.c respectively.

MarkCallow commented 4 months ago

ktxTexture_Destroy is a virtual function so exposing the underlying functions did not seem necessary as it will do the right thing regardless of the type of texture passed. Why is a reinterpret_cast to ktxTexture unsafe? Both ktxTexture1 and ktxTexture2 are derived from ktxTexture so it will always be valid.

matthew-rister commented 4 months ago

Respectfully, it seems questionable API design to require users to cast to a different type to correctly invoke the only public texture destroy function. I already know the type I'm working with is a ktxTexture2 so why do I need to explicitly cast to the base class so it can call the proper ktxTexture2_Destroy for me when I can just do that myself?

This also introduces additional complexity in C++ when using smart pointers like std::unique_ptr. If ktxTexture2_Destroy was defined in ktx.h, I could simply use std::unique_ptr<ktxTexture2, decltype(&ktxTexture2_Destroy)> to define a smart pointer to a KTX texture. Instead, I need define an additional function which handles the casting which seems unnecessary.

As for reinterpret_cast, it's prohibited by the C++ Core Guidelines because it is not type safe compared to static_cast and dynamic_cast.