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

STARK: Implement the video replay menu #1442

Merged
merged 11 commits into from Jun 12, 2018

Conversation

Projects
None yet
2 participants
@DouglasLiuGamer
Copy link
Collaborator

DouglasLiuGamer commented Jun 6, 2018

This PR is corresponding to the implementation of the video replay menu of TLJ, which is probably the easiest menu to implement.

Based on the original game, the widget for FMV entries works slightly different from our written StaticLocationWidget. The cursor will not change when hovering on it. Since the widget does not have many fancy functionalities, I chose to build it out of scratch instead of extending the StaticLocationWidget.

I don't see the necessity to dynamically calculate some UI position parameters so I hard-coded them. If the dynamic calculation is really needed like there is a version with dramatically long video titles in a different language, it could be easily fixed.

It would be important now to actually pause the game's sound when the menu is opened, otherwise, the game's sound will jam with the replayed video sound.

@DouglasLiuGamer DouglasLiuGamer requested a review from bgK Jun 6, 2018

}

void FMVWidget::onClick() {
StarkUserInterface->requestFMVPlayback(_filename);

This comment has been minimized.

@bgK

bgK Jun 7, 2018

Member

I'm getting a memory management issue here, when trying to play a FMV:

==7563==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e00006c3e0 at pc 0x55d6dcd16ffa bp 0x7ffea1c64210 sp 0x7ffea1c64200
READ of size 4 at 0x60e00006c3e0 thread T0
    #0 0x55d6dcd16ff9 in Common::String::operator+=(Common::String const&) ../common/str.cpp:269
    #1 0x55d6dcd19c3b in Common::operator+(char const*, Common::String const&) ../common/str.cpp:700
    #2 0x55d6dc8680f4 in Stark::ArchiveLoader::getExternalFile(Common::String const&, Common::String const&) const ../engines/stark/services/archiveloader.cpp:150
    #3 0x55d6dc8bad96 in Stark::FMVScreen::play(Common::String const&) ../engines/stark/ui/world/fmvscreen.cpp:74
    #4 0x55d6dc892b0c in Stark::UserInterface::requestFMVPlayback(Common::String const&) ../engines/stark/services/userinterface.cpp:154
    #5 0x55d6dc8b7642 in Stark::FMVWidget::onClick() ../engines/stark/ui/menu/fmvmenu.cpp:159
    #6 0x55d6dc8b6d12 in Stark::FMVMenuScreen::onClick(Common::Point const&) ../engines/stark/ui/menu/fmvmenu.cpp:98
    #7 0x55d6dc8b9c1f in Stark::Window::handleClick() ../engines/stark/ui/window.cpp:94
    #8 0x55d6dc8a401f in Stark::SingleWindowScreen::handleClick() ../engines/stark/ui/screen.h:84
    #9 0x55d6dc89282d in Stark::UserInterface::handleClick() ../engines/stark/services/userinterface.cpp:119
    #10 0x55d6dc8011ce in Stark::StarkEngine::processEvents() ../engines/stark/stark.cpp:222
    #11 0x55d6dc80092e in Stark::StarkEngine::mainLoop() ../engines/stark/stark.cpp:172
    #12 0x55d6dc80074a in Stark::StarkEngine::run() ../engines/stark/stark.cpp:160
    #13 0x55d6dc7c5591 in runGame ../base/main.cpp:265
    #14 0x55d6dc7c7e7c in scummvm_main ../base/main.cpp:532
    #15 0x55d6dc7c1126 in main ../backends/platform/sdl/posix/posix-main.cpp:45
    #16 0x7fcb3b9d106a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)
    #17 0x55d6dc7ba8a9 in _start (/home/bbouclet/prog/residualvm/build/residualvm+0x1028a9)

0x60e00006c3e0 is located 0 bytes inside of 160-byte region [0x60e00006c3e0,0x60e00006c480)
freed by thread T0 here:
    #0 0x7fcb3ed879c9 in operator delete(void*) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cc:135
    #1 0x55d6dc8b6e72 in Stark::FMVMenuScreen::freeFMVWidgets() ../engines/stark/ui/menu/fmvmenu.cpp:117
    #2 0x55d6dc8b6b0f in Stark::FMVMenuScreen::close() ../engines/stark/ui/menu/fmvmenu.cpp:76
    #3 0x55d6dc892d36 in Stark::UserInterface::changeScreen(Stark::Screen::Name) ../engines/stark/services/userinterface.cpp:167
    #4 0x55d6dc892ad0 in Stark::UserInterface::requestFMVPlayback(Common::String const&) ../engines/stark/services/userinterface.cpp:152
    #5 0x55d6dc8b7642 in Stark::FMVWidget::onClick() ../engines/stark/ui/menu/fmvmenu.cpp:159
    #6 0x55d6dc8b6d12 in Stark::FMVMenuScreen::onClick(Common::Point const&) ../engines/stark/ui/menu/fmvmenu.cpp:98
    #7 0x55d6dc8b9c1f in Stark::Window::handleClick() ../engines/stark/ui/window.cpp:94
    #8 0x55d6dc8a401f in Stark::SingleWindowScreen::handleClick() ../engines/stark/ui/screen.h:84
    #9 0x55d6dc89282d in Stark::UserInterface::handleClick() ../engines/stark/services/userinterface.cpp:119
    #10 0x55d6dc8011ce in Stark::StarkEngine::processEvents() ../engines/stark/stark.cpp:222
    #11 0x55d6dc80092e in Stark::StarkEngine::mainLoop() ../engines/stark/stark.cpp:172
    #12 0x55d6dc80074a in Stark::StarkEngine::run() ../engines/stark/stark.cpp:160
    #13 0x55d6dc7c5591 in runGame ../base/main.cpp:265
    #14 0x55d6dc7c7e7c in scummvm_main ../base/main.cpp:532
    #15 0x55d6dc7c1126 in main ../backends/platform/sdl/posix/posix-main.cpp:45
    #16 0x7fcb3b9d106a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)

previously allocated by thread T0 here:
    #0 0x7fcb3ed86a91 in operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cc:90
    #1 0x55d6dc8b700e in Stark::FMVMenuScreen::loadFMVWidgets(int) ../engines/stark/ui/menu/fmvmenu.cpp:128
    #2 0x55d6dc8b71aa in Stark::FMVMenuScreen::changePage(int) ../engines/stark/ui/menu/fmvmenu.cpp:136
    #3 0x55d6dc8b69eb in Stark::FMVMenuScreen::open() ../engines/stark/ui/menu/fmvmenu.cpp:72
    #4 0x55d6dc892dea in Stark::UserInterface::changeScreen(Stark::Screen::Name) ../engines/stark/services/userinterface.cpp:169
    #5 0x55d6dc8a3e58 in Stark::DiaryIndexScreen::fmvHandler() ../engines/stark/ui/menu/diaryindex.cpp:117
    #6 0x55d6dc8a5724 in Common::Functor0Mem<void, Stark::DiaryIndexScreen>::operator()() const ../common/func.h:388
    #7 0x55d6dc8a6f8e in Stark::StaticLocationWidget::onClick() ../engines/stark/ui/menu/locationscreen.cpp:195
    #8 0x55d6dc8a61ef in Stark::StaticLocationScreen::onClick(Common::Point const&) ../engines/stark/ui/menu/locationscreen.cpp:113
    #9 0x55d6dc8b9c1f in Stark::Window::handleClick() ../engines/stark/ui/window.cpp:94
    #10 0x55d6dc8a401f in Stark::SingleWindowScreen::handleClick() ../engines/stark/ui/screen.h:84
    #11 0x55d6dc89282d in Stark::UserInterface::handleClick() ../engines/stark/services/userinterface.cpp:119
    #12 0x55d6dc8011ce in Stark::StarkEngine::processEvents() ../engines/stark/stark.cpp:222
    #13 0x55d6dc80092e in Stark::StarkEngine::mainLoop() ../engines/stark/stark.cpp:172
    #14 0x55d6dc80074a in Stark::StarkEngine::run() ../engines/stark/stark.cpp:160
    #15 0x55d6dc7c5591 in runGame ../base/main.cpp:265
    #16 0x55d6dc7c7e7c in scummvm_main ../base/main.cpp:532
    #17 0x55d6dc7c1126 in main ../backends/platform/sdl/posix/posix-main.cpp:45
    #18 0x7fcb3b9d106a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)
StarkUserInterface->freeGameScreenThumbnail();
}

