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

Peep debug tab #8558

Merged
merged 19 commits into from
May 6, 2019
Merged

Conversation

richard-fine
Copy link
Contributor

@richard-fine richard-fine commented Jan 5, 2019

Introduce a 'Debug tools' tab to the Guest window, when debugging tools are enabled:

image

For now it just shows a few bits of pathfinding data for that peep, but the idea is that this could be a home for all per-peep-related debugging info and tools (e.g. viewing/editing flags, forcing the peep into particular states, etc).

When debugging tools are not enabled, the tab is hidden.

@Gymnasiast
Copy link
Member

I have to say: I like the idea!
I'll try giving a more in-depth review later.

@Gymnasiast Gymnasiast self-requested a review January 5, 2019 12:42
@Gymnasiast
Copy link
Member

Apart from the minor line notes, there is major thing: you're completely bypassing the localisation system. This needs to be changed. You also need to set the correct label and text colours. There are many places where you can take a peek, but I think the ride statistics window is probably the tab most similar to what you need here.

If needed, I can answer questions or help with this.

@richard-fine
Copy link
Contributor Author

@Gymnasiast I skipped it because I figured debug tools maybe didn't need localizing. You're sure it's necessary?

@richard-fine
Copy link
Contributor Author

Also, if we are going to introduce a bunch of localized string IDs for this, then maybe it's worth having more of a discussion first about what info we actually want to display. I threw the current values into the window as a proof of concept as much as anything else.

@Gymnasiast
Copy link
Member

You're sure it's necessary?

Yes. Especially since you cannot set stuff like colours properly if you bypass it.

Also, if we are going to introduce a bunch of localized string IDs for this, then maybe it's worth having more of a discussion first about what info we actually want to display. I threw the current values into the window as a proof of concept as much as anything else.

It's not very difficult to change it afterwards. I think the current information is fine, but perhaps @duncanspumpkin (who did a lot of stuff with pathfinding) has more suggestions.

@richard-fine
Copy link
Contributor Author

Alright. I've fixed the minor issues, and I'll wait to see if @duncanspumpkin or anyone else has feedback on the info we actually want to see before I start allocating new string IDs.

@janisozaur
Copy link
Member

ping @zaxcav

@zaxcav
Copy link
Contributor

zaxcav commented Jan 5, 2019

I haven't done any active development on OpenRCT2 for well over a year, though I have been observing the amazing work all the active devs have been doing during this time. Great work all.
The A* pathfinding prototype is still waiting for me to get back to it. Maybe later this year, if it's still relevant.
If someone else is interested in looking at some aspects of the pathfinding I'm happy to answer questions if I can.

The current question, I believe, is what information might be useful to the peep debug tab?
The basic details that are available outside the search function are interesting but extremely minimal and honestly not very illuminating for debugging pathfinding issues. The search function itself generates a massive amount of data and then throws all this away at the end, returning only the chosen direction. It's a pity really, because the full chosen path (or even only the first n steps) would be really useful to have available, for multiple reasons not just debugging.

What would be useful to have in the peep debug tab would be the capability to set the peep's pathfinding data, as is suggested in the OP. That would make testing the pathfinding much easier - set up a peep as needed rather than waiting for an agreeable peep to pass by.

@ZehMatt
Copy link
Member

ZehMatt commented Jan 10, 2019

I've had a look into it, while useful information its lacking very basic things. One example would be perhaps a checkbox on the debug Tab to highlight path finding affected tiles with different colors or any other indicator sitting on the tile. And as zaxcav suggested there is no interactivity, would be cool to have a picker tool that allows us to set certain coordinates on those fields, better yet being able to modify all properties the pathfinding model contains.

@AaronVanGeffen
Copy link
Member

I've had a look into it, while useful information its lacking very basic things. One example would be perhaps a checkbox on the debug Tab to highlight path finding affected tiles with different colors or any other indicator sitting on the tile. And as zaxcav suggested there is no interactivity, would be cool to have a picker tool that allows us to set certain coordinates on those fields, better yet being able to modify all properties the pathfinding model contains.

These are some very good points, but shouldn't block the initial merge, IMO.

@ZehMatt
Copy link
Member

ZehMatt commented Jan 10, 2019

I'm not against it, however I feel like this is way to raw at this point, if it could maybe already provide a basic function set that would at least provide a better example where and how to put things if anyone else decides to further work on the path finding/AI, that would be a better start. Also I don't really like the font but that's probably because the localization was ignored which supports text attributes to make it more lightweight.

Edit: Also it might be perhaps better to try to map variables from a peep directly to UI components, if we used stronger types with metadata or something we could actually provide direct access for in-game editing, maybe that's worth a thought.

@Gymnasiast
Copy link
Member

I agree with @AaronVanGeffen that we shouldn't block the initial merge. After all, the improvements @ZehMatt mentions can be added later on. I do think it would be good to put these into an issue, so we don't forget about them.

