Added dynamic interface option #9413

felix642 · 2025-01-05T18:23:47Z

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).

github-actions

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/fheroes2/system/settings.cpp

Districh-ru

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?

src/fheroes2/dialog/dialog_interface_settings.cpp

src/fheroes2/game/game_startgame.cpp

src/fheroes2/system/settings.cpp

src/fheroes2/dialog/dialog_interface_settings.cpp

Districh-ru

Well done!

ihhub

Hi @felix642 , I put several comments here. Would you mind to take a look?

ihhub · 2025-01-26T13:13:42Z

src/fheroes2/game/game_startgame.cpp

@@ -735,6 +735,12 @@ fheroes2::GameMode Interface::AdventureMap::StartGame()

        // Fully update fog directions if there will be only one human player.
        Interface::GameArea::updateMapFogDirections();
+
+        if ( conf.getInterfaceType() == InterfaceType::DYNAMIC ) {
+            reset();


This doesn't seem right. At line 702 we call reset and then we are calling it here. The code looks more like a hack. Could you please explain why we cannot do the same at line 702?

This is because when the game is first drawn (L. 702 - 710), the current player has not been selected yet (L. 734).
This code was added to redraw the interface to make sure that it matches the current player, but you're right, there should be a way to only draw the interface once, I'll check if I can move some code around to make it happen.

ihhub · 2025-01-26T13:16:44Z

src/fheroes2/system/settings.h

+    GOOD = 0,
+    EVIL = 1,
+    DYNAMIC = 2,
+    COUNT


Either all entries have no value or all of them. SonarQube will complain about this warning.

ihhub · 2025-01-26T13:17:42Z

src/fheroes2/system/settings.h

@@ -379,6 +389,7 @@ class Settings
    int ai_speed;
    int scroll_speed;
    int battle_speed;
+    InterfaceType _interfaceType;


This member doesn't have default value.

ihhub · 2025-01-26T13:17:54Z

src/fheroes2/system/settings.h

@@ -191,6 +199,9 @@ class Settings
    bool isHideInterfaceEnabled() const;
    bool isEvilInterfaceEnabled() const;

+    void setInterfaceType( InterfaceType type );
+    InterfaceType getInterfaceType() const;


This method should be inlined.

ihhub · 2025-01-26T13:20:30Z

src/fheroes2/system/settings.cpp

-void Settings::setEvilInterface( const bool enable )
-{
-    if ( enable ) {
-        _gameOptions.SetModes( GAME_EVIL_INTERFACE );


GAME_EVIL_INTERFACE must be renamed into UNUSED_GAME_EVIL_INTERFACE as we are no longer supporting it.

ihhub · 2025-01-26T13:23:56Z

src/fheroes2/system/settings.cpp

+        // Keep the UI of the last player during the AI turn
+        for ( auto iter = Settings::Get().GetPlayers().rbegin(); iter < Settings::Get().GetPlayers().rend(); ++iter ) {
+            if ( *iter && ( *iter )->isControlHuman() ) {
+                return Race::isEvilRace( ( *iter )->GetRace() );
+            }
+        }


Do we need to check this? UI Interface changes only during player's turn.

Some parts of the UI is updated during the AI's turn.
Mainly the top part with the shield:

If I remove this code, the background of the shield will change color very rapidly based on the AI factions

ihhub · 2025-01-26T13:25:22Z

src/fheroes2/system/settings.cpp

+        os << "interface type = Good" << std::endl;
+        break;
+    case EVIL:
+        os << "interface type = Evil" << std::endl;
+        break;
+    case DYNAMIC:
+        os << "interface type = Dynamic" << std::endl;


None of settings have values starting from a capital letter. Please keep it the same way,.

ihhub · 2025-01-26T13:25:38Z

src/fheroes2/system/settings.cpp

-    if ( config.Exists( "use evil interface" ) ) {
-        setEvilInterface( config.StrParams( "use evil interface" ) == "on" );


Here we are breaking the existing settings players set. We should read the value of the previous setting.

ihhub · 2025-01-26T13:27:32Z

src/fheroes2/game/game_startgame.cpp

+                        if ( conf.getInterfaceType() == InterfaceType::DYNAMIC ) {
+                            reset();
+                            redraw( Interface::REDRAW_RADAR );
+                            redraw( Interface::REDRAW_ALL & ( ~Interface::REDRAW_RADAR ) );
+                        }


Do we want to rebuild everything even if the interface type is going to be the same?

ihhub · 2025-01-26T13:31:56Z

src/fheroes2/dialog/dialog_interface_settings.cpp

@@ -287,7 +297,7 @@ namespace fheroes2
                windowType = showConfigurationWindow( saveConfiguration );
                break;
            case SelectedWindow::InterfaceType:
-                conf.setEvilInterface( !conf.isEvilInterfaceEnabled() );
+                conf.setInterfaceType( static_cast<InterfaceType>( ( conf.getInterfaceType() + 1 ) % InterfaceType::COUNT ) );


This is the only place where InterfaceType::COUNT is really needed. We can simply do this for the code to avoid having an extra enumeration entry:

if ( conf.getInterfaceType() == InterfaceType::DYNAMIC ) { conf.setInterfaceType( InterfaceType::GOOD ); } else if ( conf.getInterfaceType() == InterfaceType::GOOD ) { conf.setInterfaceType( InterfaceType::BAD ); } else { conf.setInterfaceType( InterfaceType::DYNAMIC ); }

Then we won't need checks in the code for non-existing types.

ihhub · 2025-01-30T05:36:24Z

Hi @felix642 , I converted this pull request into a draft. Once you update it please mark it as ready for review again.

Added dynamic interface option

332a0f4

github-actions bot assigned felix642 Jan 5, 2025

github-actions bot reviewed Jan 5, 2025

View reviewed changes

src/fheroes2/system/settings.cpp Outdated Show resolved Hide resolved

Clang-tidy

7775059

oleg-derevenetz reviewed Jan 5, 2025

View reviewed changes

src/fheroes2/system/settings.cpp Outdated Show resolved Hide resolved

IWYU + compilation issues

09756de

felix642 force-pushed the dynamic-interface branch from 3356102 to 09756de Compare January 5, 2025 18:35

Code review

751df95

felix642 force-pushed the dynamic-interface branch from 203d29e to 751df95 Compare January 5, 2025 18:58

Copyright

3e1b60c

oleg-derevenetz added improvement New feature, request or improvement ui UI/GUI related stuff labels Jan 5, 2025

oleg-derevenetz added this to the 1.1.6 milestone Jan 5, 2025

ihhub requested review from ihhub and Districh-ru January 6, 2025 11:38

Districh-ru requested changes Jan 7, 2025

View reviewed changes

Code review

98d0701

Districh-ru self-requested a review January 20, 2025 18:49

Districh-ru reviewed Jan 23, 2025

View reviewed changes

src/fheroes2/dialog/dialog_interface_settings.cpp Outdated Show resolved Hide resolved

Code checker

1766211

Districh-ru self-requested a review January 25, 2025 16:24

Districh-ru reviewed Jan 25, 2025

View reviewed changes

src/fheroes2/dialog/dialog_interface_settings.cpp Outdated Show resolved Hide resolved

Code review

d779ded

Districh-ru self-requested a review January 25, 2025 16:52

Districh-ru approved these changes Jan 25, 2025

View reviewed changes

ihhub requested a review from Branikolog January 26, 2025 13:12

ihhub requested changes Jan 26, 2025

View reviewed changes

ihhub marked this pull request as draft January 30, 2025 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added dynamic interface option #9413

Added dynamic interface option #9413

felix642 commented Jan 5, 2025 •

edited

Loading

github-actions bot left a comment

Districh-ru left a comment

Districh-ru left a comment

ihhub left a comment

ihhub Jan 26, 2025

felix642 Jan 26, 2025

ihhub Jan 26, 2025

ihhub Jan 26, 2025

ihhub Jan 26, 2025

ihhub Jan 26, 2025

ihhub Jan 26, 2025

felix642 Jan 26, 2025

ihhub Jan 26, 2025

ihhub Jan 26, 2025

ihhub Jan 26, 2025

ihhub Jan 26, 2025

ihhub commented Jan 30, 2025

		if ( config.Exists( "use evil interface" ) ) {
		setEvilInterface( config.StrParams( "use evil interface" ) == "on" );

Added dynamic interface option #9413

Are you sure you want to change the base?

Added dynamic interface option #9413

Conversation

felix642 commented Jan 5, 2025 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

Districh-ru left a comment

Choose a reason for hiding this comment

Districh-ru left a comment

Choose a reason for hiding this comment

ihhub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihhub commented Jan 30, 2025

felix642 commented Jan 5, 2025 •

edited

Loading