Add new platform: Atari #4687
Add new platform: Atari #4687
Conversation
There was a problem hiding this comment.
I see several issues with this PR.
First, several commits do not pertain to this PR and should be made inside a specific PR to distinguish them.
Next, there is a faire share of ASM and I don't understand the reason behind some functions (all the native features stuff).
The C2P functions are not documented so it's not obvious that they convert to planar.
Finally, I am not sure if the project should accept another backend while the first one is still maintained.
And, if this backend would be added, I am almost sure it won't be backported to 2.7 as it's a really big change.
| @@ -33,6 +33,8 @@ | |||
| #define SAMPLES_PER_SEC 11025 | |||
| #elif defined(PLAYSTATION3) || defined(PSP2) || defined(NINTENDO_SWITCH) | |||
| #define SAMPLES_PER_SEC 48000 | |||
| #elif defined(__MINT__) | |||
There was a problem hiding this comment.
As this would apply to already existing port (as I suspect __MINT__ is defined here as well),this change should be checked by the SDL based port maintainer.
As you are making a port without SDL why is this changed?
There was a problem hiding this comment.
Yes, this is a fix for Atari/FreeMiNT. As I am more or less the official maintainer of the SDL port for Atari (see https://github.com/mikrosk/SDL-1.2) I can assure you that this has nothing to do with SDL itself but more with the hardware it is running on. 49170 Hz is a native frequency for our platform, 44100 leads to unnecessary resampling.
I've included it out of courtesy. Found it when trying to compile/run/profile ScummVM on Atari SDL.
If you really insist, I can create separate PRs for those one-liners but as I found them only when compiling on my platform, one could argue that they are somewhat related to this PR.
Up to you of course, just tell me beforehand so I don't waste time on unnecessary things for my own needs.
Is it? Except the platform-specific code it's literally a set of one-liners. Even if the port would be crashing in 99% of cases on my platform, it would still be 1% better than before because there is nothing to regress from (by the way, Also, I see a steady stream of commits into the 2.7 branch supporting Kolibri OS, with way more invasive changes to the common areas. |
| @@ -223,6 +223,9 @@ void initCommonGFX() { | |||
|
|
|||
| if (gameDomain->contains("shader")) | |||
| g_system->setShader(ConfMan.get("shader")); | |||
|
|
|||
| if (gameDomain->contains("gfx_mode")) | |||
There was a problem hiding this comment.
Could you double-check that this doesn't cause any issues in the SDL port when switching between the OpenGL and SurfaceSDL graphics managers?
We used to have two backends for ios. |
mikrosk
force-pushed
the
atari-master
branch
12 times, most recently
from
92106ef
to
f2f8225
Compare
|
@samo790 that is indeed very interesting, thanks for pointing that out. I've added it to my TODO list. |
|
@mikrosk While there is value in this PR and approach of having a port that is closer to the hardware than the current one, there is an obstacle which must be removed before proceeding further. The obstacle is not technical, and I am hoping it could be changed. In this relatively short-living PR you managed to counter already three developers of the ScummVM Team, and the very README starts with a long rant against the current port maintainer using derogatory language. In the ScummVM Team, one of the critical requirements is having respect for other devs' work and not going down to personalities. In the cases when the quality is not there, or there are other reasons to complain, we patiently educate each other and improve things together. The attitude expressed so far in this PR and in the sword1 detection bugreport is not on par with this approach. I am looking forward to your reaction on this matter before we proceed with further development and spending time reviewing and improving the code. |
|
@sev- I have no problem with changing anything you consider inappropriate. To give you some context, firstly I hadn't considered merging my changes in the official repository so I just made a, erhm, more freestyle readme explaining what motivated to start working on this port. It was never my intention to vilify anyone, I just wanted to describe issues I've found with the current code, engines etc. and perhaps I should have reviewed my tone in readme more carefully (tbh, I did not expect anyone except the Atari users even reading it :)) I myself have done numerous Atari ports using SDL, plus for the last couple of years I have been sort of maintaining Atari port of SDL so I'm the last person to look from above on someone's work using this framework. Plus I've been in touch with KeithS over the years, he's a known Atari porter as well. So you see, it's just perhaps my underestimation how my choice of words looks from the point of someone who doesn't know me or is outside of our community. As for the sword1 report, I tried to explain there basically the same thing. Just wanted to point out that it's not the piracy I'm advocating for but the fact that something worked and now it doesn't so I, as the scummvm user in that situation, have to solve a problem which didn't exist in 2.6.1. Perhaps again bad choice of words to articulate my argument. So to sum this up, as it seems I'm pretty terrible at communicating my concerns, just tell me what to change and I'll do it. ;-) |
|
So where we are standing these days? I spent some time with applying your suggestions (some of them are great ideas, btw!) and on #2382. In the meantime I've released the Atari version of 2.6.1 to the public to gather some feedback (but I assume there is no point of merging the changes into There's still a few review points open where I'd appreciate your feedback guys, all the other (straightforward) changes I'd applied I marked as resolved. If you have further concerns about |
mikrosk
force-pushed
the
atari-master
branch
5 times, most recently
from
7a88e0e
to
029bbc0
Compare
mikrosk
force-pushed
the
atari-master
branch
from
a07456b
to
2d47da4
Compare
|
I replied to a couple of things, and we are close to the merge. Though, it now has to be rebased as there are now conflicts. |
mikrosk
force-pushed
the
atari-master
branch
from
fad16e6
to
7eb6cd4
Compare
| @@ -60,6 +60,9 @@ dist-generic: $(EXECUTABLE) $(PLUGINS) | |||
| mkdir -p ./dist-generic/scummvm/data | |||
| mkdir -p ./dist-generic/scummvm/doc | |||
| cp $(EXECUTABLE) ./dist-generic/scummvm | |||
| ifeq ($(BACKEND), atari) | |||
| m68k-atari-mint-flags -S ./dist-generic/scummvm/$(EXECUTABLE) | |||
There was a problem hiding this comment.
You might want to create a separate packaging rule in a separate file so you have more freedom to customise things.
There was a problem hiding this comment.
I have been thinking about this, too. However when I found out that I don't really need to repack or rename anything, I was like ok, maybe you will let me slip this in. ;) (but it's true it would be nice to supply readme.txt in the installation process, maybe even separate data into data and themes). Will keep this in mind for my next PR(s).
| @@ -1420,6 +1421,10 @@ void ScummEngine::setupScumm(const Common::String &macResourceFile) { | |||
| } | |||
|
|
|||
| _res->setHeapThreshold(400000, maxHeapThreshold); | |||
| #else | |||
| // RAM is cheap, disk I/O isn't... helps with retaining the resources in COMI and similar | |||
There was a problem hiding this comment.
How much RAM would you normally expect to have in the average Atari system?
There was a problem hiding this comment.
Average Atari system is still out of reach for my work, unfortunately. The absolute minimum is set to a 32 MHz CPU (ideally 50 MHz) and 64 MB of so called Fast/TT RAM (this is easily available via addon cards).
However I have seen reports that some games are playable (with native MIDI instead of emulation) even on basic configs like 16 MHz without too much RAM (that was before this commit) so I might reconsider decreasing this huge cache; see my TODO: https://github.com/scummvm/scummvm/pull/4687/files#diff-7ce93d26e2f2ac33135484bf47a2063bf4bb1b3732195c40797f29098d13611fR398-R399
There was a problem hiding this comment.
Maybe in a separate PR we could add a new function to OSystem that wraps around SDL_GetSystemRAM and use that to decide how much memory should be allocated for the cache?
There was a problem hiding this comment.
Sounds good. I wonder how many engines have hard limits like those in scumm set. But maybe other engines don't have such a concept of loading/releasing resources, don't know...
| } | ||
|
|
||
| void AtariMixerManager::update() { | ||
| if (_audioSuspended && !_muted) { |
There was a problem hiding this comment.
I've opened an issue with some general ideas on how to optimise the software mixer, so we can revisit this later.
|
Rebased, applied & retested everything. In my next PR (which I start working on after this gets merged) will focus on the following features:
AFAICS there's about 10 unresolved discussions, I let them open intentionally for you to notice any showstoppers, otherwise feel free to resolve them. |
|
@ccawley2011 I don't see reason why not but before I do it, I just want to make sure: is there really a point of doing that at this stage? (when basically everything has been reviewed) I mean, from what I have seen you guys prefer to fast-forward PRs and I have made a lot of effort to separate my work in the way that this PR could be merged like that (i.e. please don't squash-merge it even if I do split those commits to separate PRs). I just want to avoid unnecessary work and even bigger delays (for instance 7795b24 must be merged before my changes otherwise my backend wont work properly) but if you want to discuss some things in that PRs then it of course makes sense to create them. |
IMHO commit 7795b24 is the only major blocker since it may have a negative effect on the SDL port, since switching between OpenGL and SurfaceSDL is quite fragile. Alternatively, you can wrap the change using |
|
A couple of extra points that might be interesting for future PRs:
|
mikrosk
force-pushed
the
atari-master
branch
from
7eb6cd4
to
1e07d53
Compare
|
One more thing to mention (possibly for a follow up PR) is that it may be worth increasing the file buffer size if file IO is a bottleneck, either with |
Compile as: ./configure --backend=atari --host=m68k-atari-mint --enable-release --disable-mt32emu --disable-lua --disable-nuked-opl --disable-16bit --disable-scalers --disable-translation --disable-eventrecorder --disable-tts --disable-bink --opengl-mode=none --enable-verbose-build --prefix=/usr --bindir=/ --datarootdir=share --datadir=data && make -j 16 && m68k-atari-mint-flags -S -r ./scummvm && make install DESTDIR=$PWD/_release/ && mv $PWD/_release/scummvm $PWD/_release/scummvm.ttp
- add NDEBUG for smaller file size and better performance when building with --enable-release - add exe extensions for both ATARI and FreeMiNT - use "dist-generic" instead of the clunky ./configure paths Build as: ./configure --backend=atari --host=m68k-atari-mint --enable-release --disable-mt32emu --disable-lua --disable-nuked-opl --disable-16bit --disable-scalers --disable-translation --disable-eventrecorder --disable-tts --disable-bink --opengl-mode=none --enable-verbose-build && make -j 16 && rm -rf dist-generic; make dist-generic
|
Rebased on top of |
|
Thank you! |
This is my attempt to include a new platform to the official ScummVM tree. It is closely related to another one which is already officially supported:
Atari/FreeMiNT. However this platform is not very well suitable for lower-end configurations and honestly, I don't like it at all. Some of the reasons are listed here:scummvm/backends/platform/atari/readme.txt
Lines 12 to 47 in 4c26e0f
As most of the code is backend, it shouldn't disrupt anything existing. I've included a few fixes (as separate commits) and even a fix for the
Atari/FreeMiNTbuild.I guess the most 'controversial' commits would be the last three, I'm happy to discuss them.
Also, I've had this change originally applied for
branch-2-6-1, I'm happy to backport this platform intobranch-2-7for the upcoming release if you like.P.S. I'm not sure what is the proper way to include a readme for given platform in the configure/makefile ecosystem, please advise.