Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCUMM: Some graphics rendering cleanup #2382

Closed
wants to merge 11 commits into from
Closed

SCUMM: Some graphics rendering cleanup #2382

wants to merge 11 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 27, 2020

While working on the AmigaOS 3 port of ScummVM, I ran into issues
with a lot of spread out code and looparounds, making it difficult to
add any optimizations that would allow the m68k processors in these
machines to run things better, so I wanted to clean up at least a bit
of the main graphics rendering loop.

There are still quite a few different memcpy/rendering loops in the
other files (charset, actor, etc.), but I will continue to look at
these as I make progress with getting the AmigaOS 3 port back
in better shape.

This lowers CPU load in games that do a lot of blast object/text
rendering, such as Curse of Monkey Island (naturally) and the
V6/V7 games by avoiding data copying in favor of just erasing dirty
rects from layers directly. The simple two layer rendering for
games like INDY4 may be a tiny bit slower than before, but not to
any noticeable extent even on an m68k processor.

NOTE:
I have not tested all SCUMM/HE games with these changes, but I did
test most of the ones I own to a reasonable extent to make sure that
everything seemed to be working correctly compared to before these
changes were made.)

Bjorn Astrom added 7 commits July 24, 2020 16:37
Makes implementation of additional blit functions and allocation of
purpose-specific VirtScreens easier.
A generic masked blit function for blitting only the pixels not matching
the mask color to the destination buffer.
Consolidate the consortium of function-specific code to use blit and
masked blit instead.
The _compositeBuf allocated in the ScummEngine constructor was
never actually used for anything, and could instead sometimes
result in a null pointer free(). Added a larger _compositeBuf
for V8 layered rendering.
This commit is a bit unwieldly, since it required editing a lot
of rendering functions with previously hardcoded destination
surfaces in order to be able to render to and erase from
specific layers. Does not seem to impact performance negatively
in any game, lowers CPU load in CoMI by a relative lot, since
it avoids a ton of data copying.
@Mataniko
Copy link
Contributor

Very cool, do you have some metrics around impact to CPU usage for these changes?

@ghost
Copy link
Author

ghost commented Jul 27, 2020

I don't have any precise metrics, unfortunately. The V6/V7 games all hovered around 0% CPU usage on my main PC, any difference was too small for even clock() (microseconds) to measure it. CoMI dropped in CPU usage from spikes to ~3% total CPU usage to around 1%. There may or may not be a very minor negative performance impact on pre-V6 games, but if there is, it's not enough to be noticeable even on a 50MHz CPU. The V6+ games all ran better when I tested them on my Amiga 4000 compared to before, CoMI showing (in a relative sense, of course) amazing improvements in frame times overall.

Bjorn Astrom added 3 commits July 28, 2020 00:20
I had managed to nuke the Day of the Tentacle text/verb virtual
screens, so the inventory and status line were invisible.
Noticed while testing on Amiga that the virtual screens were still
being reallocated, apparently I forgot to actually set the byte
size of an initialized virtual screen when creating it.
According to the description, restoreCharSetBg would only restore the
pixels that had been affected by any displayed text, but instead it
cleared and redrew the entire screen. Many thanks to mheyer32 for
noticing that something strange was up with this.
Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I browsed through your changes for the obvious things and code formatting issues.

Will perform a deeper analysis later.

engines/scumm/actor.cpp Outdated Show resolved Hide resolved
engines/scumm/gfx.cpp Outdated Show resolved Hide resolved
engines/scumm/gfx.cpp Outdated Show resolved Hide resolved
engines/scumm/gfx.cpp Show resolved Hide resolved
engines/scumm/gfx.cpp Outdated Show resolved Hide resolved
engines/scumm/gfx.h Outdated Show resolved Hide resolved
engines/scumm/gfx.h Outdated Show resolved Hide resolved
@ghost ghost closed this May 2, 2021
@ghost ghost deleted the scumm-engine-cleanup branch May 2, 2021 13:39
@sev-
Copy link
Member

sev- commented May 2, 2021

Any reason why did you close this?

@ghost
Copy link
Author

ghost commented May 2, 2021

Uhh, I was cleaning up some of my old github stuff earlier, and figured that since there had been no activity here for months that it wasn't interesting for the main ScummVM branch. The changes themselves are still available in my scummvm-amigaos3 branch.

