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

[CR] iOS build #61550

Closed
wants to merge 23 commits into from
Closed

[CR] iOS build #61550

wants to merge 23 commits into from

Conversation

smileynet
Copy link
Contributor

Summary

Build "Support for iOS builds using Xcode"

Purpose of change

To create a reasonably automated way for people to build any branches, including experimental, to play on iOS devices. Additionally this lays the groundwork for future improvements of the native iOS experience (and possibly integrate some existing android features). So far I have been able to keep all additions within the scope existing libraries and code languages.

Describe the solution

The script ios-xcode.sh uses Homebrew (which includes Xcode CLI Tools) and Xcodegen to create a new Xcode project that allows users to build an image for use on their own devices using an apple developer account. The biggest hack currently is the manually included versions of static SDL libraries, as adding linked source directories would have added bloat to the repo I didn't feel comfortable introducing on my first PR here. Also, the alternative of writing up directions on how to do it manually would have dramatically increased the complexity of successfully building, and would have been a lot to navigate for people who are not familiar with Xcode.

Describe alternatives you've considered

I would eventually like to rework the build system to fit within the existing make/cmake patterns, but at this point it's low on my list relative to working on continuing to improve the in-game iOS experience.

Testing

Testing has been done on an iPad Pro 11 (3rd gen). My particular use case is with a hardware keyboard, so while a soft keyboard works, at the moment is it not a great user experience. I did not include a build chain for simulators, but may add that at a later date as well.

Additional context

Given that it's my first time contributing here, I'm new to the codebase, and that this change hits some fundamental sections, I am happy to take whatever extra steps/ provide additional explanations are needed to help make this fit cleanly and non-disruptively in to the existing codebase.

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. [C++] Changes (can be) made in C++. Previously named `Code` [Markdown] Markdown issues and PRs Code: Build Issues regarding different builds and build environments json-styled JSON lint passed, label assigned by github actions labels Oct 8, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 8, 2022
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Oct 9, 2022
@Theawesomeboophis
Copy link
Contributor

Yay, now my friends can experience this glorious game. They don't have computers and they also don't have androids. Thank you for this.

@smileynet
Copy link
Contributor Author

smileynet commented Oct 9, 2022

This seems odd given the use of #if defined(TARGET_OS_IPHONE) already existed elsewhere in the code (originally in sdltiles) and was presumably behaving as intended. There is __IPHONEOS__ that is defined as part of SDL that could be used instead, but I'm surprised that this tag is failing/ allowing the section to be improperly called in the first place.

@smileynet
Copy link
Contributor Author

I was able to do an initial build for os x by switching the tags to __IPHONEOS__. In looking in to this more the correct usage of the previous tag would be if (defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE == 1) per the way it's used in catch.hpp. Given that the __IPHONEOS__ tag is more inline with the style of other OS defines, and less verbose, I will look at swapping out the rest of the tags this week. It's also worth noting that the existing usage of the TARGET_OS_IPHONE tag in sdltiles may be causing unexpected behavior on OS X as is.

@anothersimulacrum
Copy link
Member

What's the source of the launch screen image? That must be properly attributed. Also, it's a pretty big image, is it necessary?

Compressed loading screen image.
@smileynet
Copy link
Contributor Author

Applied compression to png, it is now ~25% of the original size. It would be possible for a blank loading screen, but I would argue that it (or something other than a black screen) is a better user experience when loading the app as there is enough of a load time that the screen is briefly visible (and it appears briefly when switching back to the app as well). Source of the image is that it was generated via the DALL E project by myself, let me know if I need to document the attribution or terms somewhere:

Use of Images. Subject to your compliance with these terms and our Content Policy, you may use Generations for any legal purpose, including for commercial use. This means you may sell your rights to the Generations you create, incorporate them into works such as books, websites, and presentations, and otherwise commercialize them.

https://labs.openai.com/policies/terms

@github-actions github-actions bot added the Code: Tests Measurement, self-control, statistics, balancing. label Oct 12, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 16, 2022
doc/COMPILING/COMPILING-XCODE.md Outdated Show resolved Hide resolved
doc/COMPILING/COMPILING-XCODE.md Outdated Show resolved Hide resolved
doc/COMPILING/COMPILING-XCODE.md Outdated Show resolved Hide resolved
src/sdltiles.cpp Outdated Show resolved Hide resolved
@kevingranade
Copy link
Member