void GameScreen::close() {
_cursor->setMouseHint("");
StarkUserInterface->saveGameScreenThumbnail();
g_engine->pauseEngine(true);

This comment has been minimized.

@bgK

bgK Jun 7, 2018

Member

Pausing the engine while the game menu is open is not ok. It prevents the GMM from working.
Only the game entities need to be paused, adding a pause event handler to the Sound entities to pause them in the mixer.

@bgK
Copy link
Member

bgK left a comment

Thank! I've made a few comments, mostly about things that should not be hardcoded, that need to be addressed before this can be merged.

Common::Rect rect = _title.getRect();
_width = rect.right - rect.left;

_position.x = 202;

This comment has been minimized.

@bgK

bgK Jun 7, 2018

Member

These values should come from the FormatRectangle Item instead of being hardcoded.

Common::Rect rect = _title.getRect();
_width = rect.right - rect.left;

_position.x = 202;

This comment has been minimized.

@bgK

bgK Jun 7, 2018

Member

I don't think it should be the responsibility of the widget to compute its position in the screen. The screen should be responsible for the layout of the widgets.


private:
const static int _fmvPerPage = 18;
const static int _height = 16;

This comment has been minimized.

@bgK

bgK Jun 7, 2018

Member

The height of the widget should not be hardcoded, but computed based on the font. Different localizations will have different fonts (read from gui.ini), and thus different heights.

namespace Stark {

FMVMenuScreen::FMVMenuScreen(Gfx::Driver *gfx, Cursor *cursor) :
StaticLocationScreen(gfx, cursor, "DiaryFMV", Screen::kScreenFMVMenu) {

This comment has been minimized.

@bgK

bgK Jun 7, 2018

Member

Please make sure all the fields are initialized (even to zero) in the constructor (Here it's mostly to avoid warnings from the static code analysis tools, but it's a good practice anyway).

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 8, 2018

I don't think it should be the responsibility of the widget to compute its position in the screen. The screen should be responsible for the layout of the widgets.

I think it will make things more complex. The screen will need to generate and pass extra parameters to widgets. Plus we actually never let the screen controls the layout before. They are largely handled by the RenderEntry. So I don't think that letting the widget decides its own position is a problem.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jun 9, 2018

I think it will make things more complex. The screen will need to generate and pass extra parameters to widgets. Plus we actually never let the screen controls the layout before. They are largely handled by the RenderEntry. So I don't think that letting the widget decides its own position is a problem.

Ok.

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-fmv branch from 99229f4 to c7bc747 Jun 9, 2018

StarkGlobal->getCurrent()->getLevel()->onEnginePause(pause);
StarkGlobal->getCurrent()->getLocation()->onEnginePause(pause);
}
g_engine->_mixer->pauseAll(pause);

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 9, 2018

Author Collaborator

I am still not sure about how the pausing should work. Based on my observation, the only thing that actually does something in onEnginePause() is the animation. Yet the menu cannot be opened in most important animations anyway. For other simple animation such as when clicking April, it is naturally paused by the menu in the previous code. My guess is that the animation will naturally be paused when it is not rendered. So I really don't see the importance of pausing other things when the menu is opened. Is there a location where I can actually see that not pausing the game in the menu will cause problems?

As for pausing sounds, calling the pauseAll() of the mixer seems not to affect newly added sounds later. I see that sounds will automatically call stop() when a location is unloaded, so everything should be right..ish?

This comment has been minimized.

@bgK

bgK Jun 10, 2018

Member

I am still not sure about how the pausing should work. Based on my observation, the only thing that actually does something in onEnginePause() is the animation. Yet the menu cannot be opened in most important animations anyway. For other simple animation such as when clicking April, it is naturally paused by the menu in the previous code. My guess is that the animation will naturally be paused when it is not rendered. So I really don't see the importance of pausing other things when the menu is opened. Is there a location where I can actually see that not pausing the game in the menu will cause problems?

You can go to location "50 00" to see things go wrong when coming back from the menu without calling onEnginePause.

As for pausing sounds, calling the pauseAll() of the mixer seems not to affect newly added sounds later. I see that sounds will automatically call stop() when a location is unloaded, so everything should be right..ish?

Bypassing the entity system for sounds does not seem to follow the design of the engine to me. The entities can implement handlers to react to the events that may happen, let's use them.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 10, 2018

@bgK Quick asking: what is the difference between StarkGlobal->getLevel() and StarkGlobal->getCurrent()->getLevel()?

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jun 10, 2018

Quick asking: what is the difference between StarkGlobal->getLevel() and StarkGlobal->getCurrent()->getLevel()?

StarkGlobal->getLevel() is the global level shared across all the locations.
StarkGlobal->getCurrent()->getLevel() is the level for the currently loaded location.

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-fmv branch from cb38bfe to 4f22f33 Jun 10, 2018

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-fmv branch from a755393 to af73387 Jun 11, 2018

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 11, 2018

@bgK I feel so stupid that it took me so long to finally understand what you meant... If you think the sound problem is solved and everything is good enough, I think it is time to merge it 😄

@bgK bgK merged commit f4007ce into residualvm:master Jun 12, 2018

1 check passed

default Build finished.
Details
@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jun 12, 2018

I think it is time to merge it

Indeed, thanks

@DouglasLiuGamer DouglasLiuGamer deleted the DouglasLiuGamer:branch-fmv branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.