@samo79
Copy link

samo79 commented May 3, 2021

@beeanyew

I believe it will worth merging as your modifications could also be useful for those who use ScummVM on slow computers running OS4 and MOS

@ghost
Copy link
Author

ghost commented May 3, 2021

Hm. It does sound rather unlikely, considering that even the slowest PPC running MorphOS or AmigaOS 4 should be multiples of tens times the speed of say a 50MHz 68030, or even a 50MHz 68060 with a massively higher memory bandwidth. If they're having issues updating a 320x200 screen, the problems are probably somewhere else in their Platform implementation.

@samo79
Copy link

samo79 commented May 3, 2021

Yep maybe, however there are multiple reasons to have that changes included also in main repository .. also for others who eventually in the future would want to continue your port on AmigaOS 3.x

@ghost
Copy link
Author

ghost commented May 3, 2021

I haven't dropped the AmigaOS 3.0 port at all, which is why I'm saying that the changes are still in that repo on my github. All I've said is that the AmigaOS 3.0 port can't be made up to date with the main branch anymore due to the use of features such as UTF-32 strings and engine dependencies on libraries that are either nearly impossible to build or do not perform to any reasonable degree on this operating system from 1992. Just grab my code and make it your PR, I don't want credit for it or anything.

@samo79
Copy link

samo79 commented May 3, 2021

Well, if from the analysis of the code no problems will emerged (or any other changes that need to be made) I think we could already do the merge ..
I can test these later on AmigaOS4 for eventual regressions/issues

@sev-

What do you think, can you merge the current code ?

@sev-
Copy link
Member

sev- commented May 3, 2021

Yes, I think, the code is worth merging.

@samo79
Copy link

samo79 commented May 8, 2021

@sev-

Any news ? :-)

@sev-
Copy link
Member

sev- commented May 8, 2021

@samo79 what news? @beeanyew needs to restore his branch...

@ghost
Copy link
Author

ghost commented May 8, 2021

Just to make this clear, there's no branch to restore. I removed the scummvm repo fork from my github account because I have no interest in the main ScummVM branch. All the code I changed is still available in the commits displayed on the page for this pull request, and the changes are also in my amigaos3 repo over at https://github.com/beeanyew/scummvm-amigaos3/tree/zz9k-stuff

Like I said, I want no credit for this, or anything to do with it. Take my code and make it your own PR. It's a bit of work, sure, but not anything impossible.

@samo79
Copy link

samo79 commented May 8, 2021

@sev-

I meant if you had the time to merge these changes into the main repo, of course once you have verified the correctness of the changes.. eventually once merged i'm availible to test these under AmigaOS4

@beeanyew

Among classic Amiga of today there is also the Vampire with cpu 68080 ..

@ghost
Copy link
Author

ghost commented May 8, 2021

Please stop tagging me. I know of the Vampire accelerator boards, and they don't need any of these changes either, because they're essentially 4x the speed of a 50MHz 68060 with an infinitely faster memory interface for their RTG.

@sev-
Copy link
Member

sev- commented May 11, 2021

Okay, the topic is closed then.

@samo79
Copy link

samo79 commented Jun 28, 2021

@sev-

Ok the topic was closed but the code was not merged yet in main branch, can you do that when you have the time?
Thanks

@Mataniko
Copy link
Contributor

@samo79 There's nothing to merge as the changes require additional work, someone will have to pick it up and submit a new PR before this can be considered.

@samo790 samo790 mentioned this pull request Feb 7, 2023
@mikrosk
Copy link
Contributor

mikrosk commented Feb 7, 2023

@beeanyew As I'm working on something similar (Atari Falcon 060@66 MHz), @samo79 was kind enough to point me to your PR.

All I've said is that the AmigaOS 3.0 port can't be made up to date with the main branch anymore due to the use of features such as UTF-32 strings and engine dependencies on libraries that are either nearly impossible to build or do not perform to any reasonable degree on this operating system from 1992.

Maybe this was true when you were working on this PR but from my experience, basically everything is possible to switch off. I'm linking my scummvm only with zlib, nothing else, out of the box.

the changes are also in my amigaos3 repo over at https://github.com/beeanyew/scummvm-amigaos3/tree/zz9k-stuff

