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

New engine: Beavis and Butthead In Virtual Stupidity #435

Merged
merged 27 commits into from
Feb 21, 2014

Conversation

johndoe123
Copy link
Member

This pull request adds support for "Beavis and Butthead In Virtual Stupidity".

  • The game is completable
  • I didn't test it on a big-endian system
  • I haven't run it with Valgrind but with DrMemory which didn't give any worrysome warnings
  • The PSX version is not supported; it uses the exactly same scene files but graphics, sound and videos are all PlayStation-specific

One thing missing:
In the air guitar minigame, support for saving/loading of songs is currently not implemented. The original uses Windows' file dialogs. I didn't come up with a good idea how to reimplement this. I guess it would be ok if the files can only be saved in the savegame path and not in other directories, so maybe some kind of simpler saveload dialog could be used, since the songs don't have sceenshots etc. and it would suffice to have a simple list of filenames.
This will then be added in-tree.

I'm looking forward to your comments and suggestions!

@clone2727
Copy link
Contributor

What codec do the AVI videos use? It should try to use the best format instead of a specific 16bpp one.

@johndoe123
Copy link
Member Author

The videos use the iv32 codec.

#include "graphics/thumbnail.h"

static const PlainGameDescriptor bbvsGames[] = {
{ "bbvs", "Bbvs" },
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't here be the full game name?

@dreammaster
Copy link
Member

Looks good overall to me. The only minor thing I have issue with is the number of methods that have a blank first and/or last line. Although I guess it's not explicitly against the code formatting guidelines. Also the list of fields being read and written in saveload.cpp are large enough that you might want to consider refactoring them to share a method in common that uses a Serializer. That will make it easier to add in any new fields in the future if they're ever needed.

@bluegr
Copy link
Member

bluegr commented Feb 3, 2014

Nice work overall! Well done :)

@somaen
Copy link
Member

somaen commented Feb 5, 2014

16 commits total? Has the history here been squashed heavily?

@johndoe123
Copy link
Member Author

No, there just wasn't a useful commit history.

@digitall
Copy link
Member

digitall commented Feb 6, 2014

Nice work. I have a copy of this game and will give it a good valgrind and a run in a BE VM to see if any issues show up.

@sev-
Copy link
Member

sev- commented Feb 7, 2014

OK to merge from me

@digitall
Copy link
Member

digitall commented Feb 7, 2014

@johndoe123 : GCC warning still present...
engines/bbvs/bbvs.cpp: In member function ‘bool Bbvs::BbvsEngine::processCurrAction()’:
engines/bbvs/bbvs.cpp:1495:59: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
engines/bbvs/bbvs.cpp:1495:109: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

@johndoe123
Copy link
Member Author

The GCC warning should be fixed now.


}

bool BbvsEngine::walkTestLineWalkable(const Common::Point &sourcePt, const Common::Point &destPt, WalkInfo *walkInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could get rid of the floating point math here. At least I don't exactly see why we need it, if you use "nDeltaX = wDeltaY * ptDeltaY / ptDeltaX" in the nDeltaX definition, similar to what you do in the nDeltaY definition. But probably not important anyway.

@lordhoto
Copy link
Contributor

Looks like nice work overall. I left a few comments about style and other issues, like a memory leak AFAICT. I think the only game breaker for this is the broken commit I mentioned in the previous comment.

I am however not really sure about all the static data included in the engine (it might look bigger than it actually is though). In the past we have been favoring an addition datafile for this. Any thoughts about this?

It also seems to use floating point math. But if that poses a real problem, it shouldn't be too hard to replace it with fixed point math AFAICT.

@clone2727
Copy link
Contributor

I am all for keeping the static data in the engine. I'm not really sure why we really do it anymore, except really for kyra.

@johndoe123
Copy link
Member Author

I've (hopefully) fixed some of the problems suggested by lordhoto.
I'll fix the remaining ones (mainly the commit problem and the possible memory leak) the next days.
About the static data: It's not that much data and the size of the code to read the external file would probably even it out again.

@sev-
Copy link
Member

sev- commented Feb 20, 2014

Thanks to wjp, we were able to remove the wrong commit.

Now it looks to be fine to merge. wjp, LordHoto?

@lordhoto
Copy link
Contributor

Looks good now. Great!

@sev-
Copy link
Member

sev- commented Feb 21, 2014

OK, I'm merging then. The work should continue in-tree.

sev- added a commit that referenced this pull request Feb 21, 2014
BBVS: New engine: Beavis and Butthead In Virtual Stupidity
@sev- sev- merged commit c2e9f38 into scummvm:master Feb 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants