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

IOS: Fixes the iOS port #630

Merged
merged 110 commits into from Jan 7, 2016

Conversation

Projects
None yet
4 participants
@bSr43
Copy link

bSr43 commented Dec 2, 2015

This pull request brings the compatibility with iOS 7+, the new iPhone 6/6S/6+/6S+ resolution, iTunes file sharing, and fixes many issues with the current iOS version.

It also updates the create_project tool, so that it creates valid Xcode projects for both OS X, and iOS.

Compilation instructions are given in the backends/platform/iphone/README.md file.

@bSr43 bSr43 changed the title Fixes the iOS port IOS: Fixes the iOS port Dec 2, 2015

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Dec 2, 2015

At a first glance, I have to say this is amazing work! Very well done :)

Plenty of changes to the IOS target, and updates to the create_project too, as well. Kudos!

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Dec 3, 2015

Thank you :)

@@ -52,6 +53,11 @@ enum UIViewSwipeDirection {
kUIViewSwipeRight = 8
};

enum UIViewTapDescription {
kUIViewTapSingle = 1,
kUIViewTapDouble = 2,

This comment has been minimized.

@lordhoto

lordhoto Dec 4, 2015

Member

This has an extra "," at the end which is only legal in C++11, which we do not use.

eaglLayer.drawableProperties = [NSDictionary dictionaryWithObjectsAndKeys:
[NSNumber numberWithBool:FALSE], kEAGLDrawablePropertyRetainedBacking, kEAGLColorFormatRGB565, kEAGLDrawablePropertyColorFormat, nil];
eaglLayer.drawableProperties = @{
kEAGLDrawablePropertyRetainedBacking: @NO,

This comment has been minimized.

@lordhoto

lordhoto Dec 4, 2015

Member

I'm getting the following compiler error on this line on buildbot:

backends/platform/iphone/iphone_video.mm:109:74: error: unexpected type name 'BOOL': expected expression
                                         kEAGLDrawablePropertyRetainedBacking: @NO,
                                                                                ^
usr/include/objc/objc.h:50:26: note: expanded from macro 'NO'
#define NO              (BOOL)0
                         ^

This comment has been minimized.

@bSr43

bSr43 Dec 4, 2015

This is the « new » Objective-C 2.0 syntax. Which compiler are you using? Anyway, I’ll switch back to the old notation, there is no need to use the new notation, expect for the ease of reading :)

@@ -158,31 +166,74 @@ - (void)createContext {
}
}

- (void)setupGestureRecognizers {
UISwipeGestureRecognizer *swipeRight = [[UISwipeGestureRecognizer alloc] initWithTarget:self action:@selector(twoFingersSwipeRight:)];

This comment has been minimized.

@lordhoto

lordhoto Dec 4, 2015

Member

Looks like these swipe gesture recognizers have not been available in older iOS versions (we use SDK version 3.1.2 to support all generations of devices):

backends/platform/iphone/iphone_video.mm:170:2: error: unknown type name 'UISwipeGestureRecognizer'
        UISwipeGestureRecognizer *swipeRight = [[UISwipeGestureRecognizer alloc] initWithTarget:self action:@selector(twoFingersSwipeRight:)];     
        ^

This comment has been minimized.

@bSr43

bSr43 Dec 4, 2015

This is something I feared, indeed… The major issue is that the handling of screen orientation has changed with iOS 7, and writing something which works on both cases is very complex.

}