I'm very sorry this feedback is so late, I thought I had said so already but I can't find it, so mea culpa.
We must have a different way of handling the dependencies for this. I'm neither auditing an anonymous binary bundle of files nor merging them without an audit.
Also I'm not too excited about mainlining AI generated art, the ethics around it are fraught at best and I don't want to promote it in dda.

@NetSysFire NetSysFire added the OS: macOS Issues related to macOS / OSX operating system label Nov 9, 2022
@NetSysFire
Copy link
Member

The issue about the art should be able to be resolved easily enough. I can not find any cool art on opengameart sadly but if all else fails, I would ask r/cataclysmdda and ask if any of the artists which have submitted some art in the past, or would like to contribute, if they would apply an appropriate license to their art.

Another idea I'd have would be to display some of the ascii art we already have in place of this loading screen background. And/or pre-fetch the tips of the day and display them, too.

@Zireael07
Copy link
Contributor

@NetSysFire +1 to just using our ASCII art

@nornagon
Copy link
Contributor

nornagon commented Nov 9, 2022

FYI there are a couple of other existing iOS ports, e.g. https://github.com/apollovy/Cataclysm-DDA-iOS

@smileynet
Copy link
Contributor Author

FYI there are a couple of other existing iOS ports, e.g. https://github.com/apollovy/Cataclysm-DDA-iOS

AFAIK he pockets all the money he solicits from that version without passing it back to this organization, and even worse, he doesn't build for experimental. If he at least did the latter I would still be playing on my iPad blissfully ignorant of the toils of open source contribution instead of flaying myself trying to get this merged. ;)

Updates per kevingranade's requirements coming shortly...

@esotericist
Copy link
Contributor

i believe the intent from nornagon was to point out extant work that could potentially be reusable, not to suggest there is no need for mainline ios support.

@smileynet
Copy link
Contributor Author

smileynet commented Nov 12, 2022

i believe the intent from nornagon was to point out extant work that could potentially be reusable, not to suggest there is no need for mainline ios support.

Understood. The short version is that all existing attempts at an iOS build used some kind of wrapper for the game rather than using the functionality of the SDL libs themselves. My approach allows the game to run on iOS without doing unspeakable things to the code that would cause all previous contributors to wake up from a dead sleep in a cold sweat (and given all the guff I've been given for some pretty straight forward stuff, would never have been mainlined). This approach makes it easier to pair future functionality for both Android and iOS, and is generally more maintainable.

So... yup, I am aware. And I don't mean to come across with undue snark, but my repeated pleas for help while figuring this out on my own went largely unanswered. So showing up months after I did all of this to point out that other people have tried is... neat. While the google warriors are deployed, what would be helpful is if anyone else wants to figure out a good way to build SDL libs for iOS in something of an automated fashion. Ideally via cmake. :)

@esotericist
Copy link
Contributor

your frustration is very understandable, but i feel i should point out that to a degree, the lack of help you've gotten is directly paired with why this work hasn't been done yet in mainline: lack of people who deal with ios stuff.

i do want to say i appreciate the effort, though, and having an ios build in the mainline is definitely something we've wanted, there's just been a lack of the junction of time and expertise in the same person to make all of this happen. that you've been muddling through is to your credit.

@nornagon
Copy link
Contributor

@smileynet you might be interested in my adaptation for experimental over at https://github.com/nornagon/Cataclysm-DDA-experimental-iOS. Unfortunately I'm having trouble getting on the App Store because they claim it's just a copy of the existing app 🙃

@I-am-Erk
Copy link
Member

I don't want to dive into this can of worms, because I think this is a great addition overall and I can understand the frustration... but I think, sadly, this needs to wait until after stable. There's too much chance that if we do introduce it, there will be key problems with the iOS build that need resolution to release.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not bump or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@github-actions github-actions bot added the stale Closed for lack of activity, but still valid. label Apr 7, 2023
@github-actions github-actions bot closed this May 7, 2023
@Zireael07
Copy link
Contributor

Stable has come and gone, worth revisiting

@smileynet
Copy link
Contributor Author

I've moved on to other projects, but am supportive of seeing it go in. Let me know if I can answer any questions.

If it does get merged I'll likely come back and tinker more once it's there. This being done was a prerequisite for me investing the time to overhaul the input system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments Code: Tests Measurement, self-control, statistics, balancing. <Documentation> Design documents, internal info, guides and help. json-styled JSON lint passed, label assigned by github actions [Markdown] Markdown issues and PRs OS: macOS Issues related to macOS / OSX operating system stale Closed for lack of activity, but still valid.
Projects
Status: Abandoned PRs with no one to pick them up
Development

Successfully merging this pull request may close these issues.