Unfortunately, it seems you've removed the whole repository at some point. Any chance of bringing it back? I don't see your scummvm flavour even on aminet. :-(

@ghost
Copy link
Author

ghost commented Feb 7, 2023 via email

@digitall
Copy link
Member

digitall commented Feb 7, 2023

@mikrosk : There are several clone / trees available on Github for scummvm-amigaos3.

The first one is based on v1.8.1 release and was last updated in 2016:
https://github.com/mntmn/scummvm-amigaos3

The second one is more up to date, based on v2.0.0 release:
https://github.com/AmigaPorts/scummvm-amigaos3

The most up to date one I could find is based on v2.2.0 release:
https://github.com/betajaen/scummvm-amigaos3

Hope that helps.

@samo790
Copy link

samo790 commented Feb 7, 2023

Versions 1.8.1 and 2.0.0 for AmigaOS 3 68k were lastly updated in 2016 and 2020, so for sure they cannot include the changes of this PR that beeanyew is looking for...

The most updated fork for AmigaOS 3 68k are the one of betajean based on the 2.2.0 release.
However even this version doesn't include any of the optimization changes done by beeanyew in this PR

Aniway even if the repo was shutten down, i suggest to pick the code directly from here, and then eventually do a manually merge:
https://github.com/scummvm/scummvm/pull/2382/commits

@mikrosk
Copy link
Contributor

mikrosk commented Feb 8, 2023

Okay, I've managed to rebase beeanyew's code. It goes cleanly for 2.2.0 (that can be done with zero effort), relatively effortless for 2.5 and with some sweat on the later versions. As soon as I manage to finish the master branch for 100%, I'll test it and try to measure some numbers (I use COMI as one of my test cases so I should see some improvement). After that, I can proudly presents someone else's code as a new PR. ;)

@samo790
Copy link

samo790 commented Feb 9, 2023

@mikrosk
That's a very good news, keep it up! :-)

@mikrosk
Copy link
Contributor

mikrosk commented Feb 10, 2023

Actually, in the end I took the opposite route - to be sure that the fix/patch is really working, I backported my Atari changes to branch-2-2, with and without this PR.

And I have bad news -- I simply wasn't able to reproduce any significant speedup on Atari Falcon 060@66 MHz. I even tried to disable the whole rendering part (using the null renderer), no difference either.

I've been in touch with beeanyew, he could remember some explicit cases of huge speedup he'd seen (e.g. the intro credits in Full Throttle) but to no avail. There can be various reasons for this:

  • beeanyew forgot to publish some crucial part of his code to enable this behaviour
  • somewhere between his code and 2.2 release someone else added some code which either disabled this behaviour and/or optimised rendering to the point the fixes were not really needed
  • his hardware (68030@50 MHz + some RTG card) was way more sensible to even slightest optimisations
  • Atari Falcon's hardware is so much faster that even if the optimisations are there and working, it's not noticeable
  • I did something stupid ;)

So I'm leaving this for some fellow Amiga coder. Fetching & creating this PR's branch is easy: git fetch origin pull/2382/head:pr-2382 and as I said earlier, it is quite easy to rebase it on branch-2-2 up to branch-2-5 (after that some functions from this PR started to be deprecated and/or removed).

@samo790
Copy link

samo790 commented Feb 10, 2023

68060 are maybe powerfull enough to spot any differences, eventually you may try asking someone in the Atari community to try this executable with a slower 68020/68030/68040 processors

@mikrosk
Copy link
Contributor

mikrosk commented Feb 14, 2023

@samo79 let's discuss the question of merging here. As mentioned, it's not so easy. For 2.2 - 2.5, sure, it is but when it comes to master, I couldn't even vouch for this code to be doing something.

And as you have seen in the Atari PR, guys here can be pretty thorough when it comes to reviews, so I'd feel pretty uncomfortable defending the patch, especially if parts of it will have to be removed due to a different way how master solves the problem in the present. Not to mention my inability to reproduce the issue in the first place.

So the best I can do for you is to offer a rebased branch-2-5 with this patch and someone with the Amiga toolchain can compile and run it to see whether is some difference.

As for the 68030 CPU, I do have a Falcon with 50 MHz 030 and Fast RAM but it is disassembled now. What could be a nice push why to assemble it again, indeed. ;)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants