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 main menu #1415

Merged
merged 25 commits into from May 21, 2018

Conversation

Projects
None yet
3 participants
@DouglasLiuGamer
Collaborator

DouglasLiuGamer commented May 15, 2018

This pull request is corresponding to the task of implementing the main menu in TLJ.

The first attempt I just did some simple things.

I move onScreenChanged() to the StaticLocationScreen since I think all of the menus will need to resize text.

Now I just make a simple MainMenuScreen class with nothing in it and let the game load it when it starts, so that I can use the debug mode to dump the resources. But now there is a major problem. When I do this, an assertion failed in resourcereference.cpp, line 61, as below

assert(resource->getIndex() == element.getIndex());

I printed out those values. Most of the time both of them are zeros but at a point element.getIndex() becomes 69.

I guess this is related to how TLG loads its resources and now I am still not very clear about it.

FYI, I add the line _currentScreen->open() in UserInterface::init(), which I think is necessary. The reason why it wasn't there before, I guess, is that the game will trigger a video to play when it starts, which triggers thechangeScreen() and therefore trigger the open().

I made some stupid things in sync my branch and a typo in the name of commits. Hope it is fine.

@bgK

This comment has been minimized.

Member

bgK commented May 15, 2018

But now there is a major problem. When I do this, an assertion failed in resourcereference.cpp, line 61.

What happens is indeed related to how the engine loads its resources. Resources can be loaded from two kinds of resource trees. The world resource tree and the static resource tree. You changed the user interface code to open the main menu which opens a static location that needs resources from the static tree. When a static location is loaded the engine will only load resources from the static tree. The code that starts a new game upon engine startup is still there. It tries to load world resources which fails.

setStartupLocation();

The call to setStartupLocation should be replaced with something that opens the main menu. The code in setStartupLocation will be useful for the new game button on the main menu.

I made some stupid things in sync my branch and a typo in the name of commits. Hope it is fine.

What a great occasion to make yourself familiar with the interactive rebase feature of git. You should be able to rebase your branch on top of master and remove the unwanted commits with it. Read some documentation about it to understand it. But the gist is to run git rebase -i origin/master and then delete the unwanted commits in the text editor. That will reapply the commits you did not delete on top of the latest version of the code.
Worst case scenario, you can still recover your commits from your github branch.
Also, I suggest you setup git to rebase instead of merging when pulling to avoid the creation of merge commits ($ git config branch.master.rebase true).

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 16, 2018

I have rebased my branches to clean up the mess I left before now. Thanks for the help 😄

Right now the game shall open with the main menu. But now there is another major problem: For some reason, all buttons on the main menu separate the clicking area and the helping text. Due to this reason, the original isMouseInside() is not able to correctly check the bounding area since it just check image, smacker and text, while those buttons' areas contain only text and are very tiny. Right now I am not sure how to get the original bounding area. I tried to compare the resources dumped from main menu and diary index but there is nothing remarkable.

There is another small problem: there are two special widgets on the main menu: "VERSION INFO" and "VERSION INFO REALLY". Strange names and the current version renders them differently compared to the steam version, as below:

Steam version:
steam

ResidualVM version:
vm

Seems like those texts are generated by other things. Not sure about this.

@bgK

This comment has been minimized.

Member

bgK commented May 16, 2018

Those buttons' areas contain only text and are very tiny. Right now I am not sure how to get the original bounding area.

Seems like the buttons use an empty VisualText to mark the clickable areas. What happens is that VisualText::createTexture recomputes the visual size based on the text content. In this case it is empty, resulting in very small rects. VisualText::createTexture needs to change to use the existing rect width and height if it is defined in the resources instead of computing them from the text content.

Seems like those texts are generated by other things. Not sure about this.

Yes, this is a special case that should be handled in the main menu code.

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 16, 2018

The VisualText is modified to properly set the bounding area when the text is blank. I choose to actually check the string instead of hardcoded so as to prevent unexpected cases in the game data, although I believe this will not happen.

I'll add the version information issue into the project and tackle it later.

