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 #748

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@BlueCobold
Contributor

BlueCobold commented Dec 9, 2014

A little more support for iOS. A minor fix for ContextSettings to handle stencil bits correctly and various changes to support orientation changes of the device. Tested on iOS 7.x and 8.x on emulators and real devices. Also includes retina-support. Both the retina-support and the orientation changes make use of the info.plist settings for the app for more freedom of the using developer.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 11, 2014

Member

@BlueCobold Can you describe what the behavior now is for the device orientation bit?

@LaurentGomila Since you wrote the original code, could you take a look at it?

Member

eXpl0it3r commented Dec 11, 2014

@BlueCobold Can you describe what the behavior now is for the device orientation bit?

@LaurentGomila Since you wrote the original code, could you take a look at it?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 11, 2014

Member

Sure.

  1. The two new GLEXT_GL macros that you define are not used, you still use the original macros -- which is good: these flags are used locally in the iOS code, so they don't need to be abstracted in GLExtensions.hpp. So you should remove the GLEXT_GL flags that you add.
  2. As eXpl0it3r said, can you explain the new behavior for orientation?
  3. I don't really like the scaleIn / scaleOut functions and the global function that recomputes the scale everytime it is requested, you should remove all this stuff and just multiply / divide when necessary. The scale factor itself should be retrieved at init and stored where it makes sense (in the app delegate, in the view, or whatever is suitable for it).
  4. What's the new behaviour with support for retina displays? There should be two kinds of "surfaces": those that use logical pixels, and those that use physical pixels. How does this apply to the various SFML entites that deal with pixels (windows, view, ...)? Or, in other words, what changes for the user, and how does he take advantage of the retina support?
Member

LaurentGomila commented Dec 11, 2014

Sure.

  1. The two new GLEXT_GL macros that you define are not used, you still use the original macros -- which is good: these flags are used locally in the iOS code, so they don't need to be abstracted in GLExtensions.hpp. So you should remove the GLEXT_GL flags that you add.
  2. As eXpl0it3r said, can you explain the new behavior for orientation?
  3. I don't really like the scaleIn / scaleOut functions and the global function that recomputes the scale everytime it is requested, you should remove all this stuff and just multiply / divide when necessary. The scale factor itself should be retrieved at init and stored where it makes sense (in the app delegate, in the view, or whatever is suitable for it).
  4. What's the new behaviour with support for retina displays? There should be two kinds of "surfaces": those that use logical pixels, and those that use physical pixels. How does this apply to the various SFML entites that deal with pixels (windows, view, ...)? Or, in other words, what changes for the user, and how does he take advantage of the retina support?
@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Dec 11, 2014

