Matthieu wrote:

Dit it. Looking at the code, maybe some tests have to be done on osx < 10.5. My patch have been tested on OSX 10.8.2.

Attachments

• fs4847.diff (5.42 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4847#comment11953

Alberth wrote:

Hi,

Great that you decided to try and fix the open OSX bugs.
I had a look at your patch, and even though I don't understand objective C, I believe you can improve on the patch.

Your current patch seems to do several things at the same time, making it hard to understand. It
- adds and removes empty lines for seemingly no reason at all,
- changes things in the font handling, which look unrelated to bootstrapping,
- it makes changes to several functions around "applicationDidFinishLaunching" and friends (which seem to belong here, but is it really one elementary change?).

For you the structure of the change is clear, but we have to reverse-engineer that structure from the text-changes, so less cluttered patches are better. Solve one problem at a time instead of 2 or 3. Also, solve a problem in small semantic changes rather than lumping them all together (the latter takes practice, but it's useful to keep in mind). Look at the commits that we do for examples.

I would suggest you drop your font changes, they don't belong here (bootstrapping and font support are different problems to me).
In the same way, don't mix whitespace changes with this patch (bootstrapping and code-layout are different problems too).

Not sure whether it applies here, but it might be useful to add some description of what a patch aims to fix.
In particular, as changes in the Apple code have a history where fixes for one system breaks the program at another system, so we want to be very careful here.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4847#comment11955

Matthieu wrote:

I'm french and my english is not very good...

I've take a look at the coding style recommandations before patching but didn't mind that removing empty lines have to be made in a separate patch. Note that those lines have no reasons to be. Moreover in others OSX files, the code flow is not made the same way...

For me, bootstrapping process have to begin when no gfx set is detected and finish in a state that the game is when it has a gfx set at launch. Maybe i made a mistake.

I'll try to explain my changes:
First, to allow running bootstrap, i had to remove the preprocessor macro that hide it to APPLE arch (bootstrap_gui.cpp).
Looking at the others preprocessor macro on the same line, bootstrap is launched on WIN32 or if fontconfig is enabled. Then i removed from config.lib the lines that here disabling fontconfig for OSX.
Now, bootstrap can be launched.
Calling CheckForMissingGlyphs cause the app to crash due to a lack of initialization when calling FindMissingGlyphs in SetFallbackFont method (fontcache.cpp). So, i removed it. Moreover another call to that method is made at the end of SetFallbackFont method where enough init has been done and then the app continue in the bootstraping process.
Now, changes to cocoa_v.mm. You can look at the comments i wrote in patch. The main problem was that from the time there is no bootstraping process, some methods were not adapted to game modes GM_BOOTSTRAP and GM_MENU:
- cursor hidden within bootstrap process cause there is no sprite to draw it => must use default system cursor.
- The app main loop is launched for GM_BOOTSTRAP, stopped and relaunched for GM_MENU. On the first launch, it calls applicationDidFinishLaunching which launch game loop. But applicationDidFinishLaunching is only called once in OSX. So i had to made some changes to launch the game loop at the second run of app main loop.

I hope my explanations are clear enough.

My changes will not break the program on other system because changes remains only on preprocessor macros for APPLE arch. As i said earlier, some tests must be made on OSX platforms < 10.5.

Can you tell me if i'm really wrong looking at my comment. Thanks.

I send my patch without empty lines removing. Look at the third version as the second is not a good version. Sorry.

Attachments

• fs4847_2.diff (5.17 KiB)
• fs4847_3.diff (5.18 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4847#comment11957

Rubidium wrote:

Hi,

I'm seeing some approaches that aren't, in my opinion, the right one.

Fontconfig is for some reason seems to be not the method that is used on OS X; otherwise there wouldn't be an OS X specific font fallback method. Or is it completely superfluous, i.e. can the OS X specific code be dropped? As far as I can see it looks like fontcache is not available on all OS Xes that are claimed to be supported, so I would think it can't be. Moving the !defined(APPLE) as defined(APPLE) in the or with WITH_FONTCONFIG and WIN32 will make the bootstrap code be called (no need to enable fontconfig on OS X). The !defined(APPLE) was added there because it crashed.

The call to FindMissingGlyphs is needed for the case where Mac OS X < 10.5 is used. The method failing means that that issue needs to be addressed. Removing it is not the solution.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4847#comment11959

Matthieu wrote:

Hello

Ok, i understand that fontconfig is not needed on osx.

I said earlier that calling FindMissingGlyphs (in the beginning of SetFallbackFont) made the application to crash. That was not really true. In fact, an error is raised by the code in getGlyph function in fontcache.cpp when calling FindMissingGlyphs in SetFallbackFont function.

if (face == NULL) error("No sprite font and no real font either... bailing!");

That mean that no sprite have been found for the passed in character nor for the replacement character "?".
There is no code before that call that can provide some font where to get sprites (even in the freetype initialization). That's why i removed the call to checkForMissingGlyphs. Those fonts are provided in the SetFallbackFont function. And at the end of this function, a second call to FindMissingGlyphs is made. And then it works.
I don't understand why you say the first call to FindMissingGlyphs is needed for the case where osx < 10.5. Looking at the code didn't help me understanding that. The second call to FindMissingGlyphs is made whatever the osx version you compiled against.
Can you tell me more about please ?

Attachments

• fs4847_4.diff (4.71 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4847#comment11987

Rubidium wrote:

Line 491 has the first call to FindMissingGlyphs, which then sets "str".
Line 570 reads "str", thus "str" must be initialised (removing FindMissingGlyphs makes it not initialised). The following lines try to find a font that contains the characters given in the string, so we need to have some string data of the language in which we are going to show the message.
Line 661 calls FindMissingGlyphs.

The attached patch might do the trick, but it needs to be compile tested and properly tested.

Attachments

• fs4847_add.diff (0.51 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4847#comment11989

Matthieu wrote:

Hi.

I see. But i don't think that's the good way. Calling findMissingGlyphs with a null string param is the same as calling with NULL param. So there seems to be no need to pass "str" to it.

Looking at others platforms implementations of setFallbackFont, we must ensure that font have glyph for each characters or test next available font. Osx implementations (> 10.5 & < 10.5) don't really do that check. The implementation for < 10.5 seems want to do that but since str is always null, nothing is tested. Moreover none of those implementations take into account the result of findMissingGlyphs.

What we must do is calling findMissingGlyphs for each font tested. Moreover for the osx < 10.5 implementation, we have to init str with a value. I think we must init it to '?'. That's the first character tested in findMissingGlyph and it's the replacement character.

There is another way to implement the version for osx < 10.5. Keep the actual implementation and make a call to findMissingGlypha at the beginning. Then we have to correct findMissingGlyph for it to return the string to check the font against… But that's not the solution i choose… I let people making some comments about my choice.

I creates a crosscompiler under my debian VMWare for i386 and ppc architecture. So that works and my code compiled and run. Some other tests by others could be made on ppc. For crosscompiler, i'll try to make some improvements for universal building with x86_64 arch… but it's another story.

Attachments

• fs4847_5.diff (8.66 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4847#comment12013

michi_cc closed the ticket.

Reason for closing: Fixed

In r25661.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4847