#8425 closed patch
EPOC/Symbian port
| Reported by: | SF/sumthinwicked | Owned by: | anotherguest |
|---|---|---|---|
| Priority: | normal | Component: | Port: Symbian |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hey folks, here is the first version of the first patch I have ever submitted. Basically it's an RFC, so don't hesitate :P
There are some #ifdefs in weird places, but without it gcc/UIQ will barf all over itself :(
Also please not that I am not the author of the original ports (0.5.1, 0.6.1 & 0.7.1, done by Andreas 'Sprawl' Karlsson), I am only the one that is trying to get it up to current ScummVM standards and into CVS.
If you want to try to build this puppy yourself using the UIQ2.1 framework, then ask me for some pointers first, so you don't have to invent the wheel all over again. (And trust me: there's a lot of wheels to be invented!)
I'll be seein' ya! SumthinWicked
Ticket imported from: #1188433. Ticket imported from: patches/530.
Attachments (3)
Change History (46)
by , 19 years ago
| Attachment: | ScummVM-EPOC.v1.diff added |
|---|
comment:1 by , 19 years ago
http://www.scummvm.org/documentation.php?view=conventions
And if possible, compress your patch before submitting.
comment:2 by , 19 years ago
Some comments/questions on the patch (besides what Eugene already said :-)
* The mods in common/scaler.cpp don't make sense to me. Why are they there? Looks not like something I'd accept into CVS...
* printf uses are commented out. But OTOH, printf is #defined. Why do both? Anyway, we can try to replace most usages of printf with warning/debug calls, if that helps.
* g_sysfont_huge: why that? you really need a *bigger* font? Last night you made the impression that you need a *smaller* font... huh?
* The hint about ttf2bdf is useful; I guess it would be a good idea to add this, as well as some rough info on how to use convbdf (and its output), somewhere. Not sure where, though, maybe to tools/README
* You modify the MSVC8 project files, is that on purpose? If so, what do the changes do?
* All your new source files must have a GPL / copyright header. This is important. You probably will want to give credit to the original author, yourself, but also the ScummVM team. Look at e.g. backends/sdl/sdl.cpp
* The copyright header in backends/fs/symbian/symbian-fs.cpp is obviously wrong
* The change in OSystem_SDL::setupIcon (allocating 'icon' on the heap) was made due to stack size limitations, I assume? If so, I'd explain that in a comment next to the code, otherwise somebody might revert it. Also, out of curiosity, is that code actually used on Symbian?
* Why did you add a commented out WRITE_UINT16 in scummsys.h?
* What about the RandomSource::RandomSource() mods?
* What does this strange "releaseTimerResource" code in scumm/smush/smush_player.cpp do ? It looks a lot like a deeply flawed attempt to provide thread sync... does the Symbian port feature 'real' threads, or are they emulated?
comment:3 by , 19 years ago
First: thanks for the comments, second: this is not originally my port: so a few of the changes to 0.7.1 I don't even understand, they just work and need to be there for it to compile on gcc/UIQ. I am open to better suggestions to handle some of the hacks. Let's discuss them. I am in the process of trying to figure out why each mod was made and possibly working around them.
* common/scaler.cpp: it's a way not to compile the scalers in and still be able to use SDL, this removes the dependencies on scalebit.cpp & friends. Since the UIQ build system can't use DISABLE_SCALERS... Any suggestions?
* printf: true: it's a combination of both.. at first when I was looking through the code I noticed that Andreas had commented these out, without documenting why: later I found out that EScummVM crashes when something is printf ()'ed. Later I 'constructed' the neater #define, so the outcommenting can go now. I added this yesterday. Night. Uhh.. morning :) debug calls would be the best solution, then the entire #define hack can go. I changed them to debug (1, ...) for now...
* g_sysfont_huge: yes: bigger and more to the point: fatter: you can't read the normal g_sysfont_big font @ around 320x200. If there is a better way to make the font more readable for small screens without introducing this font please let me know.
* ttf2bdf: invent a wheel: tell people about it :)
* MSVC8: I shouldn't have included these: all-nighter-oopsy. The files on CVS do miss some .cpp files by now, especially for gob & saga I thought.
* GPL: true: all header stuff that is in there was done/omitted by Andreas. I think I did one file: ScummApp.cpp. I added headers to all the files now.
* OSystem_SDL::setupIcon: that was another one of those undocumented changes. Actually: Symbian can do without it, so I removed the changes and added another !defined() around the call of setupIcon(); (MacOSX doesn't use it either)
* WRITE_UINT16: because in CVS it's not there at all: the LE/BE READ/WRITE macros are incomplete: maybe because they are not used (yet). Don't we want to complete the READ/WRITE-16/32 set? I added the missing macros.
* RandomSource::RandomSource(): another undocumented one by Andreas. I added a comment explaining this: // SumthinWicked says: hmmm.. weird.. I could swear time(0) used to cause problems on Symbian/WINS... It seems to be compiling ok now, so I decided to remove the mod entirely.
* releaseTimerResource: in short: I haven't the faintest... we would have to ask Andreas. I'll send him a message to ask him if he would look at my work so far...
* coding conventions: backends/epoc code is all by Andreas: I will update these later...
Cheers, SumthinWicked
comment:4 by , 19 years ago
* Your build system can't use DISABLE_SCALERS ? Well, no problem, simply do not compile those files *at all*, that should be sufficient. Or do you need some select scalers/functions from scaler.cpp? If so, tell us which, and we can discuss whether it makes sense to split the file
* One problem with #define'ing printf probably is that the Debugger has a method with that name. Well, we could rename that method, no big deal.
* In some cases, we would replace printf with debug(0,...) or debug(1,...), in others with warnings; there are a few cases, though, when we really want to print something... hmm
* You can't read g_sysfont_big at 320x200 ?? Hu? You definitely can over here. And you can also read the small font. Maybe your problem is not the pixel size, but rather that you are looking at a tiny screen? I imagine if 320x200 was displayed at the size of a small stamp, I'd have troubles reading the font...
* ttf2bdf -> I put a README file into the tools/ directory which mentions it
* Regarding WRITE_UINT16: We use the READ_* macros for LE/BE specific access, nothing else. READ_UINT32 is an anachronism, used together with MKID, that's all. There's no reason for a WRITE_UINT16 macro, or simililar "native endian" read/write macros.
* releaseTimerResource: good
I'll now take a look at your new patch.
comment:5 by , 19 years ago
| Owner: | set to |
|---|
comment:6 by , 19 years ago
* what is 'backends/epoc/build/.placeholder' good for?
* why were empty destructors added to various ~Debugger classes
* The change in simon/vga.h is possibly needed on other systems, too. I extended it and commited it to CVS.
* In OSystem_SDL_Symbian::OSystem_SDL_Symbian, you do this: ConfMan.set("FM_high_quality", false); ConfMan.set("FM_medium_quality", true); That seems odd, you always override these values. Either you really meant to use registerDefault here; or, if the values are hard coded anyway, don't store them into the config manager, and instead use a #define for them...
* Our source files should end with a newline; many of the files in the epoc/ dir do not.
* Of course, you two guys should be covered in the credit lists.
* I am astonished that g_sysfont_huge works for you, considering that our font renderer currently only supports fonts with a width of up to 16 pixels, while the huge font is 24 pixels wide...
comment:7 by , 19 years ago
* DISABLE_SCALERS: true: not compiling is already what I do, but graphics.cpp/.h uses stuff in scaler.cpp. Symbian only uses GFX_NORMAL, so only Normal1x is used. I copied the trivial Normal1x() to SymbianOS.cpp together with gBitFormatand and InitScalers(). Now scalers.cpp is no longer nescessary.
* printf: there are situations where you want scummvm to report errors, just not warnings/notices, hmm yes: we should introduce a notice(...) fcn in the style of the warning() and debug() fcns. Output could then easily be disabled for __SYMBIAN32__. We could leave the Debugger printf() alone then.
* WRITE_UINT16: I removed my additions. sky/rnc_deco.php: I remember now: there was a WRITE_UINT16() in here: it was used by Andreas in a part of fcn: RncDecoder::makeHufftable() if (huffLength[i] == bitLength) { WRITE_UINT16(table, (1 << bitLength) - 1); table++; this seemed no longer nescessary, so I removed it, I'll have to ask Andreas if there was a reason for the macro.
* 'backends/epoc/build/.placeholder' is used in the build process to create the .sis Symbian installer package. It ensures that the directory 'C:\documents\EScummVM' is created, so scummvm.ini can be saved here by the app on first execution. If omitted, the dir is not created & the save fails. We could add a createDir() for this, but I think this is better: this means the dir & .ini are removed on app uninstall. The installer is configured to not remove this file on an upgrade.
* empty destructors for ~Debugger classes: without them compilation fails :( They do no harm, right? I added comments to signify their importance? Hmm .. weird: I tried moving the destructor implementations from the .cpp to the .h files, but gcc/ARMi b0rked, so I had to move them back.
* ConfMan.set("FM_*_quality", *): you have found another :) I assume this is to preserve processing speed: there might be a better way to handle this though... I guess we need to ask Andreas again :)
* newlines: fixed.
* coding conventions: fixed.
* credits.pl: would that be me under ScummVM team and Andreas under Contributors? I'm not sure where and if Andreas wants to be... something to add in the mail I'll send him I guess :)
* g_sysfont_huge: I experimented a bit with _enableScaling & different fonts. This seems to work for now. I'll work on it a bit more lateron: maybe we don't need to add this font at all... I also figured out why you thought there wouldn't be problems with g_sysfont_big @ 320x200: in graphics.cpp there is a piece of code in setGraphicsMode() that changes to GFX_DOUBLE if (_screenWidth * newScaleFactor < _overlayWidth). If you comment out this code, scummvm does not even work correctly anymore: game elements are ok, but the UI still renders 2x! My P900 has 208x320, so it's pixel perfect!
* added a comprehensive README with full build instructions, links, kudos & the like
* I'm trying to find out why this is: if you compile in config- file.cpp, then the application will fail to start in the WINS UIQ Emulator with an "Application couldn't be started" error. Any ideas? Any unhandled exceptions in the new config-file.cpp?
comment:8 by , 19 years ago
* "empty destructors for ~Debugger classes: without them compilation fails" -> interesting what error do you get?
* g_sysfont_huge: I still don't understand how it ever could have worked for you. The font renderer (!) doesn't support wide fonts. I.e. the code responsible for drawing the font, no matter whether it is scaled or not.
* config-file.cpp is only used by some SCUMM-HE code at this point. Any run time failures make no sense.
comment:9 by , 19 years ago
What's the status of this? I'd like to get this integrated soon, but I still need to hear some answers:
* g_sysfont_huge is still unclear. As I explained, our font renderer will not even work with that font, so I don't see how it was working for you. Can you elaborate?
* config-file.cpp had some missing code in older revisions, maybe that caused your problems. Does it work with latest CVS?
comment:10 by , 19 years ago
In the meantime, some additional OSystem methods were added, you may have to slightly adapt some of your code (maybe not, though, since you are subclassing the SDL backend).
If you can provide a new patch which applies to CVS, and clarify the remaining questions, I am sure we can have this in CVS very soon (and you can get commit access etc.)
comment:11 by , 19 years ago
hey folks, just to let you know: I'm in Mexico now on an Internship for my University degree. I'll be here for about 5 months. After I settle in and get all my things in order (like my room here, my new notebook, P900 docking station/software, VS2005, UIQ2.1 build environment, etc) I will resume work on the EPOC port. I suspect this to be in about a week.
Further, I haven't heard form Sprawl/Andreas yet, and I would really like him to take a look at my work so far, as he still knows more about the EPOC platform than I do...
so, hope to post here again soon!
regards, SumthinWicked
comment:12 by , 19 years ago
Hi! Well.. actually it is me that has made the latest ports & updates to the EPOC/Symbian port. It seems like some of my patches has been going into the platform which I think is great. But still there are some heaving patching todo,as the Symbian platform is in deed more sensitive to alignment problems compares to the Pocket PC version.
So have you grabbed the latest 0.7.1 version from Sprawls/Andreas homepage where I have made some improvements, as recommended by Max!
comment:13 by , 19 years ago
*printf.. On UIQ it brings up a textconsole on top of the screen, even as a different process, this does n't look very good. The best way for scummvm would be to use defines, so different platforms could call different functions *scalers - Well on a 320x208 platform, there is no reason to have any other scalers except the 1x in there. There is no current way to disable the others except the 1x scaler or? * RandomSource::RandomSource The time(0) is crashing the Symbian emulator when called. Why I dont know.. it just happens.. Works fine on target * There are stackproblems in ScummVM at least for the Wins platform, so I have removed as many as I can. * MP3/Movie support. A great deal of patching has gone into this, as Symbian can't share filehandles between different threads. Some of this was fixed for 0.7.1 but still some patching was needed. * Fix the alignment problems in Beneath a steel sky. I know it can be made a lot cleaner, and I am not sure if the game is completable in the current version. * Same for Queen ...
comment:14 by , 19 years ago
Also notice the removal of <TEMPLATE> on some of the destructors. They just dont compile with the version of GCC used for Symbian OS right now.
Also.. A note on a port to S60. One reason it do work as well as it does on UIQ is that all current devices allow direct screen memory writes. S60 does not support this, and as bitmap data used for S60 is not sharable between threads, it would probably crash for all those games animating under a timerdriven thread. (Beneath a steel sky does so in the emulator version, where a second bitmap is used for drawing).
And the MACRO in Beneath a steel sky was used to handle data alignment. Somehow all scumm games have been given a much better alignment handling than other additional games.
comment:15 by , 19 years ago
And should n't the SDL source be separated from the SCUMMVM source!? Two different diffs I guess!
comment:16 by , 19 years ago
And ZLib is mostly already implemented in the Symbian OS ver 7, the whole lib is NOT needed!
comment:17 by , 19 years ago
SO rnc_deco.cpp indeed needs modification, but I guess adopting it the proper way is the way to go. (Current diff ver 3 is not enough. Will lockup/crash on the device)
comment:18 by , 19 years ago
Also the Symbian BSearch is not working very well, so I replaced it with a copy, i.e the bsearch2 implementation.
comment:19 by , 19 years ago
* I'm set up to do dev for my P900 now, so development has resumed. I'm in contact with Lars & we are figuring things out :P I hope to be able to release Patch.v4 in the near future. * Solved the whole printf to console problem by redirecting stdout & stderr to files. This also gives more info in case of problems. I'm removing other printf-related edits. * Safe to say I'm very happy with the new DISABLE_ macros, as newer HE & Scumm CVS code was grinding the ancient GCC version the UIQ framework uses to a halt. * The scaling / huge font problem has been solved too. _force1xOverlay = true; in gameDetector.cpp does wonders for Symbian :). Removed the silly font changes.
saludos, SumthinWicked
comment:20 by , 19 years ago
you always forget stuff you've done: * Wrote a Perl script that converts the list of .o object files in the module.mk files to the incredibly dense UIQ MMP makefile system. It automatically adds and disables the correct source files in the different .mmp files. This way it's easier to keep track of new/modified source files. * Added Gob project file & lib: it compiled on the first run :-) Now hope that the current version will start running games again... I just got it up to running the chooser & config dialogs again without crashing today... I think/hope Lars will be a big help in this department ... hehe
/wicked out.
comment:21 by , 19 years ago
Glad to hear your work is progressing.
BTW, if you are able to split the patch into small independant parts, then it'll be easier to commit it, step by step. E.g. if you need to make changes to particular engines, or the GUI code, etc., then you may want to add a seperate patch for these, as they should be independant of your backend code. Likewise for any changes which might be required in the SDL backend.
comment:22 by , 19 years ago
Just a thought about the mp3 integration. Is it possible to make the design so that the filehandles is always opened in the mp3 thread and not in the main thread, which then is read by the mp3/sound thread. The current symbian patch is working, but not the prettiest in the world.
comment:23 by , 19 years ago
Hm, that might be possible, but it would be quite tricky. We'd have to pass the filename and an offset to the audio playback class, and then it would have to be recoded to only open the file and read data from it the first time its sound callback is invoked. That's going to be quite dirty, I am afraid :-/
comment:24 by , 19 years ago
Yes it is definatly not the best solution for all platforms. I am thinking about adding a separate mp3 implementation just for those with this special need, and thus not messing up the current mp3 implementation.
comment:25 by , 19 years ago
| Owner: | changed from to |
|---|
comment:26 by , 19 years ago
It would be nice if we were able to start integrating this.
I would strongly recommend to not worry about the MP3 problem for now. First, let's get the basic port into CVS. Improvements can then be based on this.
Future work would e.g. include refactoring the SDL backend to make subclassing a bit easier; this would have to be coordinated with Arisme for the WinCE backend, of course.
comment:27 by , 19 years ago
I think that SomethinWicked is getting ready for a Patch nr 4. We have been adding support for S60, and restructuring the buildsystem so it will suite the different Symbian platforms. We have also been adopting SDL for S60 so it will cater for all of ScummVM needs. Also Joystick handling has to be improved for SDL, so Sumthin will try to improve that. I am looking into some alignment problems for Gob, found a couple of bugs which stopped it from working, now we have a couple of strange sprites.
comment:28 by , 19 years ago
OK. However, please keep in mind: It is preferable to perform changes in small independent sets. So, if you have to modify the SDL backend, it would be good if that was applied to CVS independently of all the other changes. Else, if anything breaks, we'll have a much harder time to track down the cause.
comment:29 by , 19 years ago
We are aware of that, and I think that we can make the submissions in small sets, i.e getting the symbian build structure in there, adding the primary changes to SDL, and other bugfixes. And as for Gob, I guess we are doing some of the alignment fixes, which the Gob team has n't been able to do yet. And luckily those are not in the way of hacks. :-) Btw. what assumptions and usage of "char" should be made?
I noticed that different platforms as different default of char beeing signed or unsigned signed, which lead to a bug in Gob.
comment:30 by , 19 years ago
char if not related to strings should be converted into either byte or int8. We've did that for gob in all places which we could spot but of course there could be some missing like you report.
And another thing Gob engine alignment/char fixes has nothing to do with Symbian port and should be submitted different. Or:
I had an impression that anotherguest is already in the team, right? So gob fixes should go directly into CVS. As well as this patch, no need for v4 just split not backends/symbian changes into smaller commits and dump whole thing in just like Fingolfin tells. Then do fixes. Just make sure you don't break anything. And come to #scummvm :)
comment:31 by , 19 years ago
Ohh yes. Well. that is true, I am already a member of the team, but since some of the changes are pretty.. well.. big.. the patches gives us a way for Fingolfin to see if the are good. As for Gob fixes, we will submit them, as we go along. As far I have made 2 changes, one is a GCC_PACK for a palette structure, and another is a compare for a -5.. where Symbian was returning a 251. And 251 was n't == -5. :-) So expect Gob fixes going in. Specially since it is one of Sumthinwickeds favorite games. :-)
comment:32 by , 19 years ago
Yo folks,
seems the patch went in w/o most people noticing, since the commit mailing list wasn't working for some reason. Glad to see this.
Now, I am currently reviewing the changes. One thing I noticed is that printf was changed in some places despite discussion taking place here. I am going to revert those changes for now; if printf causes troubles on Symbian which aren't solvable on a port specific base, we can discuss solving this properly; but just changing printf to debug(1,...) is not correct, I am afraid.
comment:33 by , 19 years ago
Hi! Well.. the reason for the debug solution is that you want to be able to route the print outs where ever you like, and debug gave you a way of doing this. I guess that we can override the printf implementation so it does n't end up in the wrong place. The problem for symbian is that a virtual text console is started (which is fullscreen btw) each time a default stdout or stderr is used. And its not very nice. But as I said, we can try to remove it by replacing printf with our own implementation. Also I have been working on the MP3 patch, so it works for symbian and multithread environment without makeing it too much different for the other ports (would be that the code would be to cluttered to read). As for smush support, I will make my own chunk.cpp as the patches in that is just to great to look nice. We are also looking at making the WINCE keymapping support a part of a more wide small screen, special device configuration.
ANd it seems like all the nightly, well at least *nix and windows is building nicely.
All comments are welcome and we will replace printf on a port basis instead.
comment:34 by , 19 years ago
I'd prefer a port based printf replacement, aye -- in particular, people will keeping using it in their engines, so you'll run into troubles with it anyway.
As for replacing chunk.cpp -- that file may undergo a rewrite soon anyway, Just so you know. When you say you replace it by your own file, I am not sure what exactly you mean; might be good to discuss this before commiting.
comment:35 by , 19 years ago
Ok.. then we are aligned then. :-) As for chunk.cpp well, I am changing the readfunctions to indicate if they are used from the timer thread or from the main thread. And then I have 2 filehandles to read from , depending on the thread. Just to get smush running. But chunk.cpp has already changed some from 0.7.1 to 0.8 so have to rethink some things.
Waiting for your cvs updates then. :-)
comment:36 by , 19 years ago
I have now submitted the MP3 patch to CVS. I think it is pretty clean eventhough it is patched in a number of places. But does n't redesign the class as such and should still be good to work with.
My biggest concern about the mp3 playing, is that ideally you should guarantee that the file is read from one only thread. As it is now it is opened and first read done by the main thread, then is additional reads done by the sound thread. What is the possibilities for the Sound thread it all along?
comment:37 by , 19 years ago
BTW I saw the last change to the Smush_Player. Great work, I will test to see if it works as expected later today. :-) Single thread filehandling is just what we where looking for. :-)
comment:38 by , 19 years ago
* Latest Smush_Player + chunk seems to work well with the FT demo I tested with. * Thinking about a generic SDL key-remapping in ScummVM. I.e map any event to any other event. For example X->F5. The easiest way I see to do that is to call a RemapEvent function after the pump event in SDL. This could also be used for WINCE key remapping or any other sutiable SDL platforms where extra keys is available but not mapped as "standard"keys.
comment:39 by , 19 years ago
Do we need to keep this tracker item open? The port is in CVS now, and discussion about it could easily go on on the mailing list. If you need to record / write down stuff, I propose you use the ScummVM wiki for that (I can make you accounts if you need 'em).
http://wiki.scummvm.org/
comment:40 by , 19 years ago
I think this could be closed too. Putting in pending state with resolution "Accepted".
comment:41 by , 19 years ago
| Status: | new → pending |
|---|
comment:42 by , 19 years ago
| Status: | pending → closed |
|---|
comment:43 by , 6 years ago
| Component: | → Port: Symbian |
|---|
Symbian port patch version 1