Contributor
  1. Indeed. It is a file committed that was changed for stencil-renderbuffer-support which is not part of these ios fixes. I will adjust it accordingly - no need for these macros in this branch.

  2. When creating a window with landscape-measures (wider than high), the ui orientation will be changed to landscape. When creating a window with portrait-measures (higher than wiede), the ui orientation will be changed to portait. Width and height of the window will be the set to full resolution of the device (for example 2048x1536 or 1536x2048 for iPad3/Air - or even higher for "iPad retina"). In the project properties of an app (Xcode interface) the developer can select the orientations supported by his app. Those are places in the info.plist of the app by Xcode. My code takes these selected orientations into consideration in case the device orientation is changed and adjusts (or not) the window coordinates accordingly. Using window flags which do not allow resize of the window will block this operation - the window size will stay the same (which imo is never what the developer wants, but that part of code was yours and I didn't touch it).
    TL;DR: When the user turns his device, the window will change size (or not, if the app doesn't support this orientation or he doesn't allow resizing his window) to again perfectly fit the changed device resolution.
    (this fixes all bugs described in here: http://en.sfml-dev.org/forums/index.php?topic=16827)

  3. The scaleIn/Out were inspired by the OS X implementation of SFML regarding retina. Since window-coordinates in iOS (and OS X) are based on points and not on pixels, all coordinates SFML takes and presents to the user must be converted from points to pixels and from pixels to points. The window-size on retina is still 1024*768 points, but 2048x1536 pixels. So the code already multiplies and divides only when necessary. But it is necessary at various points. That's also the reason for the functions being global (but internal in SFML, so no user will ever see them). There is no common point I could address to map them into a class method without creating cyclic dependencies. The scale-factor is only loaded once and then stored for the life-time of the app (it can't change anyway).

  4. Nothing "changes" for the user. When he enabled high-resolution-support from his info.plist his window will be 2048x1536 pixels and the list of supported resolutions will say so. Pixel-using entities (circles/rectangles/whatever) will of course result in half the (phyical) size when using a fixed size (as you would expect anyway when doubling resolution, but having an entiy of for example 100x100 pixels). Without it, the user only ever will have half screen resolution (each pixel gets doubled by iOS using a nearest-neighbour-filter). He can enable retina support by setting the "High Resolution Support" entry in his info.plist to "true". This is consistent with the OS X implementation, just that the OS X implementation always uses retina and never gives that choice to the developer (which imo is a bug).
    There is no need for two kind of "surfaces" either. The physical and logical resolution presented to the user will be 2048x1536 (or 1537x2048) instead of 1024x768. It's kind of like a PC user having the choice to make a 600x400 fullscreen-window or a 1200x800.

PS: Would screenshots help you understanding the results of this fix and the previous code?

Contributor

BlueCobold commented Dec 11, 2014

  1. Indeed. It is a file committed that was changed for stencil-renderbuffer-support which is not part of these ios fixes. I will adjust it accordingly - no need for these macros in this branch.

  2. When creating a window with landscape-measures (wider than high), the ui orientation will be changed to landscape. When creating a window with portrait-measures (higher than wiede), the ui orientation will be changed to portait. Width and height of the window will be the set to full resolution of the device (for example 2048x1536 or 1536x2048 for iPad3/Air - or even higher for "iPad retina"). In the project properties of an app (Xcode interface) the developer can select the orientations supported by his app. Those are places in the info.plist of the app by Xcode. My code takes these selected orientations into consideration in case the device orientation is changed and adjusts (or not) the window coordinates accordingly. Using window flags which do not allow resize of the window will block this operation - the window size will stay the same (which imo is never what the developer wants, but that part of code was yours and I didn't touch it).
    TL;DR: When the user turns his device, the window will change size (or not, if the app doesn't support this orientation or he doesn't allow resizing his window) to again perfectly fit the changed device resolution.
    (this fixes all bugs described in here: http://en.sfml-dev.org/forums/index.php?topic=16827)

  3. The scaleIn/Out were inspired by the OS X implementation of SFML regarding retina. Since window-coordinates in iOS (and OS X) are based on points and not on pixels, all coordinates SFML takes and presents to the user must be converted from points to pixels and from pixels to points. The window-size on retina is still 1024*768 points, but 2048x1536 pixels. So the code already multiplies and divides only when necessary. But it is necessary at various points. That's also the reason for the functions being global (but internal in SFML, so no user will ever see them). There is no common point I could address to map them into a class method without creating cyclic dependencies. The scale-factor is only loaded once and then stored for the life-time of the app (it can't change anyway).

  4. Nothing "changes" for the user. When he enabled high-resolution-support from his info.plist his window will be 2048x1536 pixels and the list of supported resolutions will say so. Pixel-using entities (circles/rectangles/whatever) will of course result in half the (phyical) size when using a fixed size (as you would expect anyway when doubling resolution, but having an entiy of for example 100x100 pixels). Without it, the user only ever will have half screen resolution (each pixel gets doubled by iOS using a nearest-neighbour-filter). He can enable retina support by setting the "High Resolution Support" entry in his info.plist to "true". This is consistent with the OS X implementation, just that the OS X implementation always uses retina and never gives that choice to the developer (which imo is a bug).
    There is no need for two kind of "surfaces" either. The physical and logical resolution presented to the user will be 2048x1536 (or 1537x2048) instead of 1024x768. It's kind of like a PC user having the choice to make a 600x400 fullscreen-window or a 1200x800.

PS: Would screenshots help you understanding the results of this fix and the previous code?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 11, 2014

Member

This is consistent with the OS X implementation, just that the OS X implementation always uses retina and never gives that choice to the developer (which imo is a bug).

With a bundled app you can also change the setting in the appropriate plist file. If that's not the way you expect things to work, or if it doesn't work for you, please open a thread on the forum and let's discuss it over there. 😉

Member

mantognini commented Dec 11, 2014

This is consistent with the OS X implementation, just that the OS X implementation always uses retina and never gives that choice to the developer (which imo is a bug).

With a bundled app you can also change the setting in the appropriate plist file. If that's not the way you expect things to work, or if it doesn't work for you, please open a thread on the forum and let's discuss it over there. 😉

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Dec 11, 2014

Contributor

If that's working for the OS X implementation correctly based on the user's setting, then mea culpa. From what I've seen on fast glimps at the code, it didn't look like it would take it into consideration.

Contributor

BlueCobold commented Dec 11, 2014

If that's working for the OS X implementation correctly based on the user's setting, then mea culpa. From what I've seen on fast glimps at the code, it didn't look like it would take it into consideration.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 11, 2014

Member

Out of curiosity, would you be interested in working more on this port, or did you submit these fixes only because you needed them?

Member

LaurentGomila commented Dec 11, 2014

Out of curiosity, would you be interested in working more on this port, or did you submit these fixes only because you needed them?

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Dec 11, 2014

Contributor

Well, I made them because I needed them in any case and I offer these to the public for a general benefit for everyone. I might be interested in doing more work in the future, but it's hard to tell what exactly needs to be done.

Contributor

BlueCobold commented Dec 11, 2014

Well, I made them because I needed them in any case and I offer these to the public for a general benefit for everyone. I might be interested in doing more work in the future, but it's hard to tell what exactly needs to be done.

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Dec 20, 2014

Contributor

I've cleaned up the global scale functions and squashed the bad commits. Clean history now. Any further comments what to improve?

Contributor

BlueCobold commented Dec 20, 2014

I've cleaned up the global scale functions and squashed the bad commits. Clean history now. Any further comments what to improve?

@eXpl0it3r eXpl0it3r removed the s:unassigned label Jan 8, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 9, 2015

Member

@LaurentGomila Anything else?

Member

eXpl0it3r commented Jan 9, 2015

@LaurentGomila Anything else?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 9, 2015

Member

I haven't tested it myself, but it looks good.

Member

LaurentGomila commented Jan 9, 2015

I haven't tested it myself, but it looks good.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 9, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Jan 9, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@@ -36,8 +36,6 @@ @implementation SFViewController
////////////////////////////////////////////////////////////
- (BOOL)shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation
{
(void)interfaceOrientation;

This comment has been minimized.

@mantognini

mantognini Jan 10, 2015

Member

Don't you get a warning about unused variable here when that line is removed?

@mantognini

mantognini Jan 10, 2015

Member

Don't you get a warning about unused variable here when that line is removed?

This comment has been minimized.

@BlueCobold

BlueCobold Jan 10, 2015

Contributor

No, I didn't get any warning about it.

@BlueCobold

BlueCobold Jan 10, 2015

Contributor

No, I didn't get any warning about it.

@@ -151,6 +162,36 @@ - (void)applicationWillTerminate:(UIApplication *)application
}
}
- (bool)supportsOrientation:(UIDeviceOrientation)orientation
{
if (!self.sfWindow)

This comment has been minimized.

@mantognini

mantognini Jan 10, 2015

Member

That would be nitpicking but I'd had some empty lines to make this function more readable, especially after return statements. What do you guys think?

@mantognini

mantognini Jan 10, 2015

Member

That would be nitpicking but I'd had some empty lines to make this function more readable, especially after return statements. What do you guys think?

This comment has been minimized.

@BlueCobold

BlueCobold Jan 10, 2015

Contributor

Empty lines after return statements? Never seen someone doing that, but that's what would stop this PR, its not a problem to add them. If you insist.

@BlueCobold

BlueCobold Jan 10, 2015

Contributor

Empty lines after return statements? Never seen someone doing that, but that's what would stop this PR, its not a problem to add them. If you insist.

This comment has been minimized.

@LaurentGomila

LaurentGomila Jan 10, 2015

Member

Yes. Don't hesitate to add empty lines in your code to make it more redable, by separating logical groups. Don't hesitate to look at the existing code.

@LaurentGomila

LaurentGomila Jan 10, 2015

Member

Yes. Don't hesitate to add empty lines in your code to make it more redable, by separating logical groups. Don't hesitate to look at the existing code.

This comment has been minimized.

@BlueCobold

BlueCobold Jan 10, 2015

Contributor

I'll add some.

@BlueCobold

BlueCobold Jan 10, 2015

Contributor

I'll add some.

@mantognini mantognini added this to the 2.3 milestone Jan 10, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 10, 2015

Member

Here you got my nitpicking! =P

Member

mantognini commented Jan 10, 2015

Here you got my nitpicking! =P

@eXpl0it3r eXpl0it3r self-assigned this Jan 10, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 16, 2015

Member

@BlueCobold Any updates on this?

Member

eXpl0it3r commented Jan 16, 2015

@BlueCobold Any updates on this?

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Jan 31, 2015

Contributor

@eXpl0it3r its up

Contributor

BlueCobold commented Jan 31, 2015

@eXpl0it3r its up

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 6, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Feb 6, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 10, 2015

Member

I squashed the third commit into the second commit.

Merged in cac4d58

Member

eXpl0it3r commented Feb 10, 2015

I squashed the third commit into the second commit.

Merged in cac4d58

@eXpl0it3r eXpl0it3r closed this Feb 10, 2015

@BlueCobold BlueCobold deleted the BlueCobold:ios_fixes branch Mar 2, 2015

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