-
-
Notifications
You must be signed in to change notification settings - Fork 387
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
Added dynamic interface option #9413
base: master
Are you sure you want to change the base?
Conversation
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.
Clang-Tidy
found issue(s) with the introduced code (1/1)
3356102
to
09756de
Compare
203d29e
to
751df95
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.
Hi, @felix642, I missed Dynamic interface option after the original game. Thanks for making this PR!
I left several comments to make it a bit better. Could you please check them when you have time?
else { | ||
switch ( interfaceType ) { | ||
case DYNAMIC: | ||
interfaceThemeIcon = &fheroes2::AGG::GetICN( ICN::SPANEL, 15 ); |
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.
We can make uint32_t icnIndex;
and then set its value by the interfaceType
and then use it in
const fheroes2::Sprite & interfaceThemeIcon = fheroes2::AGG::GetICN( ICN::SPANEL, icnIndex );
Doing this we will not duplicate the GetICN()
call code and will not make a pointer to Sprite.
What do you think?
@@ -287,7 +299,7 @@ namespace fheroes2 | |||
windowType = showConfigurationWindow( saveConfiguration ); | |||
break; | |||
case SelectedWindow::InterfaceType: | |||
conf.setEvilInterface( !conf.isEvilInterfaceEnabled() ); | |||
conf.setInterfaceType( static_cast<InterfaceType>( ( conf.getInterfaceType() + 1 ) % ( InterfaceType::DYNAMIC + 1 ) ) ); |
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.
To be more consistent with the other code we can add InterfaceType::COUNT
as a last item of InterfaceType
enumeration and use it here instead of ( InterfaceType::DYNAMIC + 1 )
.
if ( conf.getInterfaceType() == InterfaceType::DYNAMIC ) { | ||
reset(); | ||
redraw( Interface::REDRAW_RADAR ); | ||
redraw( Interface::REDRAW_ALL & ( ~Interface::REDRAW_RADAR ) ); | ||
} |
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.
In single-player game the race of the player is never changed so should we do this (reset();
and radar redraw) only for the Hot Seat game?
And for the single-player game do only reset();
after the line 737.
What do you think?
Added dynamic interface (good / evil) based on the player's race.
Relates to: #1329 (Despite the title, this issue also references the ability to hide the hero's path which was not added in this PR so we can't close it yet).