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

KYRA: More adlib driver code cleanup #2728

Merged
merged 7 commits into from Jan 15, 2021
Merged

Conversation

@miller-alex
Copy link
Contributor

@miller-alex miller-alex commented Jan 12, 2021

Again some improvements to the code that shouldn't change behaviour in any way.

If you don't like that I'm changing the way the callback functions work, you may drop the last commit, but I think it's cleaner like that.

miller-alex added 7 commits Dec 22, 2020
Free the sound data in SoundPC_v1::internalLoadFile() since it's owned
by this class instead of in the driver's setSoundData() method.
Call the opcode functions with a pointer to the parameters instead
of data poiner reference and first parameter. The data pointer is
updated in AdLibDriver::executePrograms() before calling the opcode
function; an opcode can change channel.dataptr if (and only if) it
wants to jump (and is responsible not to change it inadvertently).
@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Jan 12, 2021

DeepCode's analysis on #db5d96 found:

  • ℹ️ 1 minor issue. 👇

Top issues

Description Example fixes
Comparison of integral operand 0 and boolean operand notequals. Consider making both sides the same type, or check the expression by comparing to true/false/TRUE/FALSE or use if (x), if (!x). Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@miller-alex
Copy link
Contributor Author

@miller-alex miller-alex commented Jan 12, 2021

Additional note/question:

I've noticed an inconsistency in the encoding of addresses in the sound programs. There are two diffent encodings: in EoB addresses have an offset of 191, newer games use relative addresses. But the four callbacks with addresses as parameters use different conditions: update_checkRepeat() uses always relative addresses, update_jump() uses Eob style for version < 2, update_jumpToSubroutine() uses EoB style for version < 3, and update_setupSecondaryEffect1() always uses EoB style addresses. I don't know whether this is an inconsistency in the original drivers, a bug, or doesn't matter since the features aren't used by the games where it would make a difference. But unless it needs to be the way it is, the same code should be used everywhere to avoid confusion.

Do you know whether this can/should be changed?

I also considered reordering struct Channel to reduce padding, but didn't. What do you think?

@miller-alex
Copy link
Contributor Author

@miller-alex miller-alex commented Jan 12, 2021

DeepCode seems to be deeply confused. The description of the issue is nonsense. Maybe it doesn't like line 755?

@athrxx
Copy link
Member

@athrxx athrxx commented Jan 12, 2021

Heya, thanks for your work on this. Please ignore the deepcode and codacy output, unless there is actually something valid in it. No need to make code changes just to silence the tools. The only thing you really want to check is the travis output for clang and gcc (there are no warnings from your code, so all is good).

From first glance I don't see anything I wouldn't be willing to merge. Maybe change the long type to int or int32, since we don't seem to use that type in Kyra, but that's not a real point of critique...

Regarding your question about the offsets: I would like to think that we got it right the way it is, since we went over the original driver disasms often enough. But honestly, I'll just have to take another look to be sure. I'll get back to you when I have your answer.

Changing the channel struct layout isn't really necessary, unless you really want to do it and the code layout doesn't suffer from it.

@athrxx
Copy link
Member

@athrxx athrxx commented Jan 13, 2021

So, about the offsets: the absolute offsets in the original drivers are inconsistent, as they are not in match with the data provided by the sequencer (except maybe EOB1? - I remember fixing the teleporter sounds in EOB2, but EOB1 might have been correct).
From the data files (or from EOB1? can't remember) I managed to figure out that the sequencer must have based the offsets on 191.

This is what I found in the disasms:

  • secondaryEffect1(): all driver versions resolve the address via xlat so it will be an absolute offset from the ds (=current) segment.
    --> so this one seems to be okay (or at least can't reasonably be fixed).

  • update_checkRepeat()/update_jump(): only the EOB 1 version driver reads an absolute offset (in current segment), all other versions read a relative offset (EOB has 'mov si, ax', the others have 'add si, ax'
    --> so this is correct (this would definitely have caused issues if it weren't)

  • update_jumpToSubroutine(): EOB 1 and 2 version drivers use absolute offsets (in current segment), the later versions read a relative offset (EOB 1 + 2 have 'mov si, ax', the others have 'add si, ax'
    --> so this is correct (this would also have caused issues if it weren't)

Apparently Westwood changed these offsets to relative over time whenever they spotted a particular bug.
I believe that the xlat in secondaryEffect1() for the later versions is an oversight. I have run all Kyra 1 and 2 sound effects to see whether any one of these triggers the secondaryEffect1. Just played through all possible track numbers from all files. Not one single secondaryEffect1().

Still, I see no way to fix it, unless it becomes evident that there is actual corresponding data which is also correspondingly fixed.

@miller-alex
Copy link
Contributor Author

@miller-alex miller-alex commented Jan 13, 2021

Thank you for taking the time to check the situation in the disasms. Well, since the code can't be made more consistent, I won't change it, of course.

@athrxx
Copy link
Member

@athrxx athrxx commented Jan 15, 2021

Is this ready to be merged? I have done a few quick tests with the relevant games. Seems like everything still works... ;-)

@miller-alex
Copy link
Contributor Author

@miller-alex miller-alex commented Jan 15, 2021

Yes, I think it's ready.

@athrxx athrxx merged commit 3fb4d67 into scummvm:master Jan 15, 2021
2 of 3 checks passed
2 of 3 checks passed
deepcode-ci-bot Found 1 minor issue.
Details
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miller-alex miller-alex deleted the miller-alex:kyra-adlib-3 branch Jan 15, 2021
miller-alex added a commit to adplug/adplug that referenced this pull request Jan 16, 2021
* Move delete[] of sound data from AdLibDriver::setSoundData()
  to CadlPlayer::load() since the latter class owns the data.
* Add helpers to reduce code duplication in handling of timer
  wraparound and _soundData offset checks.
* Improve a comment on instruments table.
* Reduce nesting level in AdLibDriver::executePrograms().
* Simplify struct clearing (layout independent) in initChannel().
* Change calling convention of opcode callbacks: data pointer is
  no longer a reference but channel.dataptr is updated before
  calling; drop last argument with the first data value.

These changes have been merged in ScummVM.

Link: scummvm/scummvm#2728
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.