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: Improve the main menu #1467

Merged
merged 10 commits into from Jun 30, 2018

Conversation

Projects
None yet
2 participants
@DouglasLiuGamer
Copy link
Collaborator

DouglasLiuGamer commented Jun 29, 2018

Implement the version info text (#1439) and the Book of Secrets in the main menu.

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

return _visual->get<VisualImageXMG>();
}

VisualText *RenderEntry::getText() const {
if (!_visual) {
return nullptr;

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 29, 2018

Author Collaborator

In the implementation of GameWindow::onScreenChanged() I came across with render entries with no Visual. It feels strange to me since a RenderEntry without a Visual seems pretty useless. But anyway it needs to be handled.

@@ -85,6 +85,15 @@ class Settings {
/** Check whether low-resolution fmv is available */
bool hasLowResFMV() { return _hasLowRes; }

/** Enable the book of secrets */
void enableBookOfSecrets() {
ConfMan.setBool("xoBfOsterceS", true);

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 29, 2018

Author Collaborator

Just follow the name in "w_world.ini" :p

void quitToMainMenu();
void requestQuitToMainMenu() { _quitToMainMenu = true; }
bool hasQuitToMainMenuRequest() { return _quitToMainMenu; }
void performQuitToMainMenu();

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 29, 2018

Author Collaborator

Script needs to request quitting to the main menu. In the previous implementation, immediately quitting will trigger the deallocation of the Script object and therefore crash the game and it took me an eternity to realize this!!! . So it is cached here and is performed later in the update part of the game's main loop.

This comment has been minimized.

@bgK

bgK Jun 30, 2018

Member

Building with Address Sanitizer enabled helps a lot to understand these issues. It tells precisely the cause of the use after free. It's probably a bit of a pain to get it working on Windows, and IMO it's well worth having a Linux install somewhere just to be able to use it when needed.

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 30, 2018

Author Collaborator

Yeah now I know the good of Linux...

if (!StarkSettings->isDemo() && StarkSettings->hasBookOfSecrets()) {
StarkUserInterface->changeScreen(kScreenGame);
StarkResourceProvider->initGlobal();
StarkScene->setFadeLevel(1.0f);

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 29, 2018

Author Collaborator

The "Book of Secrets" will not handle the fade level by itself. I don't find a problem setting it here before the actual game starts.

This comment has been minimized.

@bgK

bgK Jun 29, 2018

Member

Shouldn't the fade level be initialized to 1.0 in the Scene class rather than here?

VersionInfoText::VersionInfoText() :
StaticLocationWidget("VERSION INFO REALLY", nullptr, nullptr) {
Common::String text = _copyrightSymbol + Common::String("1999 Funcom");

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 29, 2018

Author Collaborator

Since we cannot access the build version of the original executable and we are not using the Audiere library, I chose to just display the Funcom text.

Common::String text = _copyrightSymbol + Common::String("1999 Funcom");
_renderEntry->getText()->setText(text);
_renderEntry->setPosition(Common::Point(_posX, _posY));

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 29, 2018

Author Collaborator

The default positions of the "VERSION INFO HERE" widget in the released game and the demo are different, and both of them needs to be moved, so I chose to hard-code it.

for (uint i = 0; i < layers.size(); ++i) {
renderEntries = layers[i]->listRenderEntries();
for (uint j = 0; j < renderEntries.size(); ++j) {
text = renderEntries[j]->getText();

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 29, 2018

Author Collaborator

Texts in not only the current layer but all the layers within the same location need to reset their textures when the resolution has changed, otherwise, the user may change the screen, go back to a previous layer and find size-unmatched texts.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 29, 2018

funcom
A screenshot of the current version info text for reference

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 29, 2018

A quick way to test the Book of Secrets: Ensure that the "xoBfOsterceS" is not set in the configuration, use the debug console to change to location 3c 00, enable inventory item 15, and plant the organic leaf in the flower bed. The Book of Secrets will then be enabled.

@bgK
Copy link
Member

bgK left a comment

This is looking Ok. I've added a few very minor comments.

if (!StarkSettings->isDemo() && StarkSettings->hasBookOfSecrets()) {
StarkUserInterface->changeScreen(kScreenGame);
StarkResourceProvider->initGlobal();
StarkScene->setFadeLevel(1.0f);

This comment has been minimized.

@bgK

bgK Jun 29, 2018

Member

Shouldn't the fade level be initialized to 1.0 in the Scene class rather than here?

virtual ~VersionInfoText() {}

private:
static const int _copyrightSymbol = -87, _posX = 16, _posY = 419;

This comment has been minimized.

@bgK

bgK Jun 29, 2018

Member

_copyrightSymbol is the ISO-8859-1 code for the copyright symbol. It would be easier to understand what it is as a hexadecimal number (0xA9).

Also please don't put the position on the same line, they are really unrelated.

@@ -247,6 +247,11 @@ void StarkEngine::updateDisplayScene() {
// Clear the screen
_gfx->clearScreen();

// Quit to main menu screen, if needed
if (_userInterface->hasQuitToMainMenuRequest()) {

This comment has been minimized.

@bgK

bgK Jun 29, 2018

Member

This check feels like it does not belong here. Couldn't it be in StarkEngine::mainLoop with the quit check and the location change check?

Common::Array<Resources::Layer *> layers = StarkGlobal->getCurrent()->getLocation()->listLayers();

VisualText *text = nullptr;
Gfx::RenderEntryArray renderEntries;

This comment has been minimized.

@bgK

bgK Jun 29, 2018

Member

Please reduce the scope of these variables. That'd makes the code a bit easier to read, IMO.

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 30, 2018

Author Collaborator

Do you mean putting the declaration into the loop? I am not sure how compilers can optimize this these days but isn't that may potentially cause unnecessary variable creations and deallocations, especially since we have an array here?

This comment has been minimized.

@bgK

bgK Jun 30, 2018

Member

C++ compilers are very good with these little details. For non performance critical code it's better to focus on writing simple and readable code.

In this particular case, constructing RenderEntryArray outside of the loop may prevent Return Value Optimization.

DouglasLiuGamer added some commits Jun 30, 2018

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 30, 2018

@bgK If there's nothing left I think we can do the merge.

@bgK bgK merged commit a03cdea into residualvm:master Jun 30, 2018

1 check passed

default Build finished.
Details
@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jun 30, 2018

Thanks!

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.