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.

Sign up for GitHub

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

Jump to bottom

Implement some SkirmishBattleHonors #1126

Merged
merged 3 commits into from
Mar 27, 2024
Merged

Implement some SkirmishBattleHonors #1126

merged 3 commits into from
Mar 27, 2024

Conversation

psykana
Copy link
Contributor

@psykana psykana commented Mar 21, 2024

No description provided.

@xezon
Copy link
Contributor

xezon commented Mar 21, 2024

Oh nice. I will get to review today.

xezon
xezon reviewed Mar 21, 2024
View reviewed changes
src/hooker/setupglobals_zh.cpp Outdated Show resolved Hide resolved
src/hooker/setupglobals_zh.cpp Outdated Show resolved Hide resolved
src/game/common/system/registryget.cpp Outdated Show resolved Hide resolved
src/game/common/system/registryget.cpp Outdated Show resolved Hide resolved
src/game/common/skirmishbattlehonors.h Outdated Show resolved Hide resolved
src/game/common/skirmishbattlehonors.cpp Outdated Show resolved Hide resolved
src/game/common/skirmishbattlehonors.cpp Outdated Show resolved Hide resolved
src/game/common/skirmishbattlehonors.cpp Outdated Show resolved Hide resolved
src/game/common/skirmishbattlehonors.cpp Outdated Show resolved Hide resolved
src/game/common/skirmishbattlehonors.cpp Outdated
str = g_theGameText->Fetch("TOOLTIP:BattleHonors", nullptr);
g_theMouse->Set_Cursor_Tooltip(str, -1, 0, 1.0f);
} else if (honor != 0) {
if (honor & 0x8000000) { // not earned
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there perhaps an enum value missing for 1<<27 in SkirmishBattleHonorsTooltips?

@psykana psykana requested a review from xezon March 25, 2024 04:05
@xezon
Copy link
Contributor

xezon commented Mar 25, 2024

Code wise looks good to me. I will take a look at it ingame later today.

@xezon
Copy link
Contributor

xezon commented Mar 25, 2024

Like breaks in tool tips are wrong.

Original and Before this change

shot_20240325_183112_1

This change

shot_20240325_183309_1

xezon
xezon reviewed Mar 25, 2024
View reviewed changes
Copy link
Contributor

@xezon xezon left a comment

Choose a reason for hiding this comment

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

See above

@psykana
Copy link
Contributor Author

psykana commented Mar 25, 2024

I tested before/after at different resolutions so that went over my head, fixed.

@psykana psykana requested a review from xezon March 25, 2024 18:06
xezon
xezon approved these changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@xezon xezon left a comment

Choose a reason for hiding this comment

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

Looks good.

@psykana
Copy link
Contributor Author

psykana commented Mar 25, 2024 •
edited
Loading

Can I run format check locally so we don't have to drag this any more?

xezon
xezon reviewed Mar 25, 2024
View reviewed changes
Copy link
Contributor

@xezon xezon left a comment

Choose a reason for hiding this comment

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

Formatting check is failing. You need to apply clang format, with version 10.0.0 (as part of LLVM). If you are using Visual Studio, you can refer to custom clang-format exe in settings page, otherwise it uses clang-format as part of Visual Studio distribution.

@xezon
Copy link
Contributor

xezon commented Mar 26, 2024 

/home/runner/work/Thyme/Thyme/src/game/common/skirmishbattlehonors.cpp:379:9: error: no matching member function for call to 'Format'
    str.Format(L"%10d", wins);
    ~~~~^~~~~~
/home/runner/work/Thyme/Thyme/src/game/common/system/unicodestring.h:140:10: note: candidate function not viable: no known conversion from 'const wchar_t [5]' to 'const unichar_t *' (aka 'const char16_t *') for 1st argument
    void Format(const unichar_t *format, ...);
         ^
/home/runner/work/Thyme/Thyme/src/game/common/system/unicodestring.h:141:10: note: candidate function not viable: no known conversion from 'const wchar_t [5]' to 'Utf16String' for 1st argument
    void Format(Utf16String format, ...);
         ^
/home/runner/work/Thyme/Thyme/src/game/common/skirmishbattlehonors.cpp:430:17: error: cast from pointer to smaller type 'int' loses information
        honor = (int)Gadget_List_Box_Get_Item_Data(listbox, row, column);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/Thyme/Thyme/src/game/common/skirmishbattlehonors.cpp:431:17: error: cast from pointer to smaller type 'int' loses information
        score = (int)Gadget_List_Box_Get_Item_Data(listbox, row - 1, column);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4 warnings and 3 errors generated.

xezon
xezon reviewed Mar 27, 2024
View reviewed changes
src/game/common/skirmishbattlehonors.cpp Outdated
str = g_theGameText->Fetch("TOOLTIP:BattleHonorLoyaltyUSA", nullptr);
g_theMouse->Set_Cursor_Tooltip(str, -1, nullptr, 1.5f);
}
if (honor & LOYALTY_CHINA) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be else if? And below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Should be good to go now.

@psykana psykana requested a review from xezon March 27, 2024 21:14
@xezon xezon merged commit 54999d2 into TheAssemblyArmada:develop Mar 27, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xezon xezon Awaiting requested review from xezon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@psykana @xezon