-
-
Notifications
You must be signed in to change notification settings - Fork 377
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
Use drawOnState() for buttons precessing and cache buttons' position for event handler loop #9223
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)
@@ -635,11 +638,13 @@ bool Battle::Arena::DialogBattleSummary( const Result & res, const std::vector<A | |||
int32_t buttonHorizontalMargin = 0; | |||
const int32_t buttonVerticalMargin = 5; | |||
std::unique_ptr<fheroes2::Button> buttonRestart; | |||
std::unique_ptr<fheroes2::Rect> buttonRestartArea; |
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.
While the optimization is important, I think this change brings 2 problems:
- the code becomes longer and less readable (a subjective opinion)
- this change is based on an assumption that both pressed and released images are the same size which might not be the case
As a result we end up having such solutions with std::unique_ptr
.
If we decide to keep the original approach with area()
method usage we can make that method inlined since it's a very simple method. However, we won't get much benefit from it as we call a virtual function inside. So, I don't have a strong opinion which solution is better.
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 the button is made its area()
is often used.
Then should we just add fheroes2::Rect _areaPressed;
and fheroes2::Rect _areaReleased;
to the ButtonBase
class?
Then we can make an inline method like:
const Rect & area()
{
return _isPressed ? _areaPressed : _areaReleased;
}
And then remove all caches in of area()
in the code.
What do you think, @ihhub?
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.
@ihhub,
I've made the proposed change to cache area in ButtonBase
class and the just return a const reference to it by area()
call.
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)
…e '.area()' only when needed
This PR affects most of the dialogs in the engine including the main menu.
It replaces
le.isMouseLeftButtonPressedInArea( button.area() ) ? button.drawOnPress() : button.drawOnRelease();
withbutton.drawOnState( le.isMouseLeftButtonPressedInArea( buttonArea ) );
and dobuttonArea
cache not to call.area()
with all its calculations on every loop.It also modifies some dialogs that can be also called by the mouse right click to have its own event handler loop to reduce some checks (e.g. unit dialog, build dialog). And in some dialogs it skips the possibly unused buttons initialization making them
unique_ptr
, in example in Hero dialog: DISMISS and PATROL buttons.While testing this PR you should check rendering of the buttons (press/release) and their interaction with the mouse cursor (right press, left click).
This PR does not fix rendering issues that are present in the master build.