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

feat(UI): Further improved the new planet display UI #7072

Merged
merged 39 commits into from
Sep 19, 2022

Conversation

Hurleveur
Copy link
Member

@Hurleveur Hurleveur commented Jul 30, 2022

Bugfix: This PR addresses issue #7065, and is based on PR #7066

Fix Details

Add the possibility to use the arrow keys to go up and down.

Moves the UI elements around (will make the arrows a bit further away shortly).

Minor fix to the scrolling.

Note: marking this as a draft, no done yet but I'm open to further suggestions. I'll add a screenshot once I'm done.

Testing Done

I went in the remnant system and looked a it, tried the up & down keys, the draging, clicking on down arrow, scrolling to hide part of the sprites, clicked on the "visited" part, trying clicking on the down arrow and the government part.

Checklist of things that got fixed:

  • Add the click and drag
  • Down arrow doesn't seem to be working? Some things could be a bit off; further testing is required.
  • fix government not well clickable, "not yet visited" not showing visited but governments when it was selected
  • UI okay (as in, no longer with unconnected parts)
  • going up and down with arrow keys does it on the trade panel by default except if the mouse is over the map planet view. It is now possible to go to the first from the bottom and to the bottom from the first with the keys.
  • selecting on the orbits selects in the panel view

Optional (not directly asked by the issue, but would improve the look significantly:

  • solve the pixelized look of ring-worlds by adding the possibility of defining an icon removed because no longer required, with the new ringworld images
  • clip the images below (idk how to do this properly, help!) thanks @quyykk for the help on this, it was some nice teamwork
  • adjust the size for small screens
  • account for possible zoom to adjust the size of the panel

@Hurleveur Hurleveur marked this pull request as draft July 30, 2022 20:22
@Hurleveur Hurleveur added bug Something in the game is not behaving as intended mechanics Things dealing with the mechanics & code of how the game works labels Jul 30, 2022
Hurleveur and others added 2 commits July 31, 2022 14:51
also simplify the code to no longer account for the top part (now its drawn out of the screen)

Co-Authored-By: Nick <85879619+quyykk@users.noreply.github.com>
@Hurleveur Hurleveur added this to the 0.9.15 milestone Jul 31, 2022
@Hurleveur Hurleveur marked this pull request as ready for review July 31, 2022 13:29
@Hurleveur
Copy link
Member Author

Hurleveur commented Jul 31, 2022

I think that almost everything from the issue is addressed, I even added clipping on the bottom, with help from quyykk.

I did not change the sprites yet given I quite honestly suck at doing that kind of thing, even if its just rearranging it'll take me ages and probably not be good enough still. Would appreciate some help on that end.

it makes more sense to give its length, that way its the same on all screens
@Amazinite Amazinite linked an issue Jul 31, 2022 that may be closed by this pull request
@Amazinite Amazinite removed this from the 0.9.15 milestone Jul 31, 2022
@Hurleveur
Copy link
Member Author

I had placed the milestone on this PR because of the fact the issue is linked to the milestone as well, and this PR mostly addresses said issue, but maybe I was wrong doing that?
Also I can cut down this PR to put the drawing changes to allow for clipping in another one, if needed.

@Terin
Copy link
Collaborator

Terin commented Jul 31, 2022

I had placed the milestone on this PR because of the fact the issue is linked to the milestone as well, and this PR mostly addresses said issue, but maybe I was wrong doing that?

We don't need to add both an issue and PR related to the same core thing on milestones. The issue is already there, so that's enough.

@Terin
Copy link
Collaborator

Terin commented Aug 1, 2022

I think we're going to have to remove the map system UI element entirely as it is right now. With the current PR, we have this:

image
(Very noticeable gap on most edge permutations)

I tried moving it 10px to the left, which can work beter, but we still have the ribbon elements of the edge overlapping the system element, or the edge overlapping it entirely:

image

Tried having the system panel draw first as well, but then the left edge is very noticeable on some edges.

image

And regardless of all those, we've got those bottom ribbon elements hanging off, leading towards nothing. We could do pixel editing, but I tried to make a mini-spaceport panel as a predecessor to this, and that pixel editing is EXTREMELY hacky and painful, due to the various gradiants and drop-shadows the elements have.

I'm currently finishing up writing an email to MZ, seeing if we can crack the code on how to render the .svg source files in the same style as the current .pngs, which I have not been able to with Inkscape. Unless we get an extremely quick response we a solution though, I think the only solution is to embed the title into the side element, with a possible larger font and simple UI element demarcating it:

image

@Amazinite, would something like that work? If not, I think this would need to wait until 0.9.17, since any non-hacky solution would be fairly time-consuming.

@Hurleveur
Copy link
Member Author

Hurleveur commented Aug 1, 2022

That still leaves the issue of the trade thing below. Also you didnt even notice but its even worse when there are no planets

Here is how it looks when combining the two elements that are annoying us - they match perfectly. I think I'll be pushing this change (which is just about interface values actually, and a bit of adjustment to the code) for now at least, until we change the images, as it may not be the place where we want to put it but its the place where it fits well; perfectly, even.
combine

the X being free to place means more freedom to move things around but could lead to bugs (and ugly code repetition); this PR was never about being able to customize the interface so I removed that possibility
@Terin
Copy link
Collaborator

Terin commented Aug 1, 2022

That still leaves the issue of the trade thing below.

image

What we had before was pretty much fine. A little noisy, but everything connected at least.

Also you didnt even notice but its even worse when there are no planets

I've got a branch that had a minimum size to the scrolling panel, but if we have the system name + government built in, that'll always be filled.

also reinstate visited thing (I wrote Government for it by mistake)
also use SetScroll to set the scroll instead of modifying the variable manually
small bug introduced earlier on in the PR when moving the government Y to below the planets; it was always showing the reputations
the ring worlds looked awfull (the rest looks really good), this allows just giving them an image that will look good in a small space; without needing to do exceptions and whatnot for certain sizes of sprites
@Terin
Copy link
Collaborator

Terin commented Aug 5, 2022

Clicks also feel a bit misaligned for me now, also, where I need to click a few pixels lower. Screen size doesn't matter, does it at 1280x~780 at 100 main zoom, and 4k at 200 main zoom.

Also, the system title should probably be rendered after the planet card, otherwise you get weird clipping like this at lower resolutions.
image

@Hurleveur
Copy link
Member Author

Hurleveur commented Aug 5, 2022

I think that the proper thing to do at lower resolutions is to diminish the size of the map planet cards.

for the clicks do you mean for the trade or for everything? (I added a coefficient of 15 to tradeY when it is instanciated to make it what felt right to me but maybe it isnt the right one)

when drawing the planets, check we don't get outside the planet panel, which is not always defined by his height, but could be smaller due to a small screen
@Hurleveur
Copy link
Member Author

Clicks also feel a bit misaligned for me now, also, where I need to click a few pixels lower. Screen size doesn't matter, does it at 1280x~780 at 100 main zoom, and 4k at 200 main zoom.

I tested this 10 times with different values, this +15. at line 693 of mapDetailPanel just lines it up perfectly for my taste, as in when you click between two categories the one you are the closer to will be selected.

Also this made me think to account for main zoom being different than normal for the amount of planets that are displayed.

Copy link
Collaborator

@Amazinite Amazinite left a comment

Choose a reason for hiding this comment

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

This is looking a lot better. The system/government element being above the trade panel feels a little weird, but at least all the UI pieces go together now. If we get a better handle on making our own UI images then we can potentially revisit that placement in the future.

There are a couple strange interactions that I want to point out, though:

If you go to the outfitter key and hit the up or down arrow keys while nothing is selected, the top or bottom outfit on the list will be selected. But when I go to the ports panel and use the up or down arrows, that sometimes moves the currently selected trade commodity, but sometimes does nothing. The only way to get the arrows to change which planet is selected is to first click on one of the planets, and then the arrow keys move your selection. Or if the arrow keys are doing nothing and you want to cycle your trade selection, you have to click on a trade for the arrows to begin working.
It should be the case that which list the arrow keys traverse is determined by where your mouse is located. Looking to the shipyard or outfitter shop panels as an example, if your mouse is over the items being sold then your arrow keys navigate the currently selected item, but if your mouse is over the ship info then your arrow keys navigate the currently selected ship in your fleet. Similarly, if my mouse is located over (or perhaps near) the planet list then my arrows should cycle through that list, and the same if my mouse is located over (or perhaps near) the trade commodities list.

If I click on a planet in the planet list, that planet becomes selected on the list, the description for that planet comes up (if it has been visited) and that planet becomes selected in the orbit map.
image
But if I do the reverse and select the planet from the orbit map, the planet does not become selected on the planet list.
image

And as Terin pointed out, the draw order is still a bit off at (absurdly) low vertical resolutions. The bottom edge should be drawn below the map system.
image

@Hurleveur
Copy link
Member Author

Hurleveur commented Sep 2, 2022

What about making the down & up keys change which trade is selected, by default (because you're hovering the map), except if you're hovering over the planets?
It just seems weird to me to have them do nothing in some cases.

draw the government after the back panel

improve the selection logic, if you're not hovering on the planets move it, else it moves the planets

going up and down with nothing selected selects last/first, and one can go from last/top and in the other way

also now selecting on the orbit panel selects in the trade panel (and moves to it)
it will automatically resize to show half of the last planet if not all planets can be seen, and only the size of the screen matters now
also fix the little bit of panel shown when there was no planet/system wasnt visited
finally I made the scroll & drag work fully like other panels
Copy link
Collaborator

@Amazinite Amazinite left a comment

Choose a reason for hiding this comment

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

Beautiful.

source/MapPlanetCard.cpp Outdated Show resolved Hide resolved
@Amazinite Amazinite changed the title Fix(UI) of the map detail panel system + add full functionality of scrollable list feat(UI): Further improved the new planet display UI Sep 3, 2022
Copy link
Member

@tibetiroka tibetiroka left a comment

Choose a reason for hiding this comment

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

Grammar review for the code comments.

source/MapDetailPanel.cpp Outdated Show resolved Hide resolved
source/MapDetailPanel.h Outdated Show resolved Hide resolved
source/MapDetailPanel.h Outdated Show resolved Hide resolved
Co-authored-by: tibetiroka <68112292+tibetiroka@users.noreply.github.com>
Co-authored-by: Amazinite <jsteck2000@gmail.com>
@roadrunner56 roadrunner56 requested a review from a team September 4, 2022 02:29
Hurleveur and others added 3 commits September 4, 2022 22:55
it seemed to work just fine like this on my computer and all zooms
best to have it often take too little space than sometimes show up on top of each other
Co-Authored-By: Nick <85879619+quyykk@users.noreply.github.com>
source/StellarObject.h Outdated Show resolved Hide resolved
source/MapDetailPanel.cpp Outdated Show resolved Hide resolved
it was a nice feature but one that falls outside of the scope of this PR since it is no longer required, with the new ringworld images looking fine
made it an int
@quyykk quyykk merged commit 9ef4e92 into endless-sky:master Sep 19, 2022
@Hurleveur Hurleveur deleted the map-detail-feature-+-fix branch September 19, 2022 18:12
quyykk pushed a commit to quyykk/endless-sky that referenced this pull request Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something in the game is not behaving as intended mechanics Things dealing with the mechanics & code of how the game works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The new map planet UI needs work
5 participants