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

MUTATIONOFJB: New engine. #1270

Merged
merged 75 commits into from Aug 25, 2018

Conversation

Projects
None yet
4 participants
@LubomirR
Member

LubomirR commented Aug 4, 2018

Add a new WIP engine for Mutation of J.B.

Currently we support:

  • Rendering scene background and basic object animations,
  • Parsing and executing basic script commands (game state changing and control flow - those which are absolutely necessary to finish the game),
  • Verb actions (except for combining items),
  • Inventory,
  • Map,
  • Interactive dialogs.

Note: The game is not completable yet! The missing things we need to implement to be able to finish the game are:

  • Certain items cannot be picked up,
  • Combining items,
  • Switching game parts (the game is divided into two parts/chapters),
  • Special scenes such as computer minigame.

The information about the current status of the engine can be also found on the wiki: http://wiki.scummvm.org/index.php/MutationOfJB
And there's a video available: https://www.youtube.com/watch?v=PuY4T0kmcjk .

@dreammaster

This comment has been minimized.

dreammaster commented on engines/mutationofjb/detection.cpp in c4b8914 Aug 17, 2018

If I'm reading this right, you're manually detecting the game based on the presence of the startup.dat file. Is there any particular reason you're not using the AdvancedMetaEngine functionality like other engines do? Even if there are several variations of startup.dat, it's probably better to have an md5 list of the different variations for future reference.

This comment has been minimized.

dreammaster replied Aug 17, 2018

Plus, it's always possible other games may have a startup.dat, so using md5 detection would prevent false positive detection of the game

This comment has been minimized.

Owner

LubomirR replied Aug 18, 2018

It's one of the first commits and at that time we didn't have a good idea how to detect the game. Now I've got the original CDs for both German and Slovak version, so I can use the advanced meta engine with MD5 detection.

@dreammaster

This comment has been minimized.

Member

dreammaster commented Aug 18, 2018

Regarding comments, yeh, we've all had a tendency to under-comment the code we write. The important thing for engines is that the code be clear for posterity, so that if bugs crop up in the future, the code is understandable enough that anyone could fix it. Comments can help make the code more understandable, above and beyond good naming conventions. In any case, it's not a prerequisite for merging the engine in. Just maybe something to keep in mind; maybe even consider whether writing a method comment would be useful for each new method you create in the future.

LubomirR added some commits Feb 25, 2018

MUTATIONOFJB: Don't store ActionInfo pointers, because they might be …
…invalidated, and parse/show actions with two objects correctly.
@LubomirR

This comment has been minimized.

Member

LubomirR commented Aug 19, 2018

