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

SCI: Update Sound:vol in processUpdateCues #1536

Merged
merged 1 commit into from Aug 12, 2019

Conversation

@sluicebox
Copy link
Member

commented Mar 12, 2019

Sound:vol is updated when updating cues starting in SCI_VERSION_1_MIDDLE.

Fixes bug #10244. SQ4 localized floppy versions depend on this when getting
in the orange ship, they fade music and wait for the volume to reach zero.

Confirmed against asm that this is not in SQ4 floppy (early) but is in
LSL1VGA (middle) and SQ4 localized floppies and SQ4CD (late).

SCI: Update Sound:vol in processUpdateCues
Sound:vol is updated when updating cues starting in SCI_VERSION_1_MIDDLE.

Fixes bug #10244. SQ4 localized floppy versions depend on this when getting
in the orange ship, they fade music and wait for the volume to reach zero.

Confirmed against asm that this is not in SQ4 floppy (early) but is in
LSL1VGA (middle) and SQ4 localized floppies and SQ4CD (late).

@digitall digitall requested review from bluegr and m-kiewitz Mar 12, 2019

@m-kiewitz

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Please don't commit yet, I have to verify various interpreters to make sure that everything checks out.

@bluegr

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Any news on this?

@m-kiewitz

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Any news on this?

Still on it.
Very busy at work, had to work through one whole weekend this month already :/

@Kawa-oneechan

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Hang in there!

@bluegr

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Ping: Any news on this?

@m-kiewitz

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

Ping: Any news on this?

Not yet. Sorry.

@sluicebox

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2019

If it helps, the update cues function can be quickly found in most DOS SCI1 interpreters by the unique instruction:

05 7f 00 add ax, $7f

and in mac interpreters:

D0 7C 00 7F add.w #$7F,d0

The function ends with three consecutive calls to the set selector function prior to the change, afterwards there's a fourth for volume.

@sluicebox

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

I'm going to merge this next week unless there's a reason not to.

  • We already do this for SCI32 a few lines above in processUpdateCues()
  • I'm seeing this in every interpreter I inspect
  • Depressingly, I just saw that csnover wrote this same fix in 2017 minus the version guard: csnover@00be923

That last one is so depressing it's a wonder I don't click merge right now.

@m-kiewitz

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

* I'm seeing this in every interpreter I inspect

My point is making sure that it's in every single SCI1 MIDDLE interpreter (Leusire Suit Larry 5 Demo, Conquests of the Longbow Demo, Leusire Suit Larry VGA Remake (2.0+2.1+EGA), , Astro Chicken 2, King's Quest 5 Japanese English FM-Towns) and definitely not in any SCI1 EARLY interpreter (King's Quest 5 Floppy versions, Leisure Suit Larry Demo, Mixed-Up Fairy Tales EGA+VGA, XMAS Card 1990 EGA+VGA, Space Quest 4 EGA+1.052+1.1 Floppy, Mixed-Up Mother Goose CD and Mixed-Up Mother Goose FM-Towns).

If you have checked all of these, then it's okay to commit.
If you have checked some, please tell me which you have checked.
Sometimes a change wasn't done exactly during one of our SCI version splits, and thus such a change can cause an obscure error. That's what I want to avoid at all costs.

If for example the change was done for the last few games that we consider SCI1 EARLY, then we would have to check on top if the game is accessing that selector at any point and making sure that not setting it will cause no issues. Because if it would, then we would have to add that specific game id too for your fix.

That last one is so depressing it's a wonder I don't click merge right now.

I don't understand. What's do depressing about a fix written in 2017?

@m-kiewitz

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

@sluicebox
Can you just please tell me which of the games you checked, that I previously mentioned?

@sluicebox

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

I keep forgetting about this. Here are versions I've verified from ye olde notes:

Other than the Longbow demo, it cuts cleanly along the SCI_VERSION lines, and Longbow demo doesn't care either way since nothing in it depends on the vol property, which I tested.

NO		QFG2 1.105 (final)
NO		KQ5 0.000.051		SCI1 Early	
NO		Mixed-up Fairy Tales	SCI1 Early
NO		SQ4 1.052			SCI1 Early	
NO		XMAS1990			SCI1 Early	
NO		JONES Floppy			SCI1 Early	
YES		JONES CD			SCI1 Middle	
NO		LONGBOW DEMO		SCI1 Middle	
YES		LSL1VGA 2.0			SCI1 Middle	
YES		LSL1VGA 2.0 EGA		SCI1 Middle	
YES		LSL1VGA 2.1			SCI1 Middle	
YES		Ms Astro Chicken		SCI1 Middle	
YES		SQ4 Localized Floppy	SCI1 Late
YES		BRAIN DEMO			SCI1 Late	
YES		BRAIN 1.000			SCI1 Late	
YES		SQ4CD				SCI1 Late	

@sluicebox sluicebox merged commit 99a7478 into scummvm:master Aug 12, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.