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

Examples: Consolidated example_apple_metal to reduce class and file count #3543

Closed
wants to merge 1 commit into from

Conversation

warrenm
Copy link
Contributor

@warrenm warrenm commented Oct 18, 2020

This PR retains the behavior of the Apple iOS/macOS Metal example as much as possible, while simplifying the code and structure of the project and reducing its size on disk by ~10kB.

  • The Renderer class has been removed in favor of doing all rendering work in the view controller
  • Storyboards have been stripped down to their bare minimum in favor of creating windows, etc. directly in code
  • All classes have been consolidated into main.mm instead of spread over 7 different files.

This PR expressly does not address context cleanup or other known issues, but it does set the stage for making such changes more easily in the future.

@rokups
Copy link
Contributor

rokups commented Oct 19, 2020

Hey @warrenm thank you for a great PR! This sample is finally grokkable by someone like me who has litterally no development experience on these platforms 👍 PR works great, at least build for MacOS. Could not test iOS build as simulator does not support metal on MacOS version i run.

I absolutely love how most of the code is shared between iOS and MacOS variants!

Couple observations:

  • To further simplify project structure we could move storyboard/plist out of subfolders and prefix them with a platform tag.
  • We may want to consider following Dear ImGui code style. Although obj-c is foreign enough so this suggestion may be debatable.

@ocornut
Copy link
Owner

ocornut commented Oct 19, 2020

Thank you Warren this looks great!

Generally the intent would be that example_apple_metal and example_apple_opengl2 matches as much as possible and it looks like we are now very close to that.

Would you like to try having a pass at comparing the two files side by side and making them closer matches (apart from the rendering stuff of course)? Of course, changes can also be applied to example_apple_opengl2 to get them closer to each others.

One of the reason for the matching is any fix that goes in any examples/ can easily be applied to all other examples (including by myself without using a Mac, as cosmetic/comments changes are tested by CI).

ocornut pushed a commit that referenced this pull request Oct 23, 2020
@ocornut
Copy link
Owner

ocornut commented Oct 23, 2020

Merged this as a first step, thank you!

I have another (unpushed commits) trying to fix some shallow coding style thing, but making the two examples match more closely is out of my league for not being a OSX user. Also noticed that compared to apple_opengl2 this Metal example doesn't forward keyUp, keyDown, flagsChanged, rightMouseMoved, otherMouseMoved.

The last two ones are actually currently unused by OSX backends but afaik the first three ones are required for keyboard to work. I am right?

Will also look into #3554.

@ocornut
Copy link
Owner

ocornut commented Nov 11, 2020

I'll close this PR as merged.
It would still be good down the line to try to make both Apple examples match more closely, as there are variety of subtleties down differently in one or the other.
Presumably once we merge multi-viewports in that backends (#2778) we'll be included to look at those examples closely.

Thanks again!

@ocornut ocornut closed this Nov 11, 2020
ocornut pushed a commit that referenced this pull request Jun 28, 2021
@ocornut
Copy link
Owner

ocornut commented Jun 28, 2021

88f4c13 tries to make both Metal and OpenGL2 example more similar but there are still a bunch of larger difference that we aren't familiar with. Would be nice is someone OSX/iOS friendly could decide how/if we should bridge the differences.

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.

3 participants