{WWT_TAB, 1, 96, TabWidth + 96, 17, 43, IMAGE_TYPE_REMAP | SPR_TAB, STR_SHOW_GUEST_FINANCE_TIP}, // Tab 4
{WWT_TAB, 1, 127, TabWidth + 127, 17, 43, IMAGE_TYPE_REMAP | SPR_TAB, STR_SHOW_GUEST_THOUGHTS_TIP}, // Tab 5
{WWT_TAB, 1, 158, TabWidth + 158, 17, 43, IMAGE_TYPE_REMAP | SPR_TAB, STR_SHOW_GUEST_ITEMS_TIP}, // Tab 6
{WWT_TAB, 1, 189, TabWidth + 189, 17, 43, IMAGE_TYPE_REMAP | SPR_TAB, STR_DEBUG_TIP}, // Tab 7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at some of the other windows, all widgets which are the same for each tab pane are placed under a #define and re-used in each table.

@AaronVanGeffen
Copy link
Member

@richard-fine Are you still interested in working on this? Let us know if you need any help.

@richard-fine
Copy link
Contributor Author

Yeah, I've not been doing OpenRCT stuff lately but I'm still interested - was waiting to see if @duncanspumpkin would show up with any thoughts on what info to show, as suggested. At some point I'll get back to it and route the text via the localisation system.

@AaronVanGeffen
Copy link
Member

At the time he was mentioned, @duncanspumpkin was still on a road trip. While he was no doubt mentioned with the best of intentions, it is a bit unfortunate that you were under the impression that you had to wait for him.

He has since returned though, so while he might have some thoughts now, don't let that stop you. :-)

@duncanspumpkin
Copy link
Contributor

I'll review it tomorrow. Sorry didn't see it in my backlog. :)

Copy link
Contributor

@duncanspumpkin duncanspumpkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. There are tons of things we could expose in here. I'm not sure what would be best to expose though. I don't think there is much point showing x and next_x. I'd have to run some tests but I thought that was more internal peep->update values that end up being the same at the end of each update.

@@ -1951,7 +2062,7 @@ void window_guest_thoughts_resize(rct_window* w)
window_invalidate(w);
}

window_set_resize(w, 192, 159, 500, 450);
window_set_resize(w, 192 + (gConfigGeneral.debugging_tools ? TabWidth : 0), 159, 500, 450);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may as well make a separate function call for this to save the 7 repetitions of it.

rct_peep* peep = GET_PEEP(w->number);