- (CGFloat)optimalScale {
CGFloat screenScale = [[UIScreen mainScreen] scale];

This comment has been minimized.

@lordhoto

lordhoto Dec 4, 2015

Member
backends/platform/iphone/iphone_video.mm:198:47: warning: 'UIScreen' may not respond to 'scale'
        CGFloat screenScale = [[UIScreen mainScreen] scale];
                               ~~~~~~~~~~~~~~~~~~~~~ ^

I am pretty sure our old code had special code to detect this case. I think it's best to add it again.

CGFloat screenScale = [[UIScreen mainScreen] scale];
if (screenScale < 2) return screenScale;

if (UI_USER_INTERFACE_IDIOM() == UIUserInterfaceIdiomPad) {

This comment has been minimized.

@lordhoto

lordhoto Dec 4, 2015

Member

I guess there's more SDK version fun here:

platform/iphone/iphone_video.mm:201:6: error: use of undeclared identifier 'UI_USER_INTERFACE_IDIOM'
        if (UI_USER_INTERFACE_IDIOM() == UIUserInterfaceIdiomPad) {
            ^
platform/iphone/iphone_video.mm:201:35: error: use of undeclared identifier 'UIUserInterfaceIdiomPad'
        if (UI_USER_INTERFACE_IDIOM() == UIUserInterfaceIdiomPad) {
                                         ^

This comment has been minimized.

@bSr43

bSr43 Dec 4, 2015

Does your toolchain can be downloaded somewhere?
At this time, this port mostly dropped support for very old iOS versions. But if I can try to compile with the same toolchain than yours, I could try to merge the old support, with the new one.

CGSize screenSize;
UIScreen *mainScreen = [UIScreen mainScreen];
if ([mainScreen respondsToSelector:@selector(nativeBounds)]) {
screenSize = [mainScreen nativeBounds].size;

This comment has been minimized.

@lordhoto

lordhoto Dec 4, 2015

Member
backends/platform/iphone/iphone_video.mm:208:28: warning: instance method '-nativeBounds' not found (return type defaults to 'id') [-Wobjc-method-access]
                screenSize = [mainScreen nativeBounds].size;                                                                
                                         ^~~~~~~~~~~~                                                                       
backends/platform/iphone/iphone_video.mm:208:42: error: property 'size' not found on object of type 'id'         
                screenSize = [mainScreen nativeBounds].size;
                                                       ^

This time you check whether the selector is actually available. But it doesn't know about the proper type here. Not sure how to resolve this, this is a bit different from the code we used for the scale selector handling...

This comment has been minimized.

@bSr43

bSr43 Dec 4, 2015

It comes from the SDK you are using. In fact, the latest SDK defines this selector, so the compiler knows how to deal with it. That being said, if you want to target old devices, you don’t need to use an old SDK, you just need to select the proper deployment target.

Another thing, while I’m thinking about all this, the support for the new iPhone 6 resolution requires the use of the new image asset catalog, which is something that old devices cannot deal with. It means that I think we should have two backends, for the old devices, and for the new ones.

case 1: {
UITouch *touch = [touches anyObject];
CGPoint point = [touch locationInView:self];
NSSet<UITouch *> *allTouches = [event allTouches];

This comment has been minimized.

@lordhoto

lordhoto Dec 4, 2015

Member

This one looks really odd to me, but I don't have the time to investigate this further right now:

backends/platform/iphone/iphone_video.mm:717:16: error: expected '>'
        NSSet<UITouch *> *allTouches = [event allTouches];
                      ^
backends/platform/iphone/iphone_video.mm:717:17: error: expected unqualified-id
        NSSet<UITouch *> *allTouches = [event allTouches];
                       ^

This comment has been minimized.

@bSr43

bSr43 Dec 4, 2015

the « <UITouch *> » can be removed, this is only a new syntax element, introduced last year in clang, in order to provide information about the objects in the collection.

@lordhoto

This comment has been minimized.

Copy link
Member

lordhoto commented Dec 4, 2015

I sadly can't test the changes because this breaks compilation for me. It looks like it uses a lot of newer SDK features, which seem to indicate the port is breaking old devices. This is something we want to avoid.

What is missing before I can give any further comments is:

  • Make code compile with old SDK versions again (preferably 3.1.2, so we can still support all device generations out there).
  • Make code compile with our clang 3.3 based toolchain. It seems there are some new syntax features used, which are not supported by this compiler.
  • Test code on an actual old iOS device. I can do this, if I get the changes to compile.

If you need any instructions on how to get the toolchain to build or the SDK we use, please drop me an email. It's simply my nickname followed by the crazy a symbol followed by scummvm.org.

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Dec 4, 2015

I sent you an email :)

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Dec 15, 2015

Sadly, this now has merge conflicts, probably because of the recent changes to our devtools. Could you please fix them?

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Dec 16, 2015

It is merged…

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Jan 5, 2016

This does look OK to be merged now, since it does work with older IOS SDKs. Once the conflicts are resolved, is it in a good state to merge?

Note that there has been some duplicated effort in pull request #645 (though not as much as what has been done in this pull request). It would be nice if this was merged first, and the changes in the other pull request were added afterwards.

So, for now, please resolve the conflicts when you can :) Thanks!

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Jan 5, 2016

it's done…

@lordhoto

This comment has been minimized.

Copy link
Member

lordhoto commented Jan 5, 2016

There are still a few issues we need to tackle before this is good to go.

The iOS7 code uses our software scalers. While this is nice in theory, it is bad in practice. This limits the port to 16bit color precision, i.e. preventing it from coping with engines like Wintermute, and Sword25.
Since iOS7+ devices support OpenGL ES 2, we can think of using GLSL based filter implementations in the future (I am working on this in general for ScummVM right now, so there is no need to handle this in this pull request or iOS specific).
I plan to get rid of the iPhone specific OpenGL implementation and replace it with our generic OpenGLGraphicsManager. There are similar plans for our Android port. This will reduce the amount of code we need to maintain, while giving all our ports a similar amount of features. Introducing this scaler feature, and then removing it in the near future is confusing to the users (if we want to merge this before the upcoming 1.8.0 release), thus it is a showstopper for this PR.

Right now, the iOS7 code introduces a lot of code duplication by copying over all iPhone backend files and modifying them. This has all the well known down-sides: chances that we need to update the same code twice, etc. It is much preferable to have it all based on one common code, i.e. we could have a OSystem_IOSBase, and two subclasses for pre-iOS7 and iOS7+, residing in backends/platform/iphone and the build setup selects which one to use.

There are some odd changes to non-iOS-port specific files. Some seem supsicious. For example, dists/macosx/Info.plist was changed, but not the dists/macosx/Info.plist.in file. Addtionally, the file seems to refer to variables, it is unclear to me how our configure+make based build system assure that these are replaced properly. I will also need to look into changes to common/ code, I'll get back to that in the next few days.

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Jan 5, 2016

Regarding the scalers, it could be disabled, indeed (even if I love the current feature on my iPad, but I understand the problem :-))

For the code duplication, I completely agree with you, but maybe you should see this PR only as a first working version, which will be improved later. It required a lot of efforts to make it work smoothly on new iOS devices, but I haven't seen any real interest in merging my PR during the last weeks, although I tried to implement and modify all that have been discussed by email, resulting in lots of new modifications (including this duplication, BTW). I even wrote this script to compile a toolchain for your build system, so that you can easily integrate :-/

About the template files, they was not used by the configure script, even before I started to make changes. I think that those ".in" files could be safely removed.

@lordhoto

This comment has been minimized.

Copy link
Member

lordhoto commented Jan 5, 2016

For the code duplication, I completely agree with you, but maybe you should see this PR only as a first working version, which will be improved later.

Whoops, sorry looks like I didn't adjust all of my comment before posting... yes, we can refactor this later. This is why I didn't talk about it being a showstopper.

About the template files, they was not used by the configure script, even before I started to make changes. I think that those ".in" files could be safely removed.

The resuting files are used by our configure+make based build system. The targets bundle (for Mac OS X) and iphonebundle for iPhone use them to create the bundle files. This is used for release builds and thus should be working.

The .in version is required to automatically update our ScummVM version information (i.e. 1.8.0git etc.) whenever it changes. Btw. this reminds me: You need to add the ios7 file the subs_files "array" in devtools/update_version.pl.

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Jan 5, 2016

OK, I'll take a look tomorrow (it's midnight here, in France :-)). It'll be easy to update the ".in" files, and I think that I'll surround all the scalers code with a disabled macro. I'll also try to evaluate the amount of work for refactoring of the duplicated code, but if that's not a showstopper for you, maybe I'll create a new PR later :-)

char *soundfont_fullpath[PATH_MAX];
const char *document_path = iOS7_getDocumentsDir();
strcpy((char *) soundfont_fullpath, document_path);
strcat((char *) soundfont_fullpath, soundfont);

This comment has been minimized.

@lordhoto

lordhoto Jan 6, 2016

Member

I suggest to use a Common::String as buffer here. Just for future reference: strcpy/strcat are pretty unsafe, we usually use Common::strlcpy and Common::strlcat instead, which are implementing OpenBSD's strlcpy and strlcat.

This comment has been minimized.

@bSr43

bSr43 Jan 6, 2016

You're right. I changed it.

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Jan 6, 2016

The modifications are done!

This explains why I thought that the .in files were not used anymore: I haven't seen the update-version.pl script!

@@ -179,7 +184,16 @@ int MidiDriver_FluidSynth::open() {

const char *soundfont = ConfMan.get("soundfont").c_str();

#ifdef IPHONE_OFFICIAL

This comment has been minimized.

@lordhoto

lordhoto Jan 6, 2016

Member

This whole block needs a FIXME/HACK comment. It indicates that the chroot FS implementation breaks functionality in unexpected ways. In the future we need to get rid of this special code for iOS.

@@ -179,7 +184,16 @@ int MidiDriver_FluidSynth::open() {

const char *soundfont = ConfMan.get("soundfont").c_str();

#ifdef IPHONE_OFFICIAL
char *soundfont_fullpath[PATH_MAX];
const char *document_path = iOS7_getDocumentsDir();

This comment has been minimized.

@lordhoto

lordhoto Jan 6, 2016

Member

Looks like iPHONE_OFFICIAL is a valid define for pre-iOS7 backend too. Thus, this is probably breaking build for that.


#include "backends/fs/fs-factory.h"

class ChRootFilesystemFactory : public FilesystemFactory {

This comment has been minimized.

@lordhoto

lordhoto Jan 6, 2016

Member

Please add a FIXME comment which indicates that using this factory in your backend breaks features silently. Instances are, for example, the FluidSynth code, and the POSIX plugin code.

*
*/

#ifndef CHROOT_FS_H

This comment has been minimized.

@lordhoto

lordhoto Jan 6, 2016

Member

BACKENDS_FS_CHROOT_CHROOT_FS_H, i.e. we use the file path (most of the time).

*
*/

#ifndef CHROOT_FS_FACTORY_H

This comment has been minimized.

@lordhoto

lordhoto Jan 6, 2016

Member

BACKENDS_FS_CHROOT_CHROOT_FS_FACTORY_H, i.e. we use the file path (most of the time).

/**
* Add a path component
*/
String stringByAppendingPathComponent(String component, char sep = '/') const;

This comment has been minimized.

@lordhoto

lordhoto Jan 6, 2016

Member

This is definitiely nothing which should be in Common::String itself, it is a helper for filenames not a generic string helper. Since you only seem to use it in one place, I suggest to move it there (i.e. the chroot FS code).

But I think you don't even need this. As long as you can assure that _root is not empty, you can simply compose paths with: Common::normalizePath(_root + '/' + component, '/'); in your case.

@@ -32,6 +32,13 @@ MODULE_OBJS := \
widgets/scrollbar.o \
widgets/tab.o

# Even if it seems redundant, please keep
# these directives in that order!
# This is needed by the "create_project" tool, for the OS X / iOS Xcode project

This comment has been minimized.

@lordhoto

lordhoto Jan 6, 2016

Member

I don't understand how this works. create_project seems to set both IPHONE and MACOSX when going over our files. Now, the comment in here suggests that create_project selects based on the suffix. But, if IPHONE is defined only browser.o is ever picked up?

At any rate, this deserves a HACK (and probably FIXME) indication.

@bSr43 bSr43 force-pushed the bSr43:ios-fix branch from 247fff2 to a4f9b9e Jan 6, 2016

@mgerhardy

This comment has been minimized.

Copy link

mgerhardy commented on backends/platform/ios7/README.md in dcfe197 Jan 6, 2016

shouldn't the updated version be used?
https://github.com/tpoechtrager/cctools-port

This comment has been minimized.

Copy link
Owner

bSr43 replied Jan 6, 2016

@lordhoto

This comment has been minimized.

Copy link
Member

lordhoto commented Jan 6, 2016

Can we get the rename of IPHONE_OFFICIAL in? I'll take a last look at it tomorrow and merge it if nobody else comments till then (should be around noon CET).

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Jan 6, 2016

Yes, I'll do that in a few hours, hopefully before noon!

@bSr43 bSr43 force-pushed the bSr43:ios-fix branch from d841856 to b5ef986 Jan 7, 2016

@bSr43

This comment has been minimized.

Copy link

bSr43 commented Jan 7, 2016

@lordhoto, it should be OK now.

@lordhoto

This comment has been minimized.

Copy link
Member

lordhoto commented Jan 7, 2016

Great! Thanks!

lordhoto added a commit that referenced this pull request Jan 7, 2016

Merge pull request #630 from bSr43/ios-fix
IOS: Fixes the iOS port

@lordhoto lordhoto merged commit bd1039b into scummvm:master Jan 7, 2016

@bSr43 bSr43 deleted the bSr43:ios-fix branch Jan 7, 2016

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Jan 7, 2016

Just wanted to say: excellent work by both @bSr43 and @lordhoto. Thanks guys! :)

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