I've addressed all issues mentioned by @dreammaster. Unfortunately, due to build errors, I had to rewrite the history (rebase), so his comments are gone. I archived them and I'll put them here:

  1. In Visual Studio at least, it throws an error if you include the override keywords in the actual method implementations.

    Copy/paste error preventing compilation in C++11 mode. I've removed the extra override keyword on method implementations.

  2. _data is an uint32, so it gives an error trying to cast nullptr to an integer

    _data used to be a pointer and I forgot to change it back. Again, in C++98 this compiled fine due to C++11-compat header defining nullptr as 0. Fixed.

  3. There shouldn't be any space between casts and the variable being cast. This actually happens in multiple different places in the code, so you might want to do a global search and replace for casts to various types that have a space after the bracket.

    Fixed at many places.

  4. If I'm reading this right, you're manually detecting the game based on the presence of the startup.dat file. Is there any particular reason you're not using the AdvancedMetaEngine functionality like other engines do? Even if there are several variations of startup.dat, it's probably better to have an md5 list of the different variations for future reference.

    Plus, it's always possible other games may have a startup.dat, so using md5 detection would prevent false positive detection of the game

    This was one of the earliest commits and at that time we didn't really know how to detect the game. Now that I have original CDs of both Slovak and German version, I changed the detection code to use the advanced detector.

  5. Warning that ActionInfo is defined as a struct elsewhere, but a class here

    You changed GameData to struct, but it's still forward defined as "class GameData" in game.h. There are several other classes suffering from similar problems, such as Door, Static, Bitmap that should be fixed

    And Common::Event is a struct, not a class. In mutationofjb.h forward declaration

    Fixed. (Looks like only VS is complaining about this.)

  6. You should a blank line before and after the method. Also, it's bad form to have underscores at the start of method names, since it's confusing the methods with fields.

    Blank line between end of method and namespace

    Another needed blank line. I won't bother explicitly mentioning any other file with this problem I encounter, so just double-check all of the code files

    Added the blank line (at other places as well) and renamed the method that was starting with an underscore.

  7. I can't be sure at the moment with the current errors, but I suspect that gcc will be generating a lot of warnings about classes implementing virtual methods but not having a virtual destructor. You should try compiling ScumMVM under gcc if you're not already.. if you're using Windows, you can install MinGW, or even use VirtualBox and download an Ubuntu VM. Which is also helpful for running Valgrind to check for memory leaks.

    The destructor is marked as virtual in the base class. I don't see any warnings regarding missing virtual destructor with either gcc or clang. Didn't do anything here.

  8. I don't personally think it's necessary to mark all the internal warnings as translateable. It justs adds needless work for translators later on for messages that are never meant to be seen. But I'm still okay if you'd prefer to keep them this way.

    Removed calls to _ (translation function) from debug, warning and console messages.

  9. All method comments should be in the header files (class definitions), not the cpp files. I'll also use this opportunity to mention that the engine currently suffers from a lack of method and class comments in general. If feasible, when you have time, it would be good to flesh out comments in the engine, for posterity. Though I can appreciate that it may end up being boring trying to enter so many comments at this late a stage.

    Moved method comments to header files. We'll try to comment things more in the future (and even add missing comments to already existing methods when we change those methods).

@dreammaster

This comment has been minimized.

Member

dreammaster commented Aug 19, 2018

It's looking good and compiling fine now. There are just a few warnings left, that you might as well fix as well. Even if the debug ones are false positives.
c:\users\dreammaster\documents\visual studio 2015\projects\scummvm\engines\mutationofjb\tasks\objectanimationtask.h(32): warning C4099: 'MutationOfJB::Object': type name first seen using 'struct' now seen using 'class'
1> c:\users\dreammaster\documents\visual studio 2015\projects\scummvm\engines\mutationofjb\gamedata.h(102): note: see declaration of 'MutationOfJB::Object'

1>c:\users\dreammaster\documents\visual studio 2015\projects\scummvm\engines\mutationofjb\debug.cpp(117): warning C4701: potentially uninitialized local variable 'action' used
1>c:\users\dreammaster\documents\visual studio 2015\projects\scummvm\engines\mutationofjb\debug.cpp(194): warning C4701: potentially uninitialized local variable 'action' used

1>c:\users\dreammaster\documents\visual studio 2015\projects\scummvm\engines\mutationofjb\gamedata.h(102): warning C4099: 'MutationOfJB::Object': type name first seen using 'class' now seen using 'struct'
1> c:\users\dreammaster\documents\visual studio 2015\projects\scummvm\engines\mutationofjb\gamedata.h(102): note: see declaration of 'MutationOfJB::Object'

@LubomirR

This comment has been minimized.

Member

LubomirR commented Aug 19, 2018

Fixed. I tested it on a Windows machine with VS 2015 and I can't see any more warnings. Thanks!

@LubomirR

This comment has been minimized.

Member

LubomirR commented Aug 25, 2018

Is there anything else blocking this PR?

@dreammaster

This comment has been minimized.

Member

dreammaster commented Aug 25, 2018

I'm happy with it's status, and it's been available for review for a requisite 3 weeks. Unless anyone else decides to do some last minute reviews, I'll merge it into master tomorrow.

@sev-

This comment has been minimized.

Member

sev- commented Aug 25, 2018

Merging. Please, continue your work in-tree, and send an intro level to -devel. Also, I added the engine to the coverity, please send your request, so you could see the static analysis results. It will be first analysed on Monday 2am (night from Sun to Mon)

@sev- sev- merged commit b00395b into scummvm:master Aug 25, 2018

1 check passed

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