Apple libcurl replacement#169
Conversation
| "Include/Babylon/NetworkUtils.h" | ||
| "Include/Babylon/TicketedCollection.h" | ||
| "Source/NetworkUtils.cpp") | ||
| if(APPLE) |
There was a problem hiding this comment.
nit: I'm a little iffy about the platform-specific guards we have in here. I know they're inevitable in certain cases, but we really ended up with a ton of them before the plugin refactor (many of which we still have), and for me it felt pretty messy. Can we look for alternatives for how to minimize these, at least within scenarios where we have duplicated code, as below?
There was a problem hiding this comment.
best alternative is to have specific plugin/project per platform IMHO. instead of guards we would have more cmakelists.
There was a problem hiding this comment.
I'd be fine with that. Is this just a temporary workaround, then? If so, can we associate an issue with it?
| assert(resources.uniform_buffers.size() == 1); | ||
| const spirv_cross::Resource& uniformBuffer = resources.uniform_buffers[0]; | ||
| #if (BGFX_CONFIG_RENDERER_METAL) | ||
| #if __APPLE__ |
There was a problem hiding this comment.
Why this change? The comment below indicates that the guarded behavior is specific to Metal, not Apple; is that incorrect?
There was a problem hiding this comment.
BGFX_CONFIG_RENDERER_METAL not defined. APPLE is the only define that I've found to make the guard.
There was a problem hiding this comment.
nit: We should probably associate this with the rendering engine somewhere down the line, since this could cause problems if somebody doesn't use the default runtime for the platform (i.e., GL or MoltenVK or whatever).
| std::unique_ptr<InputManager::InputBuffer> inputBuffer{}; | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
nit: Why the lines? Is this an Objective-C style thing?
| "Include/Babylon/NetworkUtils.h" | ||
| "Include/Babylon/TicketedCollection.h" | ||
| "Source/NetworkUtils.cpp") | ||
| if(APPLE) |
There was a problem hiding this comment.
I'd be fine with that. Is this just a temporary workaround, then? If so, can we associate an issue with it?
| assert(resources.uniform_buffers.size() == 1); | ||
| const spirv_cross::Resource& uniformBuffer = resources.uniform_buffers[0]; | ||
| #if (BGFX_CONFIG_RENDERER_METAL) | ||
| #if __APPLE__ |
There was a problem hiding this comment.
nit: We should probably associate this with the rendering engine somewhere down the line, since this could cause problems if somebody doesn't use the default runtime for the platform (i.e., GL or MoltenVK or whatever).
syntheticmagus
left a comment
syntheticmagus
left a comment
There was a problem hiding this comment.
Pending the renaming stuff, and preferably with issues to track some of the temporary workaround bits we want to clean up later, looks good to me!
3 participants
ios build down from 22min to 3
macos down from 10min to 2