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

SCI32: Implement kRobot #805

Closed
wants to merge 1 commit into from
Closed

Conversation

csnover
Copy link
Member

@csnover csnover commented Aug 12, 2016

Not much else to say about this one. Makes a bunch of stuff work. Many many thanks to @wjp for helping figure out the craziest details of the audio format.

@sev-
Copy link
Member

sev- commented Aug 12, 2016

Wow. Just wow.

@csnover
Copy link
Member Author

csnover commented Aug 12, 2016

@sev-, yeah, that is a very familiar emotion, having spent like a month on this one feature, peeling away layer after layer of amazing design decisions. :)

@@ -31,6 +31,7 @@ namespace Sci {

enum ScaleSignals32 {
kScaleSignalNone = 0,
// TODO: rename to 'manual'
Copy link
Member

Choose a reason for hiding this comment

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

Does that refer to kScaleSignalNone?

@bluegr
Copy link
Member

bluegr commented Aug 12, 2016

Excellent work, for a very weirdly designed functionality :)

I had done progress on this myself, but then I read on IRC that you were working on this too, so, since my free time is very limited these days, I did not progress afterwards.

Overall, this looks good, however it's merged into one gigantic commit, which makes it quite hard to follow. Could you please split this up into smaller commits?

@wjp
Copy link
Contributor

wjp commented Aug 13, 2016

Usually I'm all in favour of smaller commits, but since practically all of this commit is adding new, related functionality and I don't quite see which parts could be split up, one single commit makes sense to me here.

@wjp
Copy link
Contributor

wjp commented Aug 17, 2016

Amazing work on this. It's gone through several debugging passes already, and I think this can be be merged as-is.

@bluegr
Copy link
Member

bluegr commented Aug 17, 2016

What is the status of V4 Robot videos? IIRC, these were only used in the SWAT demo... which used to work fine with the previous code. Does that still work? There are a lot of V4-related TODOs.

Furthermore, for completeness sake, can a comment be added mentioning which games use V4, V5 and V6 robot files?

I believe that it goes like this:

  • V4 robots: SWAT demo
  • V5 robots: KQ7, Phantasmagoria, SWAT
  • V6 robots: RAMA

IIRC, Lighthouse does not use Robot videos, right? Can the list above be double checked, and added as a comment?

Finally, I'd like a reply to my comment above (about kScaleSignalNone).

@bluegr
Copy link
Member

bluegr commented Aug 18, 2016

Sorry, Lighthouse does use robot videos. Thus, if I'm not mistaken, the list above is:

  • v4: SWAT demo
  • v5: KQ7, Phantasmagoria, SWAT
  • v6: RAMA, Lighthouse

@csnover
Copy link
Member Author

csnover commented Aug 19, 2016

Landed at 0f2748b.

@csnover csnover closed this Aug 19, 2016
@bluegr
Copy link
Member

bluegr commented Aug 19, 2016

And what about my questions and request above?

@csnover csnover deleted the sci32-kRobot branch January 8, 2017 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants