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 keyboard bindings #1474

Merged
merged 27 commits into from Jul 24, 2018

Conversation

Projects
None yet
2 participants
@DouglasLiuGamer
Copy link
Collaborator

DouglasLiuGamer commented Jul 2, 2018

This PR is corresponding to implementing the keyboard bindings in TLJ.

@DouglasLiuGamer DouglasLiuGamer requested a review from bgK Jul 2, 2018

Common::Array<Resources::Item *> inventoryItems = inventory->listChildren<Resources::Item>(Resources::Item::kItemInventory);

int16 curItem = getSelectedInventoryItem();
int16 nextItem = curItem + step;

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jul 3, 2018

Author Collaborator

The original game has a very strange feature: when you start to cycle the inventory with nothing selected at the beginning, it will skip the first (or the last) item. This happens only in the start and the skipped item will show when you keep on pressing A or S. I can't think of a reasonable way to implement this and it feels like a "bug" to me, so I didn't do it here.

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Sounds good to me.

}

// Item None, Hand, Eye, Mouth are skipped
if (nextItem < 0 || (inventoryItems[nextItem]->isEnabled() && nextItem > 3)) {

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jul 3, 2018

Author Collaborator

Can I safely assume that the item None, Hand, Eye and Mouth are the first four items in the inventory?

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Yes, see:

if (i < 4 || !inventoryItems[i]->isEnabled()) continue;

/** Toggle subtitles on and off */
void requestToggleSubtitle() { _toggleSubtitle = !_toggleSubtitle; }
bool hasToggleSubtitleRequest() { return _toggleSubtitle; }
void performToggleSubtitle();

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jul 3, 2018

Author Collaborator

Based on the original game, the toggling of subtitles will be performed after the finishing of the current speech. So some kind of delay needs to be done here.

static Common::Point prevPos;

// Mouse Move event will always exists. Specifically handled here.
if (pos == prevPos && !_options.empty()) {

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jul 3, 2018

Author Collaborator

It seems that the mouse move event will always be there, even when the mouse is not moving at all, yet the dialog panel requires that players can use the up or down key to select options even when the mouse hovers on an option. I am not sure how onMouseMove() depends on the event so I only tackle this special case here.

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Ultimately I guess I'd be better if onMouseMove was not called when the mouse is not moving, and a onGameLoop event was added for things that need to be updated on each frame. The current approach is probably good enough for now.

Common::Point hotspot = getAnim()->getHotspotPosition((*element)->getIndex());
for (uint i = 0; i < pattables.size(); ++i) {
if (pattables[i]->getDefaultAction() == _exitAction) {
Common::Point hotspot = getAnim()->getHotspotPosition(i);

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jul 10, 2018

Author Collaborator

What is the usage of Object::getIndex() anyway? What's its difference with the index of a listed array like here?

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Object::getIndex is used to uniquely identify an entity in the list of children of the parent entity.

In this case, I don't know why the index in the array was used. It seems fragile to me.

@bgK
Copy link
Member

bgK left a comment

Thanks for your work! I've noticed a few issues when reviewing / testing that need to be resolved before this can be merged.

protected:
static const int32 _exitAction = 7;

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Please add an entry in PATTable::ActionType instead.

_toggleSubtitle = false;
}

void UserInterface::cycleInventory(int step) {

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

I'm not sure this is quite right. The KnowledgeSet entity maintains the order for the inventory items (the order in which they were picked up). IMO, it would be better to implement it as KnowledgeSet::getNeighboorInventoryItem(selectedItem, step) to be able to use the proper order.

}

// Item None, Hand, Eye, Mouth are skipped
if (nextItem < 0 || (inventoryItems[nextItem]->isEnabled() && nextItem > 3)) {

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Yes, see:

if (i < 4 || !inventoryItems[i]->isEnabled()) continue;

Common::Array<Resources::Item *> inventoryItems = inventory->listChildren<Resources::Item>(Resources::Item::kItemInventory);

int16 curItem = getSelectedInventoryItem();
int16 nextItem = curItem + step;

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Sounds good to me.

@@ -187,6 +225,8 @@ class UserInterface {
bool _interactive;
bool _interactionAttemptDenied;

bool _toggleSubtitle;

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Perhaps a more precise name could be found. Maybe _subtitlesToggleRequest or _shouldToggleSubtitles.

static Common::Point prevPos;

// Mouse Move event will always exists. Specifically handled here.
if (pos == prevPos && !_options.empty()) {

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Ultimately I guess I'd be better if onMouseMove was not called when the mouse is not moving, and a onGameLoop event was added for things that need to be updated on each frame. The current approach is probably good enough for now.

static Common::Point prevPos;

// Mouse Move event will always exists. Specifically handled here.
if (pos == prevPos && !_options.empty()) {

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

There is a small problem with this code, if the mouse is over the area where a dialog option will be at the moment it appears, then the option is not highlighted.

if (hoveredOption >= 0) {
_cursor->setCursorType(Cursor::kActive);
} else if (_scrollUpArrowVisible && _scrollUpArrowRect.contains(pos)) {
if (_scrollUpArrowVisible && _scrollUpArrowRect.contains(pos)) {
_cursor->setCursorType(Cursor::kActive);

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

I noticed the cursor flickers when hovering the scroll up / down arrows instead of being just kActive.

@@ -218,9 +227,103 @@ void StarkEngine::processEvents() {
_global->setFastForward();

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Escape should go back to the previous user interface screen when not in GameScreen.

Common::Array<ClickText*> _options;

static const uint32 _aprilColor = 0xFF00C0FF;
static const uint32 _otherColor = 0xFF4040FF;
static const uint32 _optionsTop = 4;
static const uint32 _optionsLeft = 30;
static const uint32 _optionsHeight = 71;
static const uint32 _optionsNum = 4;

This comment has been minimized.

@bgK

bgK Jul 10, 2018

Member

Options can wrap over multiple lines. Thus we cannot be sure there the last displayed option is the 4th one. I would prefer if this constant was removed and the code was based on the number of options that were displayed in renderOptions.

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-keyboard branch from 55400ab to 045b071 Jul 11, 2018

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-keyboard branch from b58d133 to 2453408 Jul 14, 2018

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jul 15, 2018

The commit 3e38b1c is used to propose a solution for a case like this, assuming that an option may wrap up more than one line.

options

In the original implementation (I mean the one before 2453408), when calculating how many options are displayed on the screen, an option will be added as long as there is remaining space, even though it will exceed it (for 9 pixels, to be specific).

However, for a case shown with the pic above, it will cause problems. If in that case, the DialogPanel scrolls down, according to the game's logic, the third option here will become the next "first visible option" and the fifth option here will become the next "last visible option". Yet there is a space of one line left, so the game should include one more option, which happens to wrap up two lines, in this case.

In the original implementation, that larger option will be included, which will cause a corner case that the last option will never show up. The game should first check whether the next option may completely fit into the remaining space, then add it to the list. To do so, the original height used to bound the options needs to be enlarged to ensure that all four lines of options can be included.

Actually, I am not sure whether this is how the original game handles this. So many things could happen in scrolling if it is possible to have options wrap up multiple lines. Is it really possible? Is there a location I can check on it in the game?

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jul 15, 2018

My understanding of the (mostly reverse engineered) initial code is that the game accommodates with options spanning two lines by scrolling options by increments of 3 even if 4 lines can be drawn. That way there can be one option spanning two lines without (too much) trouble.

I've used the French version to look for a dialog option taking two lines as that language is a bit more verbose than English:
before

After scrolling:
after

I don't think it's necessary to implement something too complicated that handles all the cases. Keeping the status-quo is fine with me.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jul 15, 2018

The game accommodates with options spanning two lines by scrolling options by increments of 3 even if 4 lines can be drawn.

This sounds like what the old implementation does, yet that implementation behaves differently with my Steam version. In the old implementation, if you scroll down to the bottom you'll have only 3 options displayed:

old

But in the Steam version, you should have a full four of them:

game

What did you mean by "status-quo"? The original "scrolling by 3" or the new one I proposed? The one I wrote is not very complicated, just some very basic checking, and it doesn't seem to decrease the performance. If there is no bug there I prefer to use the one I propose.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jul 15, 2018

The one I wrote is not very complicated, just some very basic checking, and it doesn't seem to decrease the performance. If there is no bug there I prefer to use the one I propose.

Sure ok.

@@ -282,6 +282,7 @@ void StarkEngine::processEvents() {
} else if (e.kbd.keycode == Common::KEYCODE_p) {
if (_userInterface->isInGameScreen()) {
pauseEngine(true);
debug("The game is paused");

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jul 15, 2018

Author Collaborator

Since we keep freezing the mouse when the game is paused and there is no notice for the player to see, I think it will be better to add a log to the console so that at least the player will have a way to know about this.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jul 15, 2018

@bgK If you think everything is good, I think it can be merged.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jul 23, 2018

I noticed that with this branch the dialog speech is not working anymore -- the dialogs are entirely silent. Can you take a look ?

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jul 24, 2018

@bgK That's strange, dialogs work fine with speeches on my computer. Where did you get a silent dialog?

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jul 24, 2018

Indeed, something I don't understand is happening. I was able to reproduce the silent dialogs twice tonight, but not anymore after that. There's no memory problems detected by Valgrind / ASan so I'll say it was a mistake on my end for now.

Thanks for your work; merging.

@bgK bgK merged commit 7832fcd into residualvm:master Jul 24, 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
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.