-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Rewrite and migrate V
menu to ImGui
#55503
base: master
Are you sure you want to change the base?
Conversation
b7854a4
to
121194d
Compare
121194d
to
b52ac09
Compare
05357ce
to
0bf3591
Compare
This pull request introduces 3 alerts when merging 0bf3591 into f889ed9 - view on LGTM.com new alerts:
|
0bf3591
to
cb976c3
Compare
This pull request introduces 3 alerts when merging cb976c3 into d055515 - view on LGTM.com new alerts:
|
cb976c3
to
218a9a1
Compare
d39553d
to
8c02583
Compare
This pull request introduces 1 alert when merging 8c02583 into 158205f - view on LGTM.com new alerts:
|
590cff8
to
9e5eeb2
Compare
4618af9
to
a73ed64
Compare
Do we have a 'salvageable' label for PRs? |
I think I just need to ask a maintainer to reopen it when I continue working on it. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not bump or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered. |
V
menuV
menu to ImGui
de4d48d
to
d71a221
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that you’re not finished yet, but I like what you’re doing and want to help you get it right. Here are a few comments about things that I noticed as I read the code.
const Character &you = get_player_character(); | ||
const bool inattentive = you.has_trait( trait_INATTENTIVE ); | ||
bool sees = mon->sees( you ) && !inattentive; | ||
cataimgui::draw_colored_text( sees ? "!" : " ", c_yellow ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When possible, prefer to just call ImGui::TextColored
instead, as it does less work than cataimuig::draw_colored_text
.
if( bar_width < bar_max_width ) { | ||
hp_text += "<color_c_white>"; | ||
for( int i = bar_max_width - bar_width; i > 0; i-- ) { | ||
hp_text += "."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to just call ImGui::TextUnformatted
in a loop than to repeatedly append to a string.
switch( selected_tab ) { | ||
case surroundings_menu_tab_enum::items: | ||
item_data.selected_entry->select_next(); | ||
break; | ||
case surroundings_menu_tab_enum::monsters: | ||
monster_data.selected_entry->select_next(); | ||
break; | ||
case surroundings_menu_tab_enum::terfurn: | ||
terfurn_data.selected_entry->select_next(); | ||
break; | ||
default: | ||
debugmsg( "invalid tab selected" ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and every other method here seem very verbose and duplicative. Perhaps something like this would be better:
tab_data list;
switch( selected_tab ) {
case surroundings_menu_tab_enum::items:
list = item_data;
break;
case surroundings_menu_tab_enum::monsters:
list = monster_data;
break;
case surroundings_menu_tab_enum::terfurn:
list = terfurn_data;
break;
default:
debugmsg( "invalid tab selected" );
}
list.selected_entry->select_next();
They could be shortened further by putting the switch into a common method, perhaps called get_currently_selected_list
. Then it would just be:
get_currently_selected_list().selected_entry->select_next();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of that can be simplified, yes. I did so with the filtering for example. But for some others it isn't that easy, because the tabs have different underlying data types (item, Creature, mapdata_common_t).
Yeah, it's still very WIP, but that's good feedback! |
29b141c
to
ee28e11
Compare
b90179f
to
4606228
Compare
4606228
to
6ac5517
Compare
6ac5517
to
874addf
Compare
Summary
Infrastructure "Rewrite and migrate V menu to ImGui"
Purpose of change
Migrate to ImGui and increase maintainability and expandability.
Describe the solution
Completely remake everything using ImGui.
Structurally this is heavily reliant on code I wrote when I started this before ImGui two years ago. It might be a good idea to refactor some of that, but for now I mostly want to get it functional.
Describe alternatives you've considered
There is no alternative (except for implementation details).
Testing
Press
V
and play around with it.Additional context
Currently wip. Some basic functionality is implemented, so interested people can check it out and give feedback. The terrain/furniture tab won't be a part of the final version of this PR but is going to be added in another one.