Draw sector map labels with ImGui.#5247
Conversation
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
Some minor changes, some major changes. I haven't reviewed absolutely everything, but this should be most of the major change-required areas.
If I had to sum up the entire review in one useless phrase it would be "prefer writing idiomatic code" - use our types and functions, don't nest anonymous structs for namespacing (and as I write that I realize my input code is guilty), and try to minimize control flow scopes - rather than create a new block wrapped in {}, see if you can invert the logic to avoid the extra indent and new block.
|
Thanks for poking me @fluffyfreak - I've been chipping away at this review for a few days now, just haven't had the time or energy to finish it 😄 |
|
@Web-eWorks wasn't a poke just found I had a few minutes for reviews and "filing" work 😁 |
Gliese852
force-pushed
the
sectormap-labels
branch
from
338835b to
81d2cd5
Compare
|
Force-pushed because I forgot to check with clang. |
|
Looks like there's one change requested left? (and fixes to be squashed.) |
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
Looking good @Gliese852! Thanks for putting up with all of my stylistic and control flow pedantry as usual!
Add polymorphic type SectorView::Label, for displaying different types of labels. Implement two types of labels - for stars and for factions. Add highlight colors to the theme.
Gliese852
force-pushed
the
sectormap-labels
branch
from
adc3c0e to
750f9dc
Compare
|
Squashed all the fix-commits |
4 participants
Add polymorphic type SectorView::Label, for displaying different types of labels.
Implement two types of labels - for stars and for factions.
Add highlight colors to the theme.