CesiumGS / cesium-native

Apache License 2.0
391 stars 200 forks source link

Gltf Reader: use STB_IMAGE_STATIC to avoid conflicts with other libs #877

Closed TheMostDiligent closed 1 month ago

TheMostDiligent commented 2 months ago

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)
csciguy8 commented 2 months ago

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?

TheMostDiligent commented 2 months ago

@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 commented 2 months ago

@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.

csciguy8 commented 1 month ago

@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 !