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

STARK: Implement the settings menu #1417

Merged
merged 24 commits into from May 29, 2018

Conversation

Projects
None yet
2 participants
@DouglasLiuGamer
Collaborator

DouglasLiuGamer commented May 21, 2018

This pull request is corresponding to implementing the settings menu of TLJ.

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 21, 2018

The previous stack used to track the screen name is faulty. It is fixed here.

virtual void onClick();
/** Called when the mouse hovers the widget */
virtual void onMouseMove(const Common::Point &mousePos);

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 22, 2018

Collaborator

Just move the virtual member functions up for better readability.

public:
SoundManager();
~SoundManager() {}

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 23, 2018

Collaborator

The original game plays the testing sound in a special looping way. When the mouse is pressing down, the music plays in a loop and when pressing up, the music will play its last round. This sound manager helps to achieve this.

@@ -54,6 +55,8 @@ VisualEffectBubbles::~VisualEffectBubbles() {
}
void VisualEffectBubbles::render(const Common::Point &position) {
if (!StarkSettings->getBoolSetting(Settings::kSpecialFX)) return;

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 24, 2018

Collaborator

Need to know a location with visual effects to test the correctness. Any suggestion?

This comment has been minimized.

@bgK

bgK May 24, 2018

Member

changeLocation 40 00 => green fireflies
changeLocation 55 00 => bubbles and fishes

if (!StarkSettings->getBoolSetting(Settings::kHighFMV) && StarkSettings->hasLowResFMV()) {
name.erase(name.size() - 4);
name += "_lo_res.bbb";
}

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 24, 2018

Collaborator

Is it possible that some versions of TLJ will use this suffix in upper-cases?

This comment has been minimized.

@bgK

bgK May 24, 2018

Member

It does not matter, the SearchManager used to lookup files in ResidualVM is completely case insensitive.

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 24, 2018

For the setting of fmv quality, right now it is assumed that all versions of TLJ will at least contain the high-quality fmv. By default high-quality one is used since we are all on modern computers.

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 25, 2018

One problem related to the fmv quality setting: I test it with the demo version, and the low-resolution fmv has the wrong ratio when playing.

void SettingsMenuScreen::close() {
StaticLocationScreen::close();
_soundManager.stop();

This comment has been minimized.

@bgK

bgK May 26, 2018

Member

There is a small memory management problem here:

==5646==ERROR: AddressSanitizer: heap-use-after-free on address 0x61200009c144 at pc 0x5651db86c64a bp 0x7fffeb2e10b0 sp 0x7fffeb2e10a0
READ of size 4 at 0x61200009c144 thread T0
    #0 0x5651db86c649 in Stark::Resources::Sound::stop() ../engines/stark/resources/sound.cpp:137
    #1 0x5651db8bfb41 in Stark::SoundManager::stop() ../engines/stark/ui/menu/settingsmenu.cpp:379
    #2 0x5651db8bdd03 in Stark::SettingsMenuScreen::close() ../engines/stark/ui/menu/settingsmenu.cpp:199
    #3 0x5651db8a5866 in Stark::UserInterface::changeScreen(Stark::Screen::Name) ../engines/stark/services/userinterface.cpp:156
    #4 0x5651db8a5999 in Stark::UserInterface::backPrevScreen() ../engines/stark/services/userinterface.cpp:164
    #5 0x5651db8bdf5f in Stark::SettingsMenuScreen::backHandler() ../engines/stark/ui/menu/settingsmenu.cpp:236
    #6 0x5651db8c15e4 in Common::Functor0Mem<void, Stark::SettingsMenuScreen>::operator()() const ../common/func.h:388
    #7 0x5651db8b6e73 in Stark::StaticLocationWidget::onClick() ../engines/stark/ui/menu/locationscreen.cpp:191
    #8 0x5651db8b61a3 in Stark::StaticLocationScreen::onClick(Common::Point const&) ../engines/stark/ui/menu/locationscreen.cpp:113
    #9 0x5651db8c2313 in Stark::Window::handleClick() ../engines/stark/ui/window.cpp:94
    #10 0x5651db8b3fd3 in Stark::SingleWindowScreen::handleClick() ../engines/stark/ui/screen.h:81
    #11 0x5651db8a523d in Stark::UserInterface::handleClick() ../engines/stark/services/userinterface.cpp:108
    #12 0x5651db82222a in Stark::StarkEngine::processEvents() ../engines/stark/stark.cpp:220
    #13 0x5651db82198a in Stark::StarkEngine::mainLoop() ../engines/stark/stark.cpp:170
    #14 0x5651db8217a6 in Stark::StarkEngine::run() ../engines/stark/stark.cpp:158
    #15 0x5651db4332a1 in runGame ../base/main.cpp:265
    #16 0x5651db435b8c in scummvm_main ../base/main.cpp:532
    #17 0x5651db42ee36 in main ../backends/platform/sdl/posix/posix-main.cpp:45
    #18 0x7f24c2fb906a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)
    #19 0x5651db4285b9 in _start (/home/bbouclet/prog/residualvm/build/residualvm+0x1a85b9)

0x61200009c144 is located 260 bytes inside of 264-byte region [0x61200009c040,0x61200009c148)
freed by thread T0 here:
    #0 0x7f24c61529c9 in operator delete(void*) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cc:135
    #1 0x5651db86b3f3 in Stark::Resources::Sound::~Sound() ../engines/stark/resources/sound.cpp:40
    #2 0x5651db8613d5 in Stark::Resources::Object::~Object() ../engines/stark/resources/object.cpp:111
    #3 0x5651db8fdd8a in Stark::Resources::Container::~Container() ../engines/stark/resources/container.cpp:30
    #4 0x5651db8fdda5 in Stark::Resources::Container::~Container() ../engines/stark/resources/container.cpp:31
    #5 0x5651db8613d5 in Stark::Resources::Object::~Object() ../engines/stark/resources/object.cpp:111
    #6 0x5651db938be6 in Stark::Resources::Location::~Location() ../engines/stark/resources/location.cpp:48
    #7 0x5651db938c01 in Stark::Resources::Location::~Location() ../engines/stark/resources/location.cpp:49
    #8 0x5651db87a404 in Stark::ArchiveLoader::LoadedArchive::~LoadedArchive() ../engines/stark/services/archiveloader.cpp:44
    #9 0x5651db87aa27 in Stark::ArchiveLoader::unloadUnused() ../engines/stark/services/archiveloader.cpp:75
    #10 0x5651db89d82d in Stark::StaticProvider::unloadLocation(Stark::Resources::Location*) ../engines/stark/services/staticprovider.cpp:151
    #11 0x5651db8b5be1 in Stark::StaticLocationScreen::close() ../engines/stark/ui/menu/locationscreen.cpp:63
    #12 0x5651db8bdcf3 in Stark::SettingsMenuScreen::close() ../engines/stark/ui/menu/settingsmenu.cpp:198
    #13 0x5651db8a5866 in Stark::UserInterface::changeScreen(Stark::Screen::Name) ../engines/stark/services/userinterface.cpp:156
    #14 0x5651db8a5999 in Stark::UserInterface::backPrevScreen() ../engines/stark/services/userinterface.cpp:164
    #15 0x5651db8bdf5f in Stark::SettingsMenuScreen::backHandler() ../engines/stark/ui/menu/settingsmenu.cpp:236
    #16 0x5651db8c15e4 in Common::Functor0Mem<void, Stark::SettingsMenuScreen>::operator()() const ../common/func.h:388
    #17 0x5651db8b6e73 in Stark::StaticLocationWidget::onClick() ../engines/stark/ui/menu/locationscreen.cpp:191
    #18 0x5651db8b61a3 in Stark::StaticLocationScreen::onClick(Common::Point const&) ../engines/stark/ui/menu/locationscreen.cpp:113
    #19 0x5651db8c2313 in Stark::Window::handleClick() ../engines/stark/ui/window.cpp:94
    #20 0x5651db8b3fd3 in Stark::SingleWindowScreen::handleClick() ../engines/stark/ui/screen.h:81
    #21 0x5651db8a523d in Stark::UserInterface::handleClick() ../engines/stark/services/userinterface.cpp:108
    #22 0x5651db82222a in Stark::StarkEngine::processEvents() ../engines/stark/stark.cpp:220
    #23 0x5651db82198a in Stark::StarkEngine::mainLoop() ../engines/stark/stark.cpp:170
    #24 0x5651db8217a6 in Stark::StarkEngine::run() ../engines/stark/stark.cpp:158
    #25 0x5651db4332a1 in runGame ../base/main.cpp:265
    #26 0x5651db435b8c in scummvm_main ../base/main.cpp:532
    #27 0x5651db42ee36 in main ../backends/platform/sdl/posix/posix-main.cpp:45
    #28 0x7f24c2fb906a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)

previously allocated by thread T0 here:
    #0 0x7f24c6151a91 in operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cc:90
    #1 0x5651db8db88f in Stark::Formats::XRCReader::createResource(Stark::Formats::XRCReadStream*, Stark::Resources::Object*) ../engines/stark/formats/xrc.cpp:226
    #2 0x5651db8db150 in Stark::Formats::XRCReader::importResource(Stark::Formats::XRCReadStream*, Stark::Resources::Object*) ../engines/stark/formats/xrc.cpp:158
    #3 0x5651db8dca72 in Stark::Formats::XRCReader::importResourceChildren(Stark::Formats::XRCReadStream*, Stark::Resources::Object*) ../engines/stark/formats/xrc.cpp:326
    #4 0x5651db8db17a in Stark::Formats::XRCReader::importResource(Stark::Formats::XRCReadStream*, Stark::Resources::Object*) ../engines/stark/formats/xrc.cpp:160
    #5 0x5651db8dca72 in Stark::Formats::XRCReader::importResourceChildren(Stark::Formats::XRCReadStream*, Stark::Resources::Object*) ../engines/stark/formats/xrc.cpp:326
    #6 0x5651db8db17a in Stark::Formats::XRCReader::importResource(Stark::Formats::XRCReadStream*, Stark::Resources::Object*) ../engines/stark/formats/xrc.cpp:160
    #7 0x5651db8dafce in Stark::Formats::XRCReader::importTree(Stark::Formats::XARCArchive*) ../engines/stark/formats/xrc.cpp:150
    #8 0x5651db87a443 in Stark::ArchiveLoader::LoadedArchive::importResources() ../engines/stark/services/archiveloader.cpp:49
    #9 0x5651db87a803 in Stark::ArchiveLoader::load(Common::String const&) ../engines/stark/services/archiveloader.cpp:67
    #10 0x5651db89d1ea in Stark::StaticProvider::loadLocation(char const*) ../engines/stark/services/staticprovider.cpp:129
    #11 0x5651db8b5b33 in Stark::StaticLocationScreen::open() ../engines/stark/ui/menu/locationscreen.cpp:57
    #12 0x5651db8bb688 in Stark::SettingsMenuScreen::open() ../engines/stark/ui/menu/settingsmenu.cpp:44
    #13 0x5651db8a591a in Stark::UserInterface::changeScreen(Stark::Screen::Name) ../engines/stark/services/userinterface.cpp:158
    #14 0x5651db8ba456 in Stark::MainMenuScreen::settingsHandler() ../engines/stark/ui/menu/mainmenu.cpp:174
    #15 0x5651db8bb0c8 in Common::Functor0Mem<void, Stark::MainMenuScreen>::operator()() const ../common/func.h:388
    #16 0x5651db8b6e73 in Stark::StaticLocationWidget::onClick() ../engines/stark/ui/menu/locationscreen.cpp:191
    #17 0x5651db8b61a3 in Stark::StaticLocationScreen::onClick(Common::Point const&) ../engines/stark/ui/menu/locationscreen.cpp:113
    #18 0x5651db8c2313 in Stark::Window::handleClick() ../engines/stark/ui/window.cpp:94
    #19 0x5651db8b3fd3 in Stark::SingleWindowScreen::handleClick() ../engines/stark/ui/screen.h:81
    #20 0x5651db8a523d in Stark::UserInterface::handleClick() ../engines/stark/services/userinterface.cpp:108
    #21 0x5651db82222a in Stark::StarkEngine::processEvents() ../engines/stark/stark.cpp:220
    #22 0x5651db82198a in Stark::StarkEngine::mainLoop() ../engines/stark/stark.cpp:170
    #23 0x5651db8217a6 in Stark::StarkEngine::run() ../engines/stark/stark.cpp:158
    #24 0x5651db4332a1 in runGame ../base/main.cpp:265
    #25 0x5651db435b8c in scummvm_main ../base/main.cpp:532
    #26 0x5651db42ee36 in main ../backends/platform/sdl/posix/posix-main.cpp:45
    #27 0x7f24c2fb906a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)
@bgK

This comment has been minimized.

Member

bgK commented May 26, 2018

One problem related to the fmv quality setting: I test it with the demo version, and the low-resolution fmv has the wrong ratio when playing.

Can you look into fixing it?

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 26, 2018

I modified all three findBonesMesh(), yet based on the testing on the game and demo, at least at the beginning only the GlobalItemTemplete::findBonesMesh() one is used. Not sure whether it is needed to change all three of them.

BTW, the quality change is almost unnoticeable...

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 27, 2018

About the low-res FMV problem: it seems that the way low-res FMV works is that it is obtained through shrinking the original FMV on the height, so the ratio is naturally wrong. Right now I don't see a way to stretch it back. Looks like each frame of an FMV is decoded as a surface, and it does not provide stretching functionality.

_maxX = _bgPosition.x + _bgWidth - _sliderWidth / 2;
_bgPosition.x = 313;
_bgPosition.y = 303 + _settingIndex * 51;
_sliderPosition.y = _bgPosition.y;

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 27, 2018

Collaborator

Not completely identical to the original game, but should simulate well enough.

ConfMan.registerDefault(_boolKey[kTimeSkip], false);
// Use the FunCom logo video to check low-resolution fmv
_hasLowRes = StarkArchiveLoader->getExternalFile("1402_lo_res.bbb", "Global/");

This comment has been minimized.

@bgK

bgK May 27, 2018

Member

This causes a memory leak when the file is found:

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x7ff445ad1a91 in operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cc:90
    #1 0x562c7ce33a29 in StdioStream::makeFromPath(Common::String const&, bool) ../backends/fs/stdiostream.cpp:102
    #2 0x562c7ce263f4 in POSIXFilesystemNode::createReadStream() ../backends/fs/posix/posix-fs.cpp:254
    #3 0x562c7d0598e7 in Common::FSNode::createReadStream() const ../common/fs.cpp:140
    #4 0x562c7d05a6ce in Common::FSDirectory::createReadStreamForMember(Common::String const&) const ../common/fs.cpp:233
    #5 0x562c7d03f504 in Common::SearchSet::createReadStreamForMember(Common::String const&) const ../common/archive.cpp:260
    #6 0x562c7cbe460d in Stark::ArchiveLoader::getExternalFile(Common::String const&, Common::String const&) const ../engines/stark/services/archiveloader.cpp:153
    #7 0x562c7cc11c0a in Stark::Settings::Settings(Audio::Mixer*) ../engines/stark/services/settings.cpp:56
    #8 0x562c7cb7c35c in Stark::StarkEngine::run() ../engines/stark/stark.cpp:126
    #9 0x562c7cb41c61 in runGame ../base/main.cpp:265
    #10 0x562c7cb4454c in scummvm_main ../base/main.cpp:532
    #11 0x562c7cb3d7f6 in main ../backends/platform/sdl/posix/posix-main.cpp:45
    #12 0x7ff44271b06a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)
nullptr));
_widgets.back()->setVisible(false);
if (!isDemo()) {

This comment has been minimized.

@bgK

bgK May 27, 2018

Member

This test should be different. My 4 CD French version does not have AllowFF either. Perhaps test if the render entry exists?

/**
* Manager of test sound
*/
class SoundManager {

This comment has been minimized.

@bgK

bgK May 27, 2018

Member

Perhaps this class should be renamed to TestSoundManager to better reflect its responsibility.

SoundManager _soundManager;
bool isDemo() {
return StarkGameDescription->flags & ADGF_DEMO;

This comment has been minimized.

@bgK

bgK May 27, 2018

Member

There are now two implementations of isDemo. I think it'd be better to put one in StarkSettings and reuse it.

bool hasLowResFMV() { return _hasLowRes; }
private:
const static int _maxVolume = 256;

This comment has been minimized.

@bgK

bgK May 27, 2018

Member

This is not used.

_hasLowRes = StarkArchiveLoader->getExternalFile("1402_lo_res.bbb", "Global/");
// Apply the volume
setIntSetting(kVoice, getIntSetting(kVoice));

This comment has been minimized.

@bgK

bgK May 27, 2018

Member

Is this really needed?

void Engine::syncSoundSettings() {

Should already do the same.

@@ -70,11 +71,9 @@ StarkEngine::StarkEngine(OSystem *syst, const ADGameDescription *gameDesc) :
_diary(nullptr),
_userInterface(nullptr),
_fontProvider(nullptr),
_settings(nullptr),
_lastClickTime(0) {
_mixer->setVolumeForSoundType(Audio::Mixer::kPlainSoundType, 127);

This comment has been minimized.

@bgK

bgK May 27, 2018

Member

We don't need this line either as we don't use kPlainSoundType and syncSoundSettings sets it anyways.

_mixer->setVolumeForSoundType(Audio::Mixer::kPlainSoundType, Audio::Mixer::kMaxMixerVolume);

Settings::Settings(Audio::Mixer *mixer) :
_mixer(mixer) {
// Initialize keys
_boolKey[kHighModel] = "high_model";

This comment has been minimized.

@bgK

bgK May 27, 2018

Member

The settings name are user facing through the configuration file. I suggest using more explicit names:

  • high_model => use_high_resolution_models
  • specialfx => enable_special_effects
  • shadow => enable_shadows
  • high_fmv => play_high_resolution_videos
@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 27, 2018

Based on my current thinking of the save & load menu, it will provide more flexibility if StaticLocationWidget can have empty RenderEntry field, since some complex widgets may need an empty base class as their container. This naturally fixed any problem due to different versions of TLJ that have different widgets. A debug message will still be printed for now.

@bgK

This comment has been minimized.

Member

bgK commented May 28, 2018

Thanks for your changes. This is looking very good now. Can we merge?

Based on my current thinking of the save & load menu, it will provide more flexibility if StaticLocationWidget can have empty RenderEntry field, since some complex widgets may need an empty base class as their container.

Sounds ok. Maybe adding a parent class that does not have a RenderEntry to StaticLocationWidget could also work.

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 29, 2018

@bgK I don't have more improvement in mind now. If you think it is good enough, we can merge them now :D

@@ -77,7 +83,7 @@ void FMVScreen::onRender() {
stop();
}
_surfaceRenderer->render(_texture, Common::Point(0, Gfx::Driver::kTopBorderHeight));
_surfaceRenderer->render(_texture, Common::Point(0, Gfx::Driver::kTopBorderHeight), 640, 365);

This comment has been minimized.

@bgK

bgK May 29, 2018

Member

Please use the constants defined here:

static const int32 kGameViewportHeight = 365;

@@ -46,7 +46,8 @@ class OpenGLSSurfaceRenderer : public SurfaceRenderer {
virtual ~OpenGLSSurfaceRenderer();
// SurfaceRenderer API
void render(const Texture *texture, const Common::Point &dest);
void render(const Texture *texture, const Common::Point &dest) override;
void render(const Texture *texture, const Common::Point &dest, const float width, const float height) override;

This comment has been minimized.

@bgK

bgK May 29, 2018

Member

I can't see a reason why the destination rectangle dimensions would need to be floating point numbers. I'd prefer keeping the width and height as integers so it's more clear dimensions in pixels are expected, and not some kind of normalized coordinates.

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 29, 2018

Collaborator

The original implementation in render() used floating point in the width and height. I thought it is a must.

@bgK

This comment has been minimized.

Member

bgK commented May 29, 2018

All good!

@bgK bgK merged commit 36e14c8 into residualvm:master May 29, 2018

1 check passed

default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment