-
Notifications
You must be signed in to change notification settings - Fork 418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add visionOS support #323
Conversation
include/bx/platform.h
Outdated
@@ -6,6 +6,10 @@ | |||
#ifndef BX_PLATFORM_H_HEADER_GUARD | |||
#define BX_PLATFORM_H_HEADER_GUARD | |||
|
|||
#ifdef __APPLE__ | |||
#include <TargetConditionals.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No platform includes... You have to figure out how to detect it without it. If you didn't notice there are no other platform includes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced platform include with #elif __has_builtin(__is_target_os) && __is_target_os(xros)
as there is no __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__
equivalent for visionOS. Do you think that's okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have these for all platforms except visionOS?
__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__
__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__
__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__
Do you think that's okay?
This is fine, but you have to guard first __has_builtin
with something, since it's not available everywhere:
#elif __has_builtin(__is_target_os) && __is_target_os(xros)
For example (not sure if this will work, just a guess):
#elif defined(__has_builtin) && __has_builtin(__is_target_os) && __is_target_os(xros)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, unfortunately this is missing for visionOS..
Apple left some TODO
comments in AvailabilityInternal.h
(header where the rest of the defines are mentioned) but there is no __ENVIRONMENT_XR_OS_VERSION_MIN_REQUIRED__
or __ENVIRONMENT_VISIONOS_OS_VERSION_MIN_REQUIRED__
.
I've checked and #elif defined(__has_builtin) && __has_builtin(__is_target_os) && __is_target_os(xros)
works great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked and #elif defined(__has_builtin) && __has_builtin(__is_target_os) && __is_target_os(xros) works great.
Great! LGTM.
Are you still adding changes or this is it? I'm ready to merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the point of bx repo I think it's all. If anything pops out I can create a follow up PR.
So yeah, I think we can merge it 😄
9bdc6e1
to
45c24ed
Compare
This PR is the first step of adding visionOS support to
bgfx
followup PR tobgfx
will be opened soon.