@@ -144,4 +159,13 @@ void VisualText::resetTexture() {
createTexture();
}
bool VisualText::isBlank() {
for (auto c : _text) {
if (c != ' ' && c != '\r' && c != '\n' && c != '\t') {

This comment has been minimized.

@bgK

bgK May 16, 2018

Member

You can use isSpace from util.h instead.

@@ -144,4 +159,13 @@ void VisualText::resetTexture() {
createTexture();
}
bool VisualText::isBlank() {
for (auto c : _text) {

This comment has been minimized.

@bgK

bgK May 16, 2018

Member

We can't use c++11 features yet. That means no foreach and no auto :(

ResidualVM follows the same rules as ScummVM, and we don't do c++11 there either. This is mostly because the CI server has not yet been updated with c++11 compatible compilers for all platforms.

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 17, 2018

Collaborator

I see that the override specifier is used heavily so I thought c++11 is available. Looks like the c++11-compat.h redefines it. Neat.

@@ -62,6 +63,9 @@ class VisualText : public Visual {
void createTexture();
void freeTexture();
// Check whether the text is blank

This comment has been minimized.

@bgK

bgK May 16, 2018

Member

Please use Doxygen style comments when documenting methods/classes (so they appear in http://doxygen.residualvm.org/).

"NewGame",
nullptr,
nullptr));
_widgets.back()->setupSounds(0, 1);

This comment has been minimized.

@bgK

bgK May 16, 2018

Member

Please fix the whitespaces here. We use tabs for indentation and spaces for alignment (see http://wiki.scummvm.org/index.php/Code_Formatting_Conventions).

_originalRect.right = _originalRect.left + maxScaledLineWidth;
_originalRect.bottom = _originalRect.top + StarkGfx->scaleHeightOriginalToCurrent(_targetHeight);
_originalRect.right = _originalRect.left + _targetWidth;
_originalRect.bottom = _originalRect.top + _targetHeight;

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 17, 2018

Collaborator

The previous implementation is wrong. The width and height should not be scaled according to the resolution since the testing of the bounding box is based on original resolution.

@bgK

This comment has been minimized.

Member

bgK commented May 17, 2018

This is working quite well now 👍

@@ -67,9 +69,14 @@ UserInterface::~UserInterface() {
void UserInterface::init() {
_cursor = new Cursor(_gfx);
_currentScreen = _gameScreen = new GameScreen(_gfx, _cursor);
_mainMenuScreen = new MainMenuScreen(_gfx, _cursor);

This comment has been minimized.

@bgK

bgK May 17, 2018

Member

_mainMenuScreen is never freed (memory leak).

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 17, 2018

Collaborator

Fixed.

@@ -0,0 +1,111 @@
#include "engines/stark/ui/menu/mainmenu.h"

This comment has been minimized.

@bgK

bgK May 17, 2018

Member

Please include the standard ResidualVM licence header for new files.

@@ -97,7 +97,7 @@ class StarkServices : public Common::Singleton<StarkServices> {
#define StarkGameInterface StarkServices::instance().gameInterface
#define StarkUserInterface StarkServices::instance().userInterface
#define StarkFontProvider StarkServices::instance().fontProvider
#define StarkGameDescription StarkServices::instance().gameDescription
#define StarkGameDescription StarkServices::instance().gameDescription

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 17, 2018

Collaborator

Add this field so that MainMenuScreen may check the game version.

@@ -180,6 +180,9 @@ void StaticLocationWidget::onClick() {
_soundMouseClick->play();
}
// Ensure the click sound is played completely
while (_soundMouseClick->isPlaying()) {};

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 17, 2018

Collaborator

Based on the original game (steam version here), the click sound is completely played before the action is executed. Although using while loop here will freeze the game, that will also happen in the original game, so this should be enough "original...ish".

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 17, 2018

Collaborator

Wait the pointer needs to be checked beforehand.

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 17, 2018

I am planning to implement the Quit and Credits, but I am not sure how to find out their corresponding resources.

For Credits I need to load the credits fmv but right now I have no idea how to find the name. As for Quit, according to the original game (steam version), when the user hits the button, there will be a message box coming up to let user chooses "yes" or "no" with either mouse or keyboard, no idea how to find those resources. I can't screenshot the message box since the steam version has problems rendering it properly.

@bgK Maybe we can discuss on email if the solution is a bit complex.

@bgK

This comment has been minimized.

Member

bgK commented May 17, 2018

I don't have enough time to write a full answer right now. The credits video is global\xarc\0e02.bbb. The Funcom logo that plays before the main menu is displayed is global\xarc\1402.bbb. Both are hardcoded in the game executable. I don't think there are resource descriptors.

There are a few other confirmation dialog boxes. I believe those are actually skinned Windows dialogs. We'll have to come up with a replacement solution. Perhaps we could use ResidualVM GUI dialogs.

@@ -132,6 +132,12 @@ void MainMenuScreen::helpTextHandler(StaticLocationWidget &widget, const Common:
}
}
void MainMenuScreen::creditsHandler() {
if (!isDemo()) {
StarkUserInterface->requestFMVPlayback("0e02.bbb");

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 18, 2018

Collaborator

There is no 0e02.bbb in the demo game data. So demo has no credits video or with a different name?

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 18, 2018

One small and strange yet a little bit funny thing to ask: in my steam version game, before the FunCom logo video, there is a screen that fades very quickly with a text "Please Wait...". Do we need to make that too? 😂😂😂

@@ -171,6 +178,8 @@ void StaticLocationWidget::onClick() {
if (_soundMouseClick) {
_soundMouseClick->play();
// Ensure the click sound is played completely
while (_soundMouseClick->isPlaying()) {};

This comment has been minimized.

@bgK

bgK May 18, 2018

Member

It's not good to busy-wait for something to complete. The game appears unresponsive while waiting and the CPU is used at 100%.
In game engine design it's best to have a single game loop that roughly checks for user input, updates the simulated world and then renders. So far, Stark has managed to follow that rule. So here, we'll need to find another solution to let the sound fully play. Perhaps something is stopping the sound, preventing it from completing while it should not. Perhaps this code needs to wait for the sound to complete by checking each frame if it is finished.

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 18, 2018

Some names are changed. Just my personal preference.
The overall idea of fixing the busy-wait of clicking sound is to delay the performance of changing the displayed screen. Now changeScreen() only issues a request of changing the screen without actually changing it. The UserInterface will handle the change only when the required sound stops playing.
Due to this change, right now any StaticLocationWidget is not encouraged to perform anything right after the call to changeScreen(). Therefore, the MainMenuScreen makes a new field called _closeAction to store a pointer to a function, which handles what needs to be done after the change of screen and will be called in the close() of the screen.
I understand it is generally bad to have busy-waiting, but I kind of doubt how much benefit we gain from such a large change...

Commits are discarded.

@bgK

This comment has been minimized.

Member

bgK commented May 18, 2018

I understand it is generally bad to have busy-waiting, but I kind of doubt how much benefit we gain from such a large change...

You're right, this is quite a mess. Maybe it's better to just synchronously wait. But in that case there should at least be a delay in the loop body to keep the CPU usage down.

_diaryIndexScreen->onScreenChanged();
if (!isInGameScreen()) {
_currentScreen->onScreenChanged();

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 20, 2018

Collaborator

There will be more Screen added to UserInterface later. Since StaticLocationScreen will release all the resources when it is closed, there is no need to notify the change of screen resolution for a not currently displayed screen.

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 20, 2018

One problem: even though now the game will "quit" to the main menu, if choose to start the game again, the game will just go back to the previous state, not start a completely new one. Is there a way to reset the whole GameScreen, or maybe something to twist in ResourceProvider?

@bgK

This comment has been minimized.

Member

bgK commented May 20, 2018

Hi

Is there a way to reset the whole GameScreen, or maybe something to twist in ResourceProvider?

The code for loading a saved game should contain everything you need.

_userInterface->skipFMV();

It basically clears the current state, initializes a new empty state and then loads a save.

changeScreen(Screen::kScreenGame);
StarkResourceProvider->shutdown();
StarkResourceProvider->initGlobal();
changeScreen(Screen::kScreenMainMenu);

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 20, 2018

Collaborator

It feels like here should have a StarkResourceProvider->setShouldRestoreCurrentState();, but that would corrupt the game when it starts again. Kind of want to know why.

This comment has been minimized.

@bgK

bgK May 20, 2018

Member

It would make more sense if StarkResourceProvider->initGlobal(); was not called here (nor on engine startup when going to the main menu), but when starting a new game.

StarkResourceProvider->setShouldRestoreCurrentState(); is part of restoring a saved game. It asks the resource provider to restore the game state part that is specific to the current location. There's no need to call it here nor when starting a new game.

// Initialize the UI
_userInterface->init();
if (ConfMan.hasKey("save_slot")) {
// Load game from specified slot, if any
loadGameState(ConfMan.getInt("save_slot"));

This comment has been minimized.

@bgK

bgK May 20, 2018

Member

Ultimately, it should still be possible to load a save from the ResidualVM launcher whithout going through the main menu.

// Ensure the click sound is played completely
while (_soundMouseClick->isPlaying()) {
g_system->delayMillis(10);
};

This comment has been minimized.

@sev-

sev- May 20, 2018

You need to add g_system->updateScreen() here too, otherwise you could get artefacts or jerky mouse movements.

#Ignore testing .dll files
libogg.dll
libvorbis.dll
libvorbisfile.dll

This comment has been minimized.

@sev-

sev- May 20, 2018

No newline at the end of the file

@DouglasLiuGamer

This comment has been minimized.

Collaborator

DouglasLiuGamer commented May 21, 2018

A rebasing has been done to sync with the master.

if (ConfMan.hasKey("save_slot")) {
// Load game from specified slot, if any
_resourceProvider->initGlobal();

This comment has been minimized.

@bgK

bgK May 21, 2018

Member

Those two calls are redundant now. loadGameState already calls them.

@bgK bgK merged commit a6cc6e7 into residualvm:master May 21, 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