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

Tile Inspector UX Improvements #7548

Merged
merged 8 commits into from
May 25, 2018

Conversation

Broxzier
Copy link
Member

This PR makes the list in the Tile Inspector more consistent with other windows in terms of looks. Black text, no dark/light for odd/even rows, white text on hover, and darker background colour.

The UX has also been improved, by making the window automatically resize when clicking on something from the list, as suggested by #5832, and by allowing the player to Ctrl+Click on tile elements to automatically select them.

This PR is also aimed to make the code a bit more flexible. It removes two of the intents that were made for it, automatically sets the correct page, and the pressed widgets get reset on switching. With these changes, two small issues have been fixed (after pasting an element, the page for the pasted element would not be shown, and seemingly at random buttons stay pressed).

@AaronVanGeffen AaronVanGeffen added the network version Network version needs updating - double check before merging! label May 21, 2018
- Improved: [#7302] Raising land near the map edge makes the affected area smaller instead of showing an 'off edge map' error.
- Improved: [#7435] Object indexing now supports multi-threading.
- Improved: [#7510] Add horizontal clipping to cut-away view options.
- Improved: [#7531] "Save track design" dropdown now stays open.
- Improved: [#7548] Add Ctrl + Click functionality to select tile elements with the tile inspector.
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify here how this new function differs from the old one?

Copy link
Member Author

@Broxzier Broxzier May 21, 2018

Choose a reason for hiding this comment

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

I find it difficult to summerize this in few words. The current way of selecting elements is first clicking on the surface, and then on the element in the list. to simplify that, you can now Ctrl + Click on any element, and it will select the tile that the element is on and select the element in the list.

If you have any suggestions, please let me know!

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that helps. Perhaps something like "Ctrl-clicking an element now directly selects it and its tile in the tile inspector."? Although that suggests you don't need to have the tile inspector open, perhaps.

On a side note, I think we've substituted Ctrl shortcuts with Cmd ones on macOS. Is that taken into account here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that's better already. "Ctrl-Clicking with the tile inspector open now now directly selects the element and its tile." Perhaps using "Ctrl/Cmd + Clicking" instead it clearer for MacOS users? Or is it normal to refer to the Cmd key with Ctrl?

The code cheks for PLACE_OBJECT_MODIFIER_COPY_Z rather than for Ctrl input directly.

Copy link
Member

Choose a reason for hiding this comment

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

The code cheks for PLACE_OBJECT_MODIFIER_COPY_Z rather than for Ctrl input directly.

Ah, I don't think that's been adjusted for macOS. Should be fine to just refer to Ctrl, then. So, "Ctrl-clicking with the tile inspector open now directly selects an element and its tile."?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I've amended the commit.

@AaronVanGeffen
Copy link
Member

As this PR changes the intent list, I believe it should increment the network version as well.

@Broxzier Broxzier force-pushed the improve/tile-inspector branch 2 times, most recently from a904c89 to 50a8054 Compare May 21, 2018 22:58
Copy link
Member

@Gymnasiast Gymnasiast left a comment

Choose a reason for hiding this comment

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

If anything should have zebra striping, it's the tile inspector IMO.

@Broxzier
Copy link
Member Author

@Gymnasiast I don't see how this is different from the load/save window, and zebra stripes were removed from that list.

@AaronVanGeffen
Copy link
Member

For what it's worth, I also think the zebra stripes aren't necessarily for this window.

@Gymnasiast
Copy link
Member

@Gymnasiast I don't see how this is different from the load/save window, and zebra stripes were removed from that list.

And? Doesn't prove that it improves things, just that more dialogs have been ****ed up in order to be "more consistent". But even it was a good thing for the load/save window (and I doubt that!), the tile inspector has more columns, making the benefits of zebra striping bigger.

@Broxzier
Copy link
Member Author

The way you say it is as if you're attacking me for daring to make that change. My argument was meant to explain why I made the change. Since the zebra stripes were removed from the load/save window, the tile inspector and title sequence editor are the only ones still using them, so I figured the majority would be in favour of them being removed. Personally I'm in favour of keeping them. In fact I'd like to see them used for all other lists (rides, guests, staff, load/save, title sequence editor, and probably some others). If the majority agrees, I'm more than happy to reintroduce them.

@Gymnasiast
Copy link
Member

The way you say it is as if you're attacking me for daring to make that change.

That's the way you're reading it. My point is that the stripes being removed from other windows boils down to 'all the kids are doing it'.

Basically, this decreases readability, just because apparently people think they look bad. Which is not a very convincing argument if you ask me.

@AaronVanGeffen
Copy link
Member

AaronVanGeffen commented May 22, 2018

The contrast between the zebra stripes as we know them is a bit high. I think that's why people don't really like them.

We recently compromised on a similar issue for #7420. The tile inspector and the object error windows share their colour palette, by default. Perhaps the same can be done here?

@Broxzier Broxzier mentioned this pull request May 22, 2018
@Broxzier
Copy link
Member Author

I made it exactly like the window from #7420 (right), and then a version with less contrast between the rows (left). I went with the left one.

image

@Gymnasiast ping.

@Gymnasiast
Copy link
Member

I prefer the right one. Perhaps a contrast value that is the average between the two would be an idea?

@AaronVanGeffen
Copy link
Member

AaronVanGeffen commented May 24, 2018

Hmm, I personally prefer the left one. The left one uses some dithering techniques to get its colours, but it looks alright to me. I'm afraid combining the left and right versions will lead to the window looking like this, however, which makes the text look too fuzzy to me.

@Gymnasiast
Copy link
Member

Right, I'll comprise. Let's go with the left one then.

@@ -343,20 +343,6 @@ class WindowManager final : public IWindowManager
window_tile_inspector_clear_clipboard();
break;

case INTENT_ACTION_SET_TILE_INSPECTOR_PAGE:
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It removes two of the intents that were made for it, automatically sets the correct page, and the pressed widgets get reset on switching.

Copy link
Member

@Gymnasiast Gymnasiast left a comment

Choose a reason for hiding this comment

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

Ok, looks reasonable.

Copy link
Member

@AaronVanGeffen AaronVanGeffen left a comment

Choose a reason for hiding this comment

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

Network version requires re-bumping before merge.

Broxzier and others added 8 commits May 25, 2018 00:46
- No odd/even checked colours
- Black text (white on hover)
- Darker background
- Solid fill background for hovered and selected element
Instead of having to set the page and setup the widgets correctly when a new elements gets selected, it now switches automatically on invalidation to the correct tab. This fixes the issue where pasted elements were selected but the page for them not shown, and removes two of the intents that were made for the tile inspector.
In the invalidate function of the tile inspector, the checkboxes get checked to match the selected tile element. Some of those checkboxes share their widget index with buttons from other pages. When a checkbox is checked, the it's set to be "pressed", which causes the butons in the other tabs so appear that way. This behaviour is fixed by unpressing widgets when switching to another page.
Co-authored-by: wolfreak99 <jbminor1991@gmail.com>
@Broxzier Broxzier merged commit 7be4072 into OpenRCT2:develop May 25, 2018
@Broxzier Broxzier deleted the improve/tile-inspector branch May 27, 2018 13:35
janisozaur added a commit that referenced this pull request Jun 10, 2018
- Feature: [#1417] Allow saving track designs for flat rides.
- Feature: [#1675] Auto-rotate shops to face footpaths.
- Feature: [#3473] Add button in ride window's maintainance tab to refurbish the ride.
- Feature: [#6510] Ability to select edges or a row of tiles by holding down Ctrl using the land tool.
- Feature: [#7187] Option for early scenario completion.
- Feature: [#7266] Make headless instances use an interactive terminal with access to the in-game console API.
- Feature: [#7267] Leverage more historical data in Finances window.
- Feature: [#7316] Cheat to allow freezing all staff
- Feature: [#7332] Keyboard shortcuts for view path issues and cutaway view.
- Feature: [#7348] Add large half loops to the Vertical Drop Roller Coaster.
- Feature: [#7459] Allow opening and closing of parks that use no money.
- Feature: [#7579] New horizontal +/- spinner widgets to make adjusting values easier.
- Fix: [#2053] When clearance checks are disabled, a track piece ghost can remove non-ghost track pieces.
- Fix: [#2611] Some objects show (undefined string) instead of a description in Korean.
- Fix: [#3596] Saving parks, landscapes and tracks with a period in the filenames don't get their extension.
- Fix: [#5210] Default system dialog not accessible from saving landscape window.
- Fix: [#6134] Scenarios incorrectly categorised when using Polish version of RCT2.
- Fix: [#6141] CSS50.dat is never loaded.
- Fix: [#6647] Changelog window causes FPS drop.
- Fix: [#6938] Banner do not correctly capitalise non-ASCII characters.
- Fix: [#7176] Mechanics sometimes fall down from rides.
- Fix: [#7303] Visual glitch with virtual floor near map edges.
- Fix: [#7313] Loading an invalid path with openrct2 produces results different than expected.
- Fix: [#7327] Abstract scenery and stations don't get fully See-Through when hiding them (original bug).
- Fix: [#7331] Invention list in scenario editor crashes upon removing previously-enabled ride/stall entries.
- Fix: [#7341] Staff may auto-spawn on guests walking outside of paths.
- Fix: [#7354] Cut-away view does not draw tile elements that have been moved down on the list.
- Fix: [#7358] Peeps and staff entering rides still have the slope speed penalty set.
- Fix: [#7382] Opening the mini-map reverts the size of the land tool to 1x1, regardless of what was selected before.
- Fix: [#7402] Edges of neigbouring footpaths stay connected after removing a path that's underneath a ride entrance.
- Fix: [#7405] Rides can be covered by placing scenery underneath them.
- Fix: [#7418] Staff walk off paths with a connection but no adjacent path.
- Fix: [#7434] Diagonal ride segments cannot be deleted if they are isolated.
- Fix: [#7436] Only the first 32 vehicles of a train can be painted.
- Fix: [#7480] Graphs skip values of 0.
- Fix: [#7505] Game crashes when trying to make path over map edge while having clearance checks disabled.
- Fix: [#7528] In park entrance pricing tab, switching tabs happens on mouse-down instead of mouse-up
- Fix: [#7544] Starting a headless server with no arguments causes the game to freeze.
- Fix: [#7571] Hovering a ride design over scenery or tracks will give tons of money.
- Improved: [#2989] Multiplayer window now changes title when tab changes.
- Improved: [#5339] Change eyedropper icon to actual eyedropper and change cursor to crosshair.
- Improved: [#5832] Resize tile inspector automatically when selecting a tile element.
- Improved: [#6221] The scenario editor's invention list is now resizeable.
- Improved: [#7069] The arbitrary ride type selection dropdown is now sorted orthographically, and has its spinners removed.
- Improved: [#7302] Raising land near the map edge makes the affected area smaller instead of showing an 'off edge map' error.
- Improved: [#7435] Object indexing now supports multi-threading.
- Improved: [#7510] Add horizontal clipping to cut-away view options.
- Improved: [#7531] "Save track design" dropdown now stays open.
- Improved: [#7548] Ctrl-clicking with the tile inspector open now directly selects an element and its tile.
- Improved: [#7555] Allow setting the Twitch API URL, allowing custom API servers.
- Improved: [#7567] Improve the performance of loading parks and the title sequence.
- Improved: [#7577] Allow fine-tuning the virtual floor style.
- Improved: [#7608] The vehicle selection dropdown is now sorted orthographically.
- Improved: [#7613] Resizing the staff window now resizes the name and action columns too.
- Improved: [#7627] Allow scrolling up and down on spinners to change their values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
network version Network version needs updating - double check before merging!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants