![[de]](/mwf/flags/de.png "Germany")
I made a Apple Cocoa port of OC. It doesn't use SDL for display purposes and in contrast to the current SDL based Mac version it supports the developer mode:http://cdn.bitbucket.org/mortimer/openclonk/downloads/macdevmode.png
Anyone interested? The branch can be found here: http://bitbucket.org/mortimer/openclonk2 (macdevmode) (it's now openclonk2 because the old one caused bitbucket to crash around...)
Well, thanks for attributing some code to me via copyright headers, but I'm quite sure I've never touched anything in the xcode directory :)
Besides that, I don't know who here uses a mac, so I can't really comment on the code; but I think it's a nice touch to have native interfaces on all OSes, so I'm in favor.
Besides that, I don't know who here uses a mac, so I can't really comment on the code; but I think it's a nice touch to have native interfaces on all OSes, so I'm in favor.
I wonder if it's an good idea to have multiple codes for multiple OSes, but as long nobody makes some platform independant gui, it's allright to add this, too, I guess.
I'm not using a Mac so I can't try it out. Are there any regressions to the default branch and/or something that remains to be implemented? If not I'd say go ahead and merge it.
I have also seen that you started an OpenAL implementation for the sound system. Can you comment on how usable it is? I'd prefer to using it instead of SDL on Linux as well, and if we want to get rid of fmod on Windows (do we?) then that might be a good alternative. Maybe we can even use it on all three platforms :)
I have also seen that you started an OpenAL implementation for the sound system. Can you comment on how usable it is? I'd prefer to using it instead of SDL on Linux as well, and if we want to get rid of fmod on Windows (do we?) then that might be a good alternative. Maybe we can even use it on all three platforms :)
> if we want to get rid of fmod on Windows (do we?)
Well, I certainly do, because having multiple different libraries depending on which OS we're building for is a recipe for disaster. This usually can't be avoided to get nice UIs, but sound output is something that's pretty consistent across all OSes.
>If not I'd say go ahead and merge it.
Nah, it still has some issues :)
And it does some really ugly things like renaming 'pi' defined in the Clonk source to Standard_pi to avoid conflicts with pi defined in Apple headers :c
>Can you comment on how usable it is?
Not usable at all since I didn't implement anything :p
Please do a rebase of this code to get rid of the merges (did you really merge the noref branch? why?). I know mercurial makes this unnecessarily laborious, but it is still worth the time and effort. Not only is it easier for others to review, but in this part of the code, you often need to check whether a particular piece of code is important for other ports, and having a easy to understand history helps.
Oh okay. I had a feeling this was all too messy. And for why I merged noref, dunno, seemed to me it would soon become official :p
Comments on the first half:- I don't like those "id" changes. Imho, .cpp files shouldn't be compiled as if they were Objective C++, because that's just not what they are.
- I'd suggest squashing "Startup code USE_COCOA aware" and "Reorganize main code by putting the components of the application's lifecycle into a class named C4Main" because the latter deletes questionable code the former introduces. Though maybe some stuff from C4WinMain.cpp should just be put into C4Application, and the remaining trivial main function just duplicated.
- I don't like "Mac: Shuffle drawing code around to make it easier to call it inside a NSView drawRect: method". It seems to disable automatic drawing in some circumstances and define an interface to respond to expose events. That can and should be done without ifdefs. The other platforms just need a stub "StdWindow::isVisible() { return true; }", and a standard StdWindow::Redraw() method. Or does the cocoa code try to post expose events which are later responded to? That can be done with a standard StdWindow::MarkForUpdate() (or similar) method, which just calls Redraw() normally and does the roundabout thing for cocoa.
- A lot of the copyright headers are bogus. We either need to all agree to just put a "Copyright (c) 1998-2010 various contributers", or you need to carry over the copyright messages along with code you move, and put just your name on new files written from scratch.
- The commit adding StdAppCommon.cpp needs to add the file to at least CMakeLists.txt, and ideally also to Makefile.am.
- If you're not using StdSDL*, excluding it from the build is more elegant than putting in #ifdefs.
- What's the point of making lots of functions public in "Mac Console: Output + Save Panels"?
- "Mac: Implement CStdGLCtx for Cocoa" has inconsistent indentation. Also, we should probably look some more for redundancy in the four implementations, but that can wait.
- The #ifdef mess in the developer mode implementation is ugly, but I don't have any bright ideas how to improve it.
- There should be no source code in the xcode/ directory. That's only for build system stuff.
- The material preview in the toolbox should probably be done with OpenGL for all window systems, but that can also wait.
- Ah, "Add StdAppCommon to CMake" and "Include C4Include in StdAppCommon.cpp since that seems to be required by VS" should be squashed into the above-mentioned "Add StdMacWindow.mm and move some functions defined in StdWindow.h to StdAppCommon.cpp".
- "Mac: Code that should do something but doesn't" is a terrible commit message :-p
Next half later.
Thanks for reviewing.>id changes
I just compiled everything in Objective-C++ since I was too lazy to sort out only those files that somehow include header files with Objective-C declarations. It seemed to me that most do anyway :/
> I don't like "Mac: Shuffle drawing code around to make it easier to call it inside a NSView drawRect: method
I introduced those Do* methods so that calling the normal methods would still cause drawing the content even if indirectly by marking some Cocoa view as needing to be redrawn while calling the Do* methods would invoke the actual drawing code. But well, it is really ugly ;)
> Copyright headers
Yep, I somehow overlooked the list of contributors and just copy-pasted some arbitrary header o_o
> SDL
I added #ifdefs to not have to change my custom Xcode project file which is not generated from CMake since CMake seemingly can't create Xcode files with cpp files being set to compile in Objective-C++ mode -.-. But maybe one can add 'set(CMAKE_CXX_FLAGS "-x objective-c++")' or something.
> Making functions public
I think I'm calling those methods somewhere. Probably in ConsoleWindowController.mm responding to UI events.
> Terrible commit messages
Well.. yeah :p
> I just compiled everything in Objective-C++ since I was too lazy to sort out only those files that somehow include header files with Objective-C declarations. It seemed to me that most do anyway :/
As far as I can see those id changes are in files that don't need Object-C stuff. You might have to move some #includes around, but sooner or later someone probably will use "id" again, and we don't want the mac port to break then. Really, if it weren't for the developer mode with that mixture of GUI and engine internal code, everything could be nicely encapsulated in src/platform without any leakage.
>> I don't like "Mac: Shuffle drawing code around to make it easier to call it inside a NSView drawRect: method
> I introduced those Do* methods so that calling the normal methods would still cause drawing the content even if indirectly by marking some Cocoa view as needing to be redrawn while calling the Do* methods would invoke the actual drawing code. But well, it is really ugly ;)
Well, what do you think of my suggestion? (
StdWindow { virtual void ReDraw() = 0; void MarkForUpdate() { #ifndef mac ReDraw() #else FiddleCocoa() #endif } };) I think it's really the same algorithm, but with the platform differences hidden in src/platform. And that way, all platforms could use the standard interface to redraw a window in response to expose messages, and not simply redraw everything constantly even when the game is paused.> I added #ifdefs to not have to change my custom Xcode project file which is not generated from CMake since CMake seemingly can't create Xcode files with cpp files being set to compile in Objective-C++ mode -.-. But maybe one can add 'set(CMAKE_CXX_FLAGS "-x objective-c++")' or something.
Ideally we wouldn't need to... But for the time being, it should suffice to compile src/editor that way. Maybe src/editor/*.mm files which #include the *.cpp ones?
> I think I'm calling those methods somewhere. Probably in ConsoleWindowController.mm responding to UI events.
The Win32 port does this with a friend declaration, the GTK one with a bunch of static callback functions. Doesn't "friend class ConsoleWindowController" work?
>As far as I can see those id changes are in files that don't need Object-C stuff. You might have to move some #includes around, but sooner or later someone probably will use "id" again, and we don't want the mac port to break then. Really, if it weren't for the developer mode with that mixture of GUI and engine internal code, everything could be nicely encapsulated in src/platform without any leakage.
Ok.
>Well, what do you think of my suggestion? (StdWindow { virtual void ReDraw() = 0; void MarkForUpdate() { #ifndef mac ReDraw() #else FiddleCocoa() #endif } };) I think it's really the same algorithm, but with the platform differences hidden in src/platform. And that way, all platforms could use the standard interface to redraw a window in response to expose messages, and not simply redraw everything constantly even when the game is paused.
Sounds good.
>Ideally we wouldn't need to... But for the time being, it should suffice to compile src/editor that way. Maybe src/editor/*.mm files which #include the *.cpp ones?
Oh, I didn't think of that one. But that could work I guess.
>The Win32 port does this with a friend declaration, the GTK one with a bunch of static callback functions. Doesn't "friend class ConsoleWindowController" work?
I'm afraid not since ConsoleWindowController is an Objective-C class which isn't compatible with C++ concepts like 'friend'. But of course it's no big deal to just write some small wrapper C++ class if those functions really should be kept private.
> I'm afraid not since ConsoleWindowController is an Objective-C class which isn't compatible with C++ concepts like 'friend'. But of course it's no big deal to just write some small wrapper C++ class if those functions really should be kept private.
Hm. I would have thought that Objective C++ would be better integrated than that. After all, it's apparently integrated enough to respect "protected"...
Yet another set of wrapper functions certainly won't make the code any uglier, but they do raise the question whether there's a better way. Maybe we could change all those callbacks to be static and let them just fetch the singleton C4Console instance themselves. That way, most Cocoa and GTK+ callbacks could be replaced with a generic one that gets the appropriate function in the data argument, and the ConsoleDlgProc would use C4Console::Foo() instead of Console.Foo(). Maybe you have a better idea? Don't hesitate to change the existing code, or make suggestions that require someone using GTK+ and/or windows to make changes.
> Mac: Rearrange Console Window to more closely resemble the original (separate tool windows etc)
What's the deal with CStdWindow::DrawableWindowContent? I don't think it's a good idea to have the ToolsDlg interact with StdWindow stuff - the latter is complicated enough with only being responsible for viewports and fullscreen. Using it for the developer mode widgets is a bad idea, imho.
> Fix a bunch of small issues
Could you put these into the respective commits which introduced the issue, and drop the "id"-related changes?
> Mac: Work on handling fullscreen mode
Okay, here I really have to say no: C4FullScreen deriving from different classes depending on platform is bad. Just define the cross-platform StdWindow::Redraw() method already mentioned.
> Mac: Set the caption of the console window to the scenario filename and also set the window icon to be a pointer to the scenario.
Looks good, I think.
> Mac: Add OpenClonk license header to some Cocoa source files
The copyright notices seem inaccurate to wrong.
> Use USE_SDL_MAINLOOP instead of HAVE_SDL in some places
This looks wrong: On Linux, SDL is always used for gamepad input.
The OpenAL stuff looks good, though I didn't look closely.
> Mac: Some fixes that somehow fell under the carpet
Well, an empty method isn't very useful. And GetMainCtx got deleted for a reason. It should never be used, contexts should be an implementation detail of a C4Surface.
> Add check to prevent NULL pointer access in C4GameMessage
Heh, that just came up again today when somebody reported the crash. You are just papering over the bug, Target should never be 0. It looks like C4AulObjectContext doesn't prevent nullpointer anymore, but I didn't look into why.
Whew, and I'm through. Do you need help with the commit-editing? (A quick search suggests http://mercurial.selenic.com/wiki/HisteditExtension) Should I write the suggested StdWindow::ReDraw()and StdWindow::MarkForUpdate() and make the current code use it?
Sorry for note answering...In the meantime, I've accumulated more changesets which aren't as of yet published on bitbucket.org (rebasing requires stripping and re-uploading a lot of changesets as long as I'm not missing something...)
Those mostly revolve around encapsulating platform-specific console gui code into separate files which then don't require #ifdefs anymore. I'll upload them when I get the code base to compile again for all three platforms (as of now, Linux still needs some rearranging and since I've put Linux on my old laptop, I can try to fix that myself, I guess :p)
I made a Mac OSX binary build from the openclonk sources kindly provided by Mortimer which should run without installing any dependencies (bundled them together):
http://www.eternium.de/openclonk
Feedback is appreciated. It would be great to have official OSX builds eventually.
Regards,
David
http://www.eternium.de/openclonk
Feedback is appreciated. It would be great to have official OSX builds eventually.
Regards,
David
Date 2010-12-01 17:47
![[pt]](/mwf/flags/pt.png "Portugal")
Not working :(
Hi, just found about the project and tried the binaries you posted but the game crashes after I change the username in the starting screen. I can test it more, and provide any feedback.
I understand that the code has been updated after you created the build, if you can provide a more up to date package I'll be glad to test it.
Official OS X builds would be great. I know I would like to play the game on my mac :)
Hi, just found about the project and tried the binaries you posted but the game crashes after I change the username in the starting screen. I can test it more, and provide any feedback.
I understand that the code has been updated after you created the build, if you can provide a more up to date package I'll be glad to test it.
Official OS X builds would be great. I know I would like to play the game on my mac :)
New round of comments:> 9e65b5f8f15d Some fixes to C4SoundLoaders
Our headers don't include C4Include.h, our .cpp files do.
> b75512222aa6 C4Config: Save & Load -> same file
Why are you using C4ENGINENICK and not C4ENGINEID? I thought reverse DNS stuff was the norm on Macosx. If C4ENGINENICK is better, C4ENGINEID should probably be completely removed, it's only used on Macosx.
> a14b0ddf38c2 Don't crash when accidentally invoking C4Startup::Start twice
When does this happen? The assertion claims that it is a bug when the function is called twice.
> ea91bfb0b930 Mac: Add Cocoa port
CStdApp::Init doesn't need to build a windows-style commandline anymore.
This still puts ObjectiveC code into C++ files. It even regresses things, moving stuff out of MacUtility.mm. Please don't, the C++ files should be completely
readable by anyone who knows only C++.
I don't think the C4Main class makes stuff better. The interface from the platform-specific code to C4Application is minmal enough: Call Init(argc, argc), followed by Run(); and Clear();. Then, do the restart if necessary. There doesn't seem to be any common code that is worth sharing between Linux/SDL on Mac and Cocoa in C4WinMain.cpp.
What do you need CStdGL::GetMainCtx() for?
Please do not rename id to the_id.
The C4GamePadCon changes look wrong, the Linux port uses SDL for gamepad input, but the default is not to USE_SDL_MAINLOOP, but X11 or GTK.
The C4InteractiveThread.h, C4Network2.h and probably a few other changes should be in a separate commit.
At this point I looked at some of the later commit messages and realized that some of these comments are probably already addressed. So instead of reading further, I'll give a meta-comment: Please rebase your patches and fold patches fixing bugs that are introduced by earlier patches into those patches. At the moment it's almost impossible to tell whether your patch series will introduce problems.
A quick search through http://hgbook.red-bean.com/read/managing-change-with-mercurial-queues.html yields "qfold" as the tool to do this with mercurial. I'm using git rebase --interactive, so I don't have experience with it, but I think Isilkor told me that mq is usable.
Putting ObjectiveC code into C++ is not that big of a problem, I'd say. It's just the language used for the preferred API of the platform. It doesn't really make that much of a difference if some #ifdef ... #endif code contains calls to an unknown API or if that code additionally is written in a language that might seem a little bit foreign at first.
An unknown API is often quite guessable in what it does, so that one can do some trivial transformations necessary to keep code of other platforms working when the common code changes. This is already hard enough as it is, we should not make it more difficult. And dealing with ObjectiveC syntax does make it more difficult. For example, I have no idea what the [] do, and wouldn't know how to make sure that code still compiles that uses them if I would want to exchange a variable with a function call or something.Of course the real answer is that we shouldn't have #ifdefs in the common code in the first place :-) You already did a lot of work in that direction. So maybe I can help removing the leftovers, making ObjectiveC code in .cpp files unnecessary?
http://bitbucket.org/mortimer/openclonk2/changeset/63546882b63f \o/ (link might change, rebase, yadda)
Hm, nice, but:- C4_OS is mostly used for the update package name. I still think we should ship a combined Linux 32+64 bit package, but given Macosx "universal binaries" we really should only ship one Macosx package. So that change doesn't help.
- I still disagree with the Execute/DoExecute stuff, the DrawableWindowContent, and a lot of other interface choices. These interfaces aren't the most sane to begin with, so we need to be careful with them ;-) Apparently, Cocoa wants to do stuff with C++ class instances, while Windows, GTK+ and X11 all use opaque handles (well, GTK uses not-so-opaque C objects, but one can ignore that). We tried to hide these behind the StdWindow class, but apart from some event delivery mechanisms for X11 the individual window types do not really share code. Unfortunately, I don't have a good idea how we can design something better without someone learning how all three platforms work. Maybe we can approach it from the Clonk side?
- I really think we should not attempt to share the code that currently is in C4WinMain.cpp between the platforms. CStdApp::RestartAtEnd is not a method we will ever need on Windows due to the way the upgrade has to work there, so it shouldn't be part of the common API. The common API is C4Application::Init, Run and Clear. Posix and Cocoa can share the restartAtEnd variable, but there's no reason to share more.
Maybe we should start by applying the patches to split the editor implementations into platform-specific files, the changes to the sound system, and the other Cocoa-unrelated changes you made. And then work on the interfaces for StdWindow and friends. After that, the Cocoa port should be mostly code additions, which are easy to check for changes to the common code, and we can just trust your choices for the Cocoa parts and merge. Sounds like a plan?
![[gb]](/mwf/flags/gb.png "United Kingdom")
I would vote to properly separate them. It took me forever to realize that buried somewhere in C4Group.cpp was an "id" that the Objective-C compiler treated differently than the C++ compiler, leading to corrupted groups.
Okay, the c4console-movement-patch looks almost ready. I have a few nitpicks, will do a few tests, and will try to get some windows developer to have a look, but feel free to push that patch. I'll look at the subsequent ones tomorrow.- src/editor/C4PropertyDlg.cpp: The change to the first line is obviously unintended
- some files miss copyright and license notices
- C4Console.cpp has a few empty or almost empty wrapper functions like FileSelect, but it's conceivable that they won't stay empty, so no need to remove them
- not your fault, but introducing anonymous namespaces instead of continuing to use static functions was a mistake. I should not have let myself be blindsided by C++ propaganda on that one.
- I still don't like redirecting the platform-specific implementation of things like C4ToolsDlg::EnableControls through a second to a third class. At least move the middle function to C4ToolsDlg. Though we might want to rethink the code organization in general for these classes. The few leftover common pieces of code could be moved to more appropriate classes like C4Object and C4Landscape, making the GUI code an implementation details of the platformspecific parts of the console. So perhaps this can be left as-is and we'll clean this up later with a bigger broom.
edit: Okay, this should be fixed before pushing:
src/gui/C4Viewport.cpp: In member function ‘void C4ViewportList::Execute(bool)’:
src/gui/C4Viewport.cpp:776: error: ‘class C4Viewport’ has no member named ‘GetWindow’
Powered by mwForum 2.29.7 © 1999-2015 Markus Wichitill
