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

Fix high FPS while right clicking on items in lists #7647

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ihhub
Copy link
Owner

@ihhub ihhub commented Aug 27, 2023

close #6518

We shouldn't consider a right mouse click as an event for full list redraw as this event serves as an information action.

IMPORTANT
This change requires heavy testing of all lists in the game as it modifies the behavior for all of them.

@ihhub ihhub added bug Something doesn't work ui UI/GUI related stuff labels Aug 27, 2023
@ihhub ihhub added this to the 1.0.8 milestone Aug 27, 2023
@ihhub ihhub self-assigned this Aug 27, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Collaborator

@Districh-ru Districh-ru left a comment

Choose a reason for hiding this comment

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

Hi, @ihhub, I tested save/load, new game, town portal, kingdom overview, heroes, castles and select items lists. All works great, in Select game resolution/language dialogs right clicking does not provoke high FPS.
But I found one minor issue: in Battle Only open Select Monster dialog and right-click on any monster - you can notice that the info dialog is not closed immediately after releasing the right mouse button. This can be fixed in dialog_armyinfo.cpp at line 649 by placing this code (taken from the Quick View dialog code):

    // Restore background.
    restorer.restore();
    display.render( restorer.rect() );

This is not the issue of current PR because you can observe the same behavior when right-clicking any unit on the battlefield. But if you want, you can fix it here. :)

Also in one of the next PRs we should apply the same fix for holding the right mouse button on any empty monster/artifact/captain/secondary skill in castle, hero, kingdom overview or hero meting dialogs as it currently causes high FPS.

@@ -438,15 +438,15 @@ namespace Interface
_currentId = id;
ActionListSingleClick( item, mousePos, rtAreaItems.x, rtAreaItems.y + offsetY );
}
needRedraw = true;

return true;
}
else if ( le.MousePressRight( rtAreaItems ) ) {
ActionListPressRight( item, mousePos, rtAreaItems.x, rtAreaItems.y + offsetY );
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

After some checks for the cause of high FPS on every right mouse button hold on empty field (castle captain, secondary skill) in Kingdom Overview I found that this return value (true) is used to set redraw to true. In other places it seems that this state is used for the same purposes: to redraw screen or update data after changes.
But holding the right mouse button above the list item should not cause the change of any data. Therefore possibly we should return false here. What do you think, @ihhub?

PS High FPS on right mouse press on any empty artifact/unit in heroes meeting, hero and castle screens can be fixed

by changing:

}
}
return true;
}

and

}
}
return true;
}

to:

        }

        return true;
    }

    return false;
}

The solution is taken from:

bool SecondarySkillsBar::ActionBarRightMouseHold( Skill::Secondary & skill )
{
if ( skill.isValid() ) {
if ( can_change ) {
skill.Reset();
}
else {
fheroes2::SecondarySkillDialogElement( skill, _hero ).showPopup( Dialog::ZERO );
}
return true;
}
return false;
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi @Districh-ru , I think we should also return a boolean value from ActionListPressRight as well to be consistent with the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, @ihhub, you are right, then ActionListPressRight() will return true if redraw is needed and we will return it from QueueEventProcessing().

@ihhub ihhub marked this pull request as draft August 28, 2023 15:05
@ihhub ihhub modified the milestones: 1.0.8, 1.0.9 Sep 11, 2023
@sonarcloud
Copy link

sonarcloud bot commented Sep 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ihhub ihhub modified the milestones: 1.0.9, 1.0.10 Oct 11, 2023
@ihhub ihhub modified the milestones: 1.0.10, 1.0.11 Nov 18, 2023
@ihhub ihhub modified the milestones: 1.0.11, 1.1.0 Dec 23, 2023
@ihhub ihhub modified the milestones: 1.1.0, 1.1.1 May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something doesn't work ui UI/GUI related stuff
Projects
None yet
2 participants