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

Improved tile inspector and added map element helper functions #2590

Merged
merged 22 commits into from Jan 1, 2016

Conversation

Projects
None yet
4 participants
@Broxzier
Copy link
Member

Broxzier commented Dec 30, 2015

Here's an example of what the tile inspector looks like now:

Screenshot

Full list of changes:

  • Path name and path addition names are displayed int he list
  • Using new helper functions for setting and getting path addition ghost, type, and index throughout the code (I carefully checked all of them, but an extra eye won't hurt: https://github.com/OpenRCT2/OpenRCT2/pull/2590/files).
  • Ability to select a map element in the list.
  • Ability to remove selected map element.
  • Ability to move selected map element up and down in the list.
  • When inserting a corrupt map element, it will automatically be selected.
  • Redesigned UI to be more compact, and more consistent with the other windows (same button size, same margins, same placement of widgets), and added tooltips to the table headers.
    • BH: Base height
    • CH: Clearance height
    • B: Broken
    • G: Ghost
    • L: Last element for tile
  • Displaying ride type for tracks.

(note: Because of rebasing sometimes there are many commits with the same changes. Is there a better way to update the branch?)

@Gymnasiast

This comment has been minimized.

Copy link
Member

Gymnasiast commented Dec 30, 2015

First of all: great initiative. 👍

Secondly, a small thing: the button with Remove could better be labelled Remove element, IMO.

Thirdly, I don't really like the button layout. It doesn't really fit with the RCT2 style - icon buttons on the right of the window would look better (like the guest and ride windows, for example), or alternatively, buttons on top (like the staff window). Of course, this is debatable.

Lastly, it looks like your PR could do with a rebase and a squash. It even contains some merge commits, which shouldn't be needed when using Git to rebase on a newer version.

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Dec 30, 2015

@Gymnasiast That's good to hear. I agree with you on the text, however together with your third point I think having it as a bin icon would be better. The layout as it is now is based on the sequence editor window.

I rebased the branch yesterday before working on it, which has quite some conflicts that needed solving, do I need to do that again before squashing?

@janisozaur

This comment has been minimized.

Copy link
Member

janisozaur commented Dec 30, 2015

There are commits in this PR that apparently got already merged, since rebasing them creates empty commits. There are few minor conflicts, can you please clean up this PR (rebase + squash)?

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Dec 30, 2015

@janisozaur

This comment has been minimized.

Copy link
Member

janisozaur commented Dec 30, 2015

Roughly. It doesn't have to be just one commit, you can split the change into smaller logical chunks, but 52 is a bit much and makes reviewing harder.

If you want to make it just one commit, then there's better way: create a new branch from develop, checkout the tree at 3a972b2, wrap it into a commit, replace this branch with your new commit. From the top of my head:

git fetch origin
git co -b tile-inspector origin/develop
git co 3a972b2 -- .
git ci -a # write some fancy commitmsg in the editor
git push -f my_github HEAD:refactor_path_element # this will *replace* commits in here

where origin = upstream, my_github = your fork

@IntelOrca

This comment has been minimized.

Copy link
Contributor

IntelOrca commented Dec 30, 2015

You wouldn't squash something like this. Otherwise you lose the time period that this was worked over. If you just rebase to latest develop, that should remove the merge commits.

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Dec 30, 2015

Just to be sure:

git checkout develop
git pull upstream develop
git checkout <branch>
git rebase develop

Then when conflicts occur fix them, git add and when done git push?

@IntelOrca

This comment has been minimized.

Copy link
Contributor

IntelOrca commented Dec 30, 2015

@Broxzier yep
although you will need to git push -f

@Gymnasiast

This comment has been minimized.

Copy link
Member

Gymnasiast commented Dec 30, 2015

@Broxzier : Yeah, a bin icon for removing, vertical arrows for moving and a question mark (the one from miscellaneous objects) for inserting would do nicely. :)

@Broxzier Broxzier force-pushed the Broxzier:refactor_path_element branch from 3a972b2 to a4574ea Jan 1, 2016

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Jan 1, 2016

I rebased the branch and pushed it now. I'm not sure if I am doing something wrong or not, because I had to solve the same conflicts multiple times.

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Jan 1, 2016

The new layout:

@Gymnasiast: When I tried using the question mark, the background of the button was black. Maybe because it's a toolbar button? I've picked the Map sprite for now.

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Jan 1, 2016

Other than some feedback on the 'insert corrupt element' button sprite, I don't have anything else in my to-do list for this PR.

@IntelOrca

This comment has been minimized.

Copy link
Contributor

IntelOrca commented Jan 1, 2016

You could use the checkmark instead of 0 and 1 for those boolean columns.
Have a look at CheckBoxMarkString

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Jan 1, 2016

@IntelOrca

This comment has been minimized.

Copy link
Contributor

IntelOrca commented Jan 1, 2016

@Broxzier Sweet!

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Jan 1, 2016

When I rebase on develop, which I updated, I get this message:

$ git rebase develop
Current branch refactor_path_element is up to date.
@janisozaur

This comment has been minimized.

Copy link
Member

janisozaur commented Jan 1, 2016

that's likely because you haven't updated your develop branch. Fetch from upstream and rebase on top of that:

git fetch origin
git rebase origin/develop

Or update your develop:

git co develop
git pull --ff-only

and retry the rebase.

@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Jan 1, 2016

@janisozaur Both of them tell me that it's already up-to-date.

$ git rebase origin/develop
Current branch refactor_path_element is up to date.

and in develop

$ git pull --ff-only
Already up-to-date.
@janisozaur

This comment has been minimized.

Copy link
Member

janisozaur commented Jan 1, 2016

what is your origin set to?

git remote -v | grep origin
@Broxzier

This comment has been minimized.

Copy link
Member Author

Broxzier commented Jan 1, 2016

origin is my fork, upstream is the official OpenRCT2 repo.

@janisozaur

This comment has been minimized.

Copy link
Member

janisozaur commented Jan 1, 2016

then you should upstream as remote name

@Broxzier Broxzier force-pushed the Broxzier:refactor_path_element branch from 9376e99 to 560a435 Jan 1, 2016

IntelOrca added a commit that referenced this pull request Jan 1, 2016

Merge pull request #2590 from Broxzier/refactor_path_element
Improved tile inspector and added map element helper functions

@IntelOrca IntelOrca merged commit b8ab05a into OpenRCT2:develop Jan 1, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Gymnasiast

This comment has been minimized.

Copy link
Member

Gymnasiast commented Jan 1, 2016

@Broxzier 👍

@Broxzier Broxzier deleted the Broxzier:refactor_path_element branch Jan 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment