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

Preliminary cleanup of mapview_common #1169

Merged
merged 22 commits into from
Aug 5, 2022

Conversation

lmoureaux
Copy link
Contributor

This PR contains a collection of small commits that improve the quality of the mapview_common code that handles drawing the map and updating it when needed. The commits are mostly independent from each other and I'm happy to take out controversial ones and discuss them in another PR. No behavior change is expected.

I strongly suggest to review this commit by commit. See individual commit messages for more context.

lmoureaux added 10 commits July 30, 2022 20:40
All this function was doing was calling repaint().
It wasn't doing anything useful.
It wasn't doing anything useful.
It always ended up calling update_map_canvas_visible, only after a roundtrip
through the update_queue. Call directly instead.
It was the same as update_map_canvas_visible().
It was never used.
A simple bool is enough.
They do the exact same thing. Call the merged function
update_map_canvas_visible because it's used a lot.
@lmoureaux lmoureaux requested a review from psampathkumar July 30, 2022 19:05
@lmoureaux lmoureaux marked this pull request as ready for review July 30, 2022 19:05
@lmoureaux lmoureaux added the refactoring This issue requires code refactoring label Jul 30, 2022
lmoureaux added 11 commits July 30, 2022 23:32
Use QRectF instead of handling the geometry by hand. This makes the code easier
to understand (that is, once you know what QRectF::operator|= does)
Declare variables where they're first used.
QRegion is equivalent to the manual bookeeping previously in use, but is easier
to use.
Simplifies the code a tiny bit. std::swap is magic.
It doesn't have anything to do with the map.
It was just a thin wrapper around find_city_or_settler_near_tile.
We can recalculate this on the fly whenever we need it, which is not very
often.
I'm not sure since when this has been unused but I've only started seeing the
warnings recently.
@lmoureaux
Copy link
Contributor Author

The Apple clang failure should be fixed, others look like glitches...

@psampathkumar
Copy link
Contributor

psampathkumar commented Aug 1, 2022

The Apple clang failure should be fixed, others look like glitches...

Looks like the opposite to me, windows ones are linker errors which we need to fix. Apple one seems like a vcpkg issue.

@psampathkumar
Copy link
Contributor

Anyways, the code refactor looks ok. We can merge if the builds dont break.

@lmoureaux
Copy link
Contributor Author

The Apple clang failure should be fixed, others look like glitches...

Looks like the opposite to me, windows ones are linker errors which we need to fix. Apple one seems like a vcpkg issue.

Indeed, I thought they were identical to failures in other PRs but they're actually unique. I have no idea how my changes can trigger an undefined reference to main...

@jwrober
Copy link
Collaborator

jwrober commented Aug 3, 2022

Maybe we need to rebase this to master branch and let the CI run again?

@lmoureaux
Copy link
Contributor Author

Maybe we need to rebase this to master branch and let the CI run again?

I'm waiting for Windows Update to complete then I can investigate. Probably tomorrow.

@jwrober
Copy link
Collaborator

jwrober commented Aug 3, 2022

Maybe we need to rebase this to master branch and let the CI run again?

I'm waiting for Windows Update to complete then I can investigate. Probably tomorrow.

It fails on my local Visual Studio too, i'm trying to find the error message.

@lmoureaux
Copy link
Contributor Author

Builds for me without SDL and without updating my MSYS install.

$ cmake --build build -j5
-- Git HEAD Commit Hash: (hex) ea9fe7 and (dec) 15376359
-- Last tag commit hash: 137068d83d196a4e9c8391365886996a9581e54d
-- Latest Git Tag: v3.0-beta.3
SDL2 not found
SDL2_mixer not found
-- Found Lua: C:/msys64/mingw64/lib/liblua.dll.a (found suitable version "5.4.3", minimum required is "5.3")
-- Could NOT find ToLuaProgram (missing: TOLUA_PROGRAM)
-- Found Lua: C:/msys64/mingw64/lib/liblua.dll.a (found version "5.4.3")
-- Using the Windows native API for stack unwinding. This is the preferred option.
-- Using the Windows native API to retrieve stack symbols. You'll need to convert DWARF debug information to PDB.
-- Could NOT find Sphinx (missing: SPHINX_EXECUTABLE)
-- Including CPack
-- Configuring done
-- Generating done
-- Build files have been written to: C:/msys64/home/Louis/freeciv21/build

@lmoureaux
Copy link
Contributor Author

After installing SDL, it fails with:

C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:/msys64/mingw64/lib/libqtmain.a(qtmain_win.o):C:\M\mingw-w64-qt5-pkgs\src\MINGW64\qtbase\src\winmain/qtmain_win.cpp:97: undefined reference to `qMain(int, char**)'
collect2.exe: error: ld returned 1 exit status

--> hints at an interaction between Qt and SDL

@lmoureaux
Copy link
Contributor Author

f87d3d5 is the first bad commit

The competition between Qt and SDL to redefine main() on Windows (for
portability purposes and the WinMain entry point for GUI applications)
ends up badly when Qt is included before SDL. Thus make sure to include
SDL before any other header.

See longturn#1169.
@lmoureaux
Copy link
Contributor Author

Windows build fixed in 2edf4ec

@lmoureaux lmoureaux enabled auto-merge (rebase) August 4, 2022 21:21
@lmoureaux lmoureaux disabled auto-merge August 4, 2022 21:21
@lmoureaux lmoureaux merged commit df9e3e5 into longturn:master Aug 5, 2022
@lmoureaux lmoureaux deleted the refactor/mapview branch August 5, 2022 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring This issue requires code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants