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 debug console #1456

Merged
merged 7 commits into from Jun 26, 2018

Conversation

Projects
None yet
2 participants
@DouglasLiuGamer
Copy link
Collaborator

DouglasLiuGamer commented Jun 24, 2018

Issue #1430 #1431 #1432 #1435

The previous PR is merged by accident. This is a new and clean one.

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

ArchiveLoader *archiveLoader = new ArchiveLoader();
ArchiveLoader *gameArchiveLoader = StarkArchiveLoader;
StarkArchiveLoader = archiveLoader;

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 24, 2018

Author Collaborator

I spent some time deciding how to asset these two indices. One way is to assert them in the actual requestLocationChange(), but the codes there seems to be hard to revert when indices are invalid. So I decided to add an additional load and check here. This will load the same resources twice if indices are correct, but codes are easier to read and since the debug console is an extra feature, it should not be an overhead.

This comment has been minimized.

@bgK

bgK Jun 24, 2018

Member

This seems too complicated to me for the value it brings. IMO, there should be something like StarkArchiveLoader->canLoadArchive(levelIndex, locationIndex) that simply checks if the location file exists.

Common::Array<Resources::Item*> inventoryItems = _inventory->listChildren<Resources::Item>(Resources::Item::kItemInventory);
Common::Array<Resources::Item*>::iterator it = inventoryItems.begin();
for (int i = 0; it != inventoryItems.end(); ++it, i++) {
if (printAll || (*it)->isEnabled()) {
warning("Item %d: %s", i, (*it)->getName().c_str());
g_engine->getDebugger()->debugPrintf("Item %d: %s\n", i, (*it)->getName().c_str());

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 24, 2018

Author Collaborator

I did a searching and it seems that this function, along with the enableInventoryItem(), are all only called by the debug console. I chose to keep them here but they may be moved to the body of corresponding command functions

This comment has been minimized.

@bgK

bgK Jun 24, 2018

Member

I think it would be better to move those two functions to the Console class. It looks weird using g_engine->getDebugger() here.

// Assert indices
Common::String xarcFileName = Common::String::format("%s/%s/%s.xarc", argv[1], argv[2], argv[2]);
if (!Common::File::exists(xarcFileName)) {
debugPrintf("Invalid location indices %s %s\n", argv[1], argv[2]);

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 25, 2018

Author Collaborator

The code is a lot simpler, but now we cannot display the range when indices are invalid. Will it be a problem?

This comment has been minimized.

@bgK

bgK Jun 25, 2018

Member

The error message could invite the user to use the listLocations command to obtain a list of valid locations.

return true;
}

bool printAll = argc != 2;

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 25, 2018

Author Collaborator

I kept this parameter as before but I am really confused about its meaning. The whole "adding an argument after listInventory will print out enabled inventory items" seems kind of weird. Can we change it to having a -all argument or a make a new listAllInventory command?

This comment has been minimized.

@bgK

bgK Jun 25, 2018

Member

Sure, feel free to improve this command. Another approach would be to always list all the inventory items and append (possessed) or something to the enabled inventory items.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 25, 2018

Some commands like listLocations and listInventory will generate many warnings in stdout which a text "Widget <> has clipping area y < 0". At first I thought it is due to not enough buffer but I'm not sure now. Any idea?

Also the current buffer is not enough for the listLocations command. Is it okay to enlarge the buffer of the debug console? Not sure about this since other games also used the same debugger base class.

@bgK
Copy link
Member

bgK left a comment

Some commands like listLocations and listInventory will generate many warnings in stdout which a text "Widget <> has clipping area y < 0". At first I thought it is due to not enough buffer but I'm not sure now. Any idea?

No idea, this happens in ScummVM as well. It's not specific to TLJ / Stark, but rather to the shared GUI code.

Also the current buffer is not enough for the listLocations command. Is it okay to enlarge the buffer of the debug console? Not sure about this since other games also used the same debugger base class.

That code is shared with ScummVM. If you want to change it, it's best to open a pull request there. The changes will be brought back here when we next merge the changes from ScummVM.

// Assert indices
Common::String xarcFileName = Common::String::format("%s/%s/%s.xarc", argv[1], argv[2], argv[2]);
if (!Common::File::exists(xarcFileName)) {
debugPrintf("Invalid location indices %s %s\n", argv[1], argv[2]);

This comment has been minimized.

@bgK

bgK Jun 25, 2018

Member

The error message could invite the user to use the listLocations command to obtain a list of valid locations.

return true;
}

bool printAll = argc != 2;

This comment has been minimized.

@bgK

bgK Jun 25, 2018

Member

Sure, feel free to improve this command. Another approach would be to always list all the inventory items and append (possessed) or something to the enabled inventory items.

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-debug branch from e972ec1 to 1c95eb1 Jun 26, 2018

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 26, 2018

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

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.