#9152 closed patch
PSP: refactoring and 16 bit support
| Reported by: | bluddy | Owned by: | joostp |
|---|---|---|---|
| Priority: | normal | Component: | Port: PSP |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
I broke the OsystemPSP class apart into several classes, refactoring it and cleaning up the code.
Practical enhancements to the port: - 16 bit support. I had to implement memcopy to enable swapping red an blue channels for some 16 bit games such as HE. - Blurriness is gone in the main menu (unscaled and scaled images need to be handled differently when rendering) - Cursor looks cleaner with blending than with alpha masking. - Dirty implementation for all DisplayClients makes code more efficient -- we only draw if there's been a change on the screen. - Went back to storing the Overlay and Screen in hardware memory, but did it properly: I removed the depth buffer, did proper allocation, and have backup allocation in main memory in case we run out of hardware memory. - I think there's also some other speed improvement compared to the old code, but I'm not sure why.
Layout of the new code: - OsystemPSP now contains very little of the logic. - GuRenderer class and MasterGuRenderer contain all calls to the graphics hardware. - All surfaces (Screen, Overlay, Cursor, PSPKeyboard) were rebuilt to inherit the DisplayClient class. DisplayClients use one or more Buffers, Palettes and GuRenderers. - Trace system has been refined and added to almost all PSP files. We can now fully trace almost all function calls (and localize it to individual functions/classes). Tracing can be done to shell, to a file, or both. - Virtual functions have been avoided to keep performance high. (we still get them at the entrance to OsystemPSP obviously) - PSPKeyboard code was factored out (especially the complex state machine)
Ticket imported from: #2974509. Ticket imported from: patches/1257.
Attachments (4)
Change History (23)
by , 15 years ago
| Attachment: | cleanup.diff added |
|---|
comment:1 by , 15 years ago
Minor update with correction of '\r' instead of '\n' in keyboard. '\n' doesn't work in SCI games. Also, hack to make the virtual keyboard appear when working from the shell was turned off. Newlines added to the end of some files.
comment:2 by , 15 years ago
| Owner: | set to |
|---|
comment:3 by , 15 years ago
Wow, seems you've been busy! :) Thanks for this patch.
Due to its massive size it'll take me some time to review. I'll try to get around to it this week.
comment:4 by , 15 years ago
Sure no problem.
Yeah, it's taken me a while to get this one done. The factor that helped speed it up (so that I was able to finish it in a reasonable period given my limited free time) was that there wasn't too much new functionality, and the logic is pretty simple so there weren't any crazy bugs.
comment:5 by , 15 years ago
Looks like nice work! Still some general remarks:
For future submissions like this, I recommend to develop more incrementally (this is quite easy when using git-svn). I.e. complete one change, then make a local commit using git-svn. Develop the next change, etc. Try to keep them as separate as possible. This is a little bit more work during development, but it makes it *much* easier to review the change in several steps, and also easier to debug it when regressions turn up: Instead of saying "Oh this huge blob of changes caused it", you can then pinpoint which part of the changes are responsible.
And using "git rebase -i" you can even re-arrange the sequence in which you make changes, cover up embarrassing in-between brainos, etc., :)
BTW, the name "cleanup.diff" is quite misleading ;).
comment:6 by , 15 years ago
BTW, instead of the "implicitly relaying on an object to implement getObjectName()" hack , you might want to use __PRETTY_FUNCTION__ instead.
comment:7 by , 15 years ago
Hmm I need to look into git-svn then. I have a more primitive method of work which involves making ver1/2/3... folders on my HD where I copy all the modified files after every change I make. I zip it and email it to myself, and test it for regressions. Didn't want to disturb the main tree though until I had (fairly) thoroughly tested my changes, especially since debugging user issues on the PSP can be a real pain.
__PRETTY_FUNCTION__ does that? That's awesome! I knew there had to be something like that...
comment:8 by , 15 years ago
Well, __PRETTY_FUNCTION__ doesn't *quite* do what you want, it prints to much, it'll generate output like "void MyClass::myMethod(void,int)" i.e. it prints the full signature, as only that is guaranteed to uniquely identify a method / function. But I don't think that hurts; and if one really dislikes it, one could modify the underlying function to parse away the return type and arguments list (that would be quite simple to do, in fact). And you could merge PSP_ERROR and PSP_ERROR_STATIC this way :)
Reading what you wrote on "ver1/2/3... folders" etc., I *strongly* recommend that you try git-svn. It can be used to replace exactly the kind of workflow you describe, and besides making it much easier to submit an incremental set of patches, I am sure it'll also make your workflow much, much easier. Like, a thousand times or so ;).
comment:9 by , 15 years ago
I've just had a quick look at the patch. I'll try and have a more in-depth one tomorrow.
Codewise I don't have any serious remarks (yet). The getObjectName() thing was one that stood out to me, but I see it's already been discussed. I very much like the new cleanness of OSystem_PSP. Good job!
Now for some remarks about the user experience. I've only tested a few games so far (SCUMM, AGI, gob and sky):
* The new analog speed takes some getting used to. It's a bit harder now to move the cursor slowly without resorting to the modifer button. I'm not sure if I like it yet, but I suspect it'll grow on me / takes some getting used to.
* Things appear noticeably snappier, though I suppose the faster cursor also affects the perception of this. :)
* Not noticed any regressions or issues, except for: In BASS the cursor is now transparant. Seems a key colour issue/bug.
comment:10 by , 15 years ago
Glad you like the patch so far!
Regarding cursor support: I originally wanted to really fix up the cursor so that it's very slow and responsive when you move the nub a tiny bit, and can move fast when you move the nub all the way, since my experience was that when I wanted to move to the other side of the screen, the response was too slow. Unfortunately what I found out was that the nub responsiveness on the PSP is pretty crappy. It maxes out way too easily to 255 (really 128 when you recenter it around 0). That means that there aren't enough gradations to make the 'full' nub push speed across the screen, since the full nub push is reached at what feels like 65% of nub movement. As a result, I pretty much left the movement as it was, except I scaled it. ie. if my width is 320 and my height is 200, it's roughly a 1.5 ratio, so I scaled x movements by 1.5. Also, before, only 640x480 was scaled to be faster than 320x200, so the launcher, which runs at 480x272, felt very sluggish. I scale for the launcher as well.
At least these are SUPPOSED to be the only changes to the original nub formula. If you find that the code is doing something else, let me know.
- Regarding the mouse cursor, I recently found this problem in Teen Agent, but only when switching from the menu to the game screen. I think it may have to do with something like getting a new cursor (or screen) palette, which causes the key color to be cleared. This is followed by a new key color, so now 2 key colors will have been cleared. The general solution I can think of is to save the key color when we clear it and when the key color is changed, to restore the old key color if it's 0. It'll make the most sense for this functionality to be in the Palette class.
Unfortunately I don't know if I'll get a chance to test this before I get back from my trip (which I'm currently on). This should be in the 2nd week of April.
comment:11 by , 15 years ago
Uploaded new version (3)
- Fixed cursor keycolor issue (hopefully). New implementation should allow us to have only one palette for the screen and one for the cursor, but I'll leave that to do after the patch is committed. - Fixed issue with memcopy that caused stray pixels in some cursors. - Changed to support PRETTY_FUNCTION - Figured out the mouse speed issue: switch to Peek instead of Read on control sampling causes faster reads. Before we had 40ms + Read sleeps the thread waiting for vsync. Peek doesn't sleep the thread, but we have to make up for the lost sleep time, so I tried to change the read time to make the speed similar to what we had before.
comment:12 by , 15 years ago
Updated with a very minor fix (version 3.5). Keyboard was getting stuck in moving mode in some situations.
comment:13 by , 15 years ago
Aside from some code formatting inconsistencies - you may want to run astyle over it (settings are documented in the wiki), inline vs. INLINE, etc - and some copy/paste comment errors (e.g. setRendererModePalettized(false); // use palettized mechanism), the patch looks good to me.
The BASS cursor bug is indeed resolved too. I haven't found any new issues, so it's good to commit and get some more widespread testing, as far as I'm concerned.
Given the work you've done, you should credit yourself for the PSP backend (and remove the contributors entry)
comment:14 by , 15 years ago
Committed with the inlines changed but not the code formatting. The astyle settings in the wiki don't seem to be right -- I end up with if(xxx) with no spaces for example. Therefore I preferred to commit rather than play with astyle, and do the style fixes later.
comment:15 by , 15 years ago
Odd, these astyle settings work just fine over here, with astyle 1.23. Maybe you are using an older / newer version?
comment:16 by , 15 years ago
Any luck with astyle? The patch seems like a serious step forward with PSP port, and it would be a pity of this get rot.
comment:17 by , 15 years ago
Didn't realize this one was still floating open. Yep, astyle's good, and the patch has been incorporated.
comment:18 by , 15 years ago
| Status: | new → closed |
|---|
comment:19 by , 6 years ago
| Component: | → Port: PSP |
|---|
PSP Cleanup