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

PRIVATE: New engine for Private Eye #2771

Open
wants to merge 147 commits into
base: master
from
Open

Conversation

@neuromancer
Copy link
Contributor

@neuromancer neuromancer commented Feb 13, 2021

The game is completable but there are a few missing minor features:

  • The movie player in the diary.
  • The opcode to select digits and open the safe (optional to complete the game).
  • Browsing dossiers should allow to switch between first and second page.

Also, I'm still cleaning up the source code, but I believe it can be reviewed
already. I know the commits messages are not following the guidelines, so
please considering squashing them to keep this a single commit.
As expected, the game needs extensive testing and bug fixing,
since the gameplay has a lot different variations and I was not able to
test them all.

This engine only allows to play the US version of the Windows game and
its demo. I wasn't able to test any MacOS version. European versions
should be correctly detected but they are not supported. They require
to add bytecode support to run the same assets files (it shouldn't be
too difficult to implement).

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Feb 13, 2021

DeepCode's analysis on #6a3f96 found:

  • ⚠️ 4 warnings 👇

Top issues

Description Example fixes
The result of malloc, which may return null flows to the first argument of memset. This could result in undefined behavior. Consider adding a check for nullness. Occurrences: 🔧 Example fixes
Unreachable code. Occurrences: 🔧 Example fixes
Memory leak. Memory is never freed if realloc fails to allocate memory. Occurrences: 🔧 Example fixes

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

Copy link
Member

@sev- sev- left a comment

Great work. At this moment, there are a number of issues with logic, memory management and mostly with the code formatting and naming.

common/installer_archive.h Outdated Show resolved Hide resolved
engines/private/code.cpp Show resolved Hide resolved
engines/private/code.cpp Outdated Show resolved Hide resolved
engines/private/code.cpp Outdated Show resolved Hide resolved
engines/private/code.cpp Outdated Show resolved Hide resolved
engines/private/private.h Outdated Show resolved Hide resolved
engines/private/symbol.cpp Outdated Show resolved Hide resolved
engines/private/symbol.cpp Outdated Show resolved Hide resolved
engines/private/symbol.h Outdated Show resolved Hide resolved
typedef Common::List<Common::String> NameList;
typedef Common::List<Symbol*> ConstantList;

extern SymbolMap settings, variables, cursors, locations, rects;

This comment has been minimized.

@sev-

sev- Feb 13, 2021
Member

extern is C'ism here. Please remove.

This comment has been minimized.

@neuromancer

neuromancer Feb 14, 2021
Author Contributor

Removing this extern will cause the compiler to fail for the multiple definition of these variables:

engines/private/symbol.cpp:29:11: error: redefinition of 'Private::SymbolMap Private::settings'
   29 | SymbolMap settings, variables, cursors, locations, rects;
      |           ^~~~~~~~
In file included from ./engines/private/grammar.h:30,
                 from engines/private/symbol.cpp:24:
./engines/private/symbol.h:55:11: note: 'Private::SymbolMap Private::settings' previously declared here
   55 | SymbolMap settings, variables, cursors, locations, rects;

This comment has been minimized.

@sev-

sev- Feb 15, 2021
Member

Just wrap it into a class. Polluting your engine namespace like this is not the best idea.

This comment has been minimized.

@neuromancer

neuromancer Feb 17, 2021
Author Contributor

I think this is done. Please take a look

@sev-
Copy link
Member

@sev- sev- commented Feb 13, 2021

The commits must conform to our commit guidelines. Put 'PRIVATE:` prefix everywhere except the common code.

engines/private/detection.cpp Outdated Show resolved Hide resolved
Copy link
Member

@sev- sev- left a comment

Very good progress. Only a handful of relatively small notes left.

engines/private/private.h Outdated Show resolved Hide resolved
engines/private/funcs.cpp Outdated Show resolved Hide resolved
engines/private/funcs.cpp Outdated Show resolved Hide resolved
engines/private/funcs.cpp Outdated Show resolved Hide resolved
engines/private/funcs.cpp Show resolved Hide resolved
engines/private/private.cpp Outdated Show resolved Hide resolved
engines/private/private.cpp Outdated Show resolved Hide resolved
// structs

typedef struct ExitInfo {
Common::String *nextSetting;

This comment has been minimized.

@sev-

sev- Feb 19, 2021
Member

then you could check if (nextSetting.empty())

engines/private/symbol.h Outdated Show resolved Hide resolved
common/installshieldv3_archive.h Outdated Show resolved Hide resolved
Copy link
Member

@sev- sev- left a comment

Only three tiny notes and then it is ready for merge.

*
*/

#ifndef COMMON_INSTALLER_ARCHIVE_H

This comment has been minimized.

@sev-

sev- Feb 26, 2021
Member

This has to be updated after the file rename.

_setting = (Setting *)malloc(sizeof(Setting));
memset((void *)_setting, 0, sizeof(Setting));

Gen::g_vm->_prog = (Inst *)&_setting->prog;

This comment has been minimized.

@sev-

sev- Feb 26, 2021
Member

You declared using namespace Gen;. Thus, you could write directly g_vm->_prog = (Inst *)&_setting->prog;

If you cannot, then move the using inside of namespace Settings, so these namespace specifiers could be dropped.

engines/private/symbol.h Outdated Show resolved Hide resolved
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

3 participants