bkaradzic / bgfx

Cross-platform, graphics API agnostic, "Bring Your Own Engine/Framework" style rendering library.
https://bkaradzic.github.io/bgfx/overview.html
BSD 2-Clause "Simplified" License
14.59k stars 1.92k forks source link

Support older macOS/iOS version. #3283

Closed mcourteaux closed 2 months ago

mcourteaux commented 2 months ago

I tried to follow the ideas from our conversation. I made two structs macOSVersion and iOSVersion to prevent anyone from accidentally mixing the two version requirements in the helper function.

I manually went through all example apps on my MacBook (hence macOS), and can confirm they all work. I do get warnings, which I think is inevitable. Silencing the warnings can be done by one of the following:

The last option is sort of the only valid one, I fear. The api changed from macOS 12 to 13. However, it seems that all the structs are ABI compatible, but that's sort of not guaranteed, I'd assume. The compiler is now explicitly complaining about using incompatible pointers, such as:

../../../../src/renderer_mtl.mm:2481:28: warning: incompatible pointer types passing 'NSArray<MTLArgument *> *' to parameter of type 'NSArray<id<MTLBinding>> *' [-Wincompatible-pointer-types]
                                        processArguments(pso, reflection.arguments, NULL);

which is code that actually does run at runtime on my machine, because they really are MTLArgument objects, and still they are passed as id<MTLBinding> objects. Then accessing the isActive function on it works for some magical reason...

This Obj-C function-calling-by-the-string-function-name crap smells a lot like Python, and I'm very unfamiliar with it. However, as mentioned: all examples do work on macOS. To be clear: iOS is untested.

bkaradzic commented 2 months ago

Now this version checking is getting ridiculous...

By looking at code like this:

if (atLeastOSVersion(macOSVersion(13, 0, 0), iOSVersion(16, 0, 0))) {

I have no clue what's going tested here, just see random numbers that don't mean anything to me. I know they are versions of OS but I would have to look somewhere what macOS 13 is, and what iOS 16 does...

Also it ignores tvOS, and visionOS. But adding those would make whole thing impossible to follow.

What would be preferable is that during init there is check for specific feature. There are a few good examples:

        bool m_hasPixelFormatDepth32Float_Stencil8;
        bool m_hasStoreActionStoreAndMultisampleResolve;

And then in code where we check for existence of feature, we don't use random version numbers that don't mean anything to observer, rather to use bool that represents the feature we're testing.

Also I could just search m_hasStoreActionStoreAndMultisampleResolve and see where code is using it. Instead searching for atLeastOSVersion(macOSVersion(10, 12, 0), iOSVersion(10, 0, 0)) <- no clue what that is...

mcourteaux commented 2 months ago

Fair enough, I can refactor it to be that way. Will push changes somewhere later today or tomorrow I think.

mcourteaux commented 2 months ago

Replaced by #3284