set_format_arg(0, rct_string_id, peep->name_string_idx);
set_format_arg(2, uint32_t, peep->id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For anyone wondering what this code does it sets the global args to the peep name so that the peep name is correctly written at the top of the window.

gfx_draw_string(dpi, buffer, COLOUR_BLACK, x, y);
y += LIST_ROW_HEIGHT;

snprintf(buffer, sizeof(buffer), "Next: %u, %u, %u", peep->next_x, peep->next_y, peep->next_z);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these actually useful? next_x and x are usually the same. I can't quite remember the exact design of them.

Add a new tab to the Guest window which we can use to display debug information and tools for guests. At the moment it's blank and always visible; next step is to make it only show up when debugging tools are enabled.
Disable the debug tab in the guest window when the debugging tools are not turned on, causing it to be completely hidden from view.
richard-fine and others added 15 commits May 4, 2019 14:08
Introduce a named constant for the width of a tab in the Guest window, and touch the places that set the window width to add it to the minimum window width when the debugging tab is enabled, so that the tab doesn't render off the side of the window.
As a first debug stat to show, display a peep's current coordinates in the debug tab
Show more peep debug data in the Guest debug tab, mostly to help with understanding pathfinding behaviour.
Use sizeof(buffer) instead of repeating 4096 in a bunch of places. Also, 4096 was maybe a bit overkill, drop it down to 512 instead.
strcat_s is not available on all platforms, and we have a utility in the codebase that does basically the same thing already.
Use snprintf instead of _itoa as it's not available on all platforms. Also change the format specifies for unsigned variables to %u instead of %i, to be more correct...
For a static constant integer value, it's stylistically clearer to use constexpr instead of const. The resulting variable is implicitly const, but is also guaranteed to have a compile-time-computable value.
As suggested in review, it's a little easier to read this way.
I missed one... thankfully the CI did not.
src/openrct2-ui/windows/Guest.cpp Outdated Show resolved Hide resolved
@IntelOrca IntelOrca requested a review from Gymnasiast May 6, 2019 13:15
@IntelOrca IntelOrca merged commit 0d0479f into OpenRCT2:develop May 6, 2019
Gymnasiast added a commit that referenced this pull request May 10, 2019
AaronVanGeffen added a commit that referenced this pull request Jul 10, 2019
- Feature: [#485] Rides can now be simulated with ghost trains during construction.
- Feature: [#1260] Option for making giant screenshots have a transparent background.
- Feature: [#2339] Find local servers automatically when fetching servers.
- Feature: [#7296] Allow assigning a keyboard shortcut for the scenery picker.
- Feature: [#8029] Add the Hungarian Forint (HUF) to the list of available currencies.
- Feature: [#8481] Multi-threaded rendering.
- Feature: [#8558] Guest debugging tab.
- Feature: [#8659] Banner and sign texts are now shown in tooltips.
- Feature: [#8687] New multiplayer toolbar icon showing network status with reconnect option.
- Feature: [#8791] Improved tile element flag manipulation in Tile Inspector.
- Feature: [#8919] Allow setting ride price from console.
- Feature: [#8963] Add missing Czech letters to sprite font, use sprite font for Czech.
- Feature: [#9154] Change map toolbar icon with current viewport rotation.
- Change: [#7877] Files are now sorted in logical rather than dictionary order.
- Change: [#8427] Ghost elements now show up as white on the mini-map.
- Change: [#8688] Move common actions from debug menu into cheats menu.
- Change: [#9428] Increase maximum height of the Hypercoaster to RCT1 limits.
- Fix: [#2294] Clients crashing the server with invalid object selection.
- Fix: [#4568, #5896] Incorrect fences removed when building a tracked ride through
- Fix: [#5103] OpenGL: ride track preview not rendered.
- Fix: [#5889] Giant screenshot does not work while using OpenGL renderer.
- Fix: [#5579] Network desync immediately after connecting.
- Fix: [#5893] Looking at guest window tabs other than the main tab eventually causes assertion.
- Fix: [#5905] Urban Park merry-go-round has entrance and exit swapped (original bug).
- Fix: [#6006] Objects higher than 6 metres are considered trees (original bug).
- Fix: [#7039] Map window not rendering properly when using OpenGL.
- Fix: [#7045] Theme window's colour pickers not drawn properly on OpenGL.
- Fix: [#7323] Tunnel entrances not rendering in 'highlight path issues' mode if they have benches inside.
- Fix: [#7729] Money Input Prompt breaks on certain values.
- Fix: [#7884] Unfinished preserved rides can be demolished with quick demolish.
- Fix: [#7913] RCT1/RCT2 title sequence timing is off.
- Fix: [#7700, #8079, #8969] Crash when unloading buggy custom rides.
- Fix: [#7829] Rotated information kiosk can cause 'unreachable' messages.
- Fix: [#7878] Scroll shortcut keys ignore SHIFT/CTRL/ALT modifiers.
- Fix: [#8219] Faulty folder recreation in "save" folder.
- Fix: [#8480, #8535] Crash when mirroring track design.
- Fix: [#8507] Incorrect change in vehicle rolling direction.
- Fix: [#8537] Imported RCT1 rides/shops are all numbered 1.
- Fix: [#8553] Scenery removal tool removes fences and paths while paused.
- Fix: [#8598] Taking screenshots fails with some park names.
- Fix: [#8602] Wall piece collision detection deviates from vanilla
- Fix: [#8649] Setting date does not work in multiplayer.
- Fix: [#8873] Potential crash when placing footpaths.
- Fix: [#8882] Submarine Ride does not count as indoors (original bug).
- Fix: [#8900] Peep tracking is not synchronized.
- Fix: [#8909] Potential crash when invoking game actions as server.
- Fix: [#8947] Detection of AVX2 support.
- Fix: [#8988] Character sprite lookup noticeably slows down drawing.
- Fix: [#9000] Show correct error message if not enough money available.
- Fix: [#9067] Land/water tools show prices when money is disabled.
- Fix: [#9124] Disconnected clients can crash the server.
- Fix: [#9132] System file browser cannot open SV4 files.
- Fix: [#9152] Spectators can modify ride colours.
- Fix: [#9202] Artefacts show when changing ride type as client or using in-game console.
- Fix: [#9240] Crash when passing directory instead of save file.
- Fix: [#9245] Headless servers apply Discord Rich Presence.
- Fix: [#9293] Issue with the native load/save dialog.
- Fix: [#9322] Peep crashing the game trying to find a ride to look at.
- Fix: [#9324] Crash trying to remove invalid footpath scenery.
- Fix: [#9402] Ad campaigns disappear when you save and load the game.
- Fix: [#9411] Ad campaigns end too soon.
- Fix: [#9476] Running `simulate` command on park yields `Completed: (null)`.
- Fix: [#9520] Time Twister object artdec29 conversion problem.
- Fix: Guests eating popcorn are drawn as if they're eating pizza.
- Fix: The arbitrary ride type and vehicle dropdown lists are ordered case-sensitively.
- Improved: [#6116] Expose colour scheme for track elements in the tile inspector.
- Improved: Allow the use of numpad enter key for console and chat.
@richard-fine richard-fine deleted the feature/peep-inspector branch September 20, 2020 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants