Skip to content
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

Introduced a new Metal renderer implementation and a new example illustrating usage of Metal on macOS and iOS #1929

Closed
wants to merge 1 commit into from

Conversation

warrenm
Copy link
Contributor

@warrenm warrenm commented Jul 5, 2018

Partially addresses #1873

…strating usage of Metal on macOS and iOS (partially addresses ocornut#1873)
@ice1000
Copy link
Contributor

ice1000 commented Jul 8, 2018

I can't be more excited to see this, great!

But is there a cpp way to call these codes? Is it only possible to use this binding with ObjC? Could you please provide a cpp example?

@warrenm
Copy link
Contributor Author

warrenm commented Jul 8, 2018

I'm not sure I can see the motivation for making it C++-callable, since you have to provide references to Metal objects to use the renderer at all, and Metal is an Objective-C framework. Making the renderer API compatible with C++ would mean either removing type-safety or cluttering the interface for a vanishingly small number of use cases. Am I missing something?

@ocornut
Copy link
Owner

ocornut commented Jul 8, 2018

Thank you @warrenm for your work, much appreciated and super useful :)

I've tested and merged this now (doesn't show up as merged here because I went through a intermediary branch to apply some minor changes to the documentation).

I've also fixed the example_osx_opengl2/ code to pass in the view, and renamed it to example_apple_opengl2/ even though it is slightly misleading because it currently doesn't support iOS.

(I've also nuked the old example_apple/ out of orbit, it wasn't maintained and the combination of both new examples are enough of a replacement. The only extra thing example_apple/ did was to integrate uSynergy but this is not a strict requirement for the demo and made it messier. I wouldn't mind adding a Synergy demo back in a neater way if in the future we'll support Android+iOS demos more thoroughly.)

(In the future we should improve the DPI situation #1676, at least at a first step start tweaking our demo across all platforms, to rasterize the default font at double size on high-dpi settings)..

@Alzathar
Copy link
Contributor

Alzathar commented Aug 4, 2018

As requested, I paste a message related to the macOS example.

I was not able to use the Xcode project proposed for the 'example_apple_metal' binary. Xcode 9.2 sends the error message "Incompatible project version". Thanks to the README file I created a macOS Game project and copied/replaced files. This version works without any modification in the code. The key combination CTRL + Tab is correctly recognized.

The configuration of my computer is the following: macOS 10.12.6, Xcode 9.2 (9C40b), ImGui 1.63 WIP. I cannot upgrade to Xcode 9.3 or Xcode 9.4 because this is reserved for macOS 10.13 and later.

@buf1024
Copy link

buf1024 commented Oct 16, 2018

@Alzathar Just change the version code objectVersion = 50; -> objectVersion = 48; in file "example_apple_metal.xcodeproj/project.pbxproj", change the deployment target to 10.12, comment out the unnecessary included file #import <QuartzCore/CAMetalLayer.h> in file "imgui_impl_metal.mm", then works.

@ocornut
Copy link
Owner

ocornut commented Oct 16, 2018

Hello @buf1024 , @Alzathar, I would like to get this patched ideally.
@warrenm Any comment on buf1024 post above? Could we downgrade the project to earlier Xcode version?

Is there a purpose to the CAMetalLayer.h include that would break with newer version?
EDIT If so, could we add an #ifdef block to make it works both in older and newer versions?

Thank you!

@warrenm
Copy link
Contributor Author

warrenm commented Oct 16, 2018

Downgrading Xcode projects can be tricky, but I'll take a look! If/when I get it fixed on a branch, I assume you'd want a PR for upstream?

Update: Addressed by #2133
Update 2: I didn't make the CAMetalLayer change, but removing that line shouldn't have any consequences forward or backward; that class is never actually referenced in the code, so the import is truly redundant.

ocornut added a commit that referenced this pull request Oct 17, 2018
… Xcode 9.2 (last version supported on macOS 10.12) (#2133, #1929)
@ocornut
Copy link
Owner

ocornut commented Oct 17, 2018

Thank you @warrenm . I also also applied this change to the opengl example.

@Alzathar @buf1024 It is fixed for you now? Should "change the deployment target to 10.12" be something done in the project file, is "comment out the unnecessary included file" required on your end?

If the class is not referenced we could remove the include as well.
(If there are further changes Ideally @buf1024 could submit a PR)

Thanks all!

@Alzathar
Copy link
Contributor

@ocornut Unfortunately the commit 745f010 is not enough. I had to do the supplementary things recommended by @buf1024:

It works like a charm after under macOS.

screen shot 2018-10-17 at 8 56 14 am

On the other hand, I got a crash for the iOS example as reported in the following screenshot. I used the Simulator version 10.0 with the iPhone 8+ and iOS 11.2.

screen shot 2018-10-17 at 8 53 02 am

@ocornut
Copy link
Owner

ocornut commented Oct 17, 2018

@Alzathar

It works like a charm after under macOS.

Could you push a PR for both projects, and maybe users of newer XCode can confirm if it still works for them?

On the other hand, I got a crash for the iOS example as reported in the following screenshot. I used the Simulator version 10.0 with the iPhone 8+ and iOS 11.2.

A quick Google search suggests that the Simulator doesn't support Metal.

@Alzathar
Copy link
Contributor

Could you push a PR for both projects, and maybe users of newer XCode can confirm if it still works for them?

Done in the PR #2134.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants