cesium-native icon indicating copy to clipboard operation
cesium-native copied to clipboard

Gltf Reader: use STB_IMAGE_STATIC to avoid conflicts with other libs

Open TheMostDiligent opened this issue 1 year ago • 3 comments

The problem: CesiumGltfReader always uses STB_IMAGE_IMPLEMENTATION. If client application also uses stb_image, this results in duplicate symbol link errors. Moving stb_image.h into a namespace does not work because all functions are always defined as extern "C". (BTW, stb_image_resize.h includes quite a few standard headers (stdint.h, string.h, math.h, etc.). I am pretty sure that including standard headers from a namespace is not allowed in C++)

Solution: to add an option to disable STB_IMAGE_IMPLEMENTATION. With this, the problem can be solved as follows:

set(CESIUM_NO_STB_IMAGE_IMPLEMENTATION ON)
add_subdirectory(cesium-native)

TheMostDiligent avatar May 09 '24 16:05 TheMostDiligent

Thanks for the contribution @TheMostDiligent !

Moving stb_image.h into a namespace does not work because all functions are always defined as extern "C".

This, in theory, should be able to work. I did a similar thing with STD_IMAGE_RESIZE_IMPLEMENTATION in this PR. Although looking back, maybe I should have done the image implementation as well.

The trick is to declare the STBIRDEF define as empty, so it doesn't prefix the implementation's functions with extern C. This lets you put it in another namespace.

Worth a try?

csciguy8 avatar May 09 '24 17:05 csciguy8

@csciguy8

Worth a try?

The reason it does not work is because in stb_image_resize.h, all functions are perfixed with STBIRDEF. So when you defined it to be empty, all functions become local to namespace. (BTW, note that including standard headers from a namespace is most certainly not allowed in c++.)

In stb_image.h, however, all functions are already in extern "C" block.

This works though:

#define STB_IMAGE_STATIC
#define STB_IMAGE_IMPLEMENTATION
#include <stb_image.h>
#include <turbojpeg.h>

Would this be a better solution?

TheMostDiligent avatar May 09 '24 17:05 TheMostDiligent

@csciguy8 I think that using STB_IMAGE_RESIZE_STATIC would be a more correct solution rather than using stb_image_resize.h in a namespace.

TheMostDiligent avatar May 09 '24 18:05 TheMostDiligent

@csciguy8 I think that using STB_IMAGE_RESIZE_STATIC would be a more correct solution rather than using stb_image_resize.h in a namespace.

It certainly is a cleaner approach. The reason why I went with putting it in a namespace instead is because cesium-native has two libraries that use it (GltfReader, GltfContent). So rather than having two implementations in cesium-native, I only declared one and have it shared. Is this a lot of code size difference? Probably not.

In stb_image.h, however, all functions are already in extern "C" block.

Ah, very inconvenient. In this case, it looks like the only solution to this would be to declare STB_IMAGE_STATIC. Can't think of any reason we wouldn't want to do this.

I'll work on merging this in. Thanks @TheMostDiligent !

csciguy8 avatar May 17 '24 16:05 csciguy8