Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795976-port-attack-crash into lp:widelands
Review: Approve Code looks good to me -- https://code.launchpad.net/~widelands-dev/widelands/bug-1795976-port-attack-crash/+merge/356454 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1795976-port-attack-crash. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795976-port-attack-crash into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/bug-1795976-port-attack-crash into lp:widelands.
Commit message:
Fix endless loop in DefaultAI::dispensable_road_test
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1795976 in widelands: "Crash, game freezes, memory overflow"
https://bugs.launchpad.net/widelands/+bug/1795976
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1795976-port-attack-crash/+merge/356454
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1795976-port-attack-crash into lp:widelands.
=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc 2018-09-29 14:35:29 +
+++ src/ai/defaultai.cc 2018-10-11 06:58:14 +
@@ -3593,6 +3593,7 @@
uint16_t road_length = road.get_path().get_nsteps();
for (int j = 0; j < 2; ++j) {
+ std::set visited_flags;
bool new_road_found = true;
while (new_road_found && full_road.back()->nr_of_roads() <= 2 &&
full_road.back()->get_building() == nullptr) {
@@ -3616,10 +3617,16 @@
if (other_end->get_position() == full_road[full_road_size - 2]->get_position()) {
continue;
}
-full_road.push_back(other_end);
-road_length += near_road->get_path().get_nsteps();
-new_road_found = true;
-break;
+
+// Do not visit the same flag twice
+if (visited_flags.count(other_end->serial()) == 0) {
+ // We have found a new flag to add to the road
+ full_road.push_back(other_end);
+ road_length += near_road->get_path().get_nsteps();
+ new_road_found = true;
+ visited_flags.insert(other_end->serial());
+ break;
+}
}
}
// we walked to one end, now let revert the concent of full_road and repeat in opposite
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
How about trying to do what the original error message is suggesting? https://cmake.org/cmake/help/latest/policy/CMP0072.html Try this line: OpenGL_GL_PREFERENCE=LEGACY -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmakepolicy. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ship_scheduling_2 into lp:widelands
Well, I have fully mature branches that have been sitting around for a year that won't make it in because they are still waiting for a code review slot... I could just call them "UI bugs" and insist on having them in Build 20, delaying the release until who knows when, maybe another year? Every changed line of code has the potential for bugs, and as we have seen from the failing test suite, there are edge cases that don't immediately come up during gameplay testing. You are providing some complex control flow in your branches, so the potential for undiscovered new bugs is high. I have just fixed an endless loop that was introduced in February and only reported last week. So, I don't think that we should risk it with the release coming shortly. Widelands is too mature for the player base to find bugs like that acceptable in a release. Also, the release procedure that we are using has been developed by my predecessor over the course of > 15 years, so I'm expecting there to be reasons for it to be like it is. Patience is part of the game, I'm afraid. If you are too demotivated to carry on by having to wait for a few weeks until a feature gets in, here is nothing that we can do about it. I'd be sad to see you leave :'( -- https://code.launchpad.net/~widelands-dev/widelands/ship_scheduling_2/+merge/355510 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ship_scheduling_2. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795976-port-attack-crash into lp:widelands
OK, good idea... thanks -- https://code.launchpad.net/~widelands-dev/widelands/bug-1795976-port-attack-crash/+merge/356454 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1795976-port-attack-crash. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795976-port-attack-crash into lp:widelands
now when I look more deep into the code I think the function has a logical error there, I will investigate and let you know.. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1795976-port-attack-crash/+merge/356454 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1795976-port-attack-crash. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795976-port-attack-crash into lp:widelands
Do you want to take up this branch and improve it before we merge? I'll hold off merging for now. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1795976-port-attack-crash/+merge/356454 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1795976-port-attack-crash. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
@GunChleoc But that does not solve the underlying issue, does it? Setting: cmake_policy(SET CMP0072 OLD) is the same as not setting the policy at all. In both will result in: OpenGL_GL_PREFERENCE=LEGACY The first one though would silence the warning BUT we would run into issues with Cmake version older than 3.11 since they don't know this policy, as seen here: https://launchpadlibrarian.net/392627292/buildlog_ubuntu-trusty-i386.widelands_1%3A19-ppa0-bzr8875-201810101704~ubuntu14.04.1_BUILDING.txt.gz CMake Error at CMakeLists.txt:4 (cmake_policy): Policy "CMP0054" is not known to this version of CMake. Since I don't have a Linux machine I can't real test for the linking errors kaputtnik reported. The commit as it is now should solve the problem with the buildsystem. Should I open a new commit just for the buildsystem? -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmakepolicy. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
@kaputtnik can you give this another try? -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmakepolicy. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795976-port-attack-crash into lp:widelands
Continuous integration builds have changed state: Travis build 4110. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/440029552. Appveyor build 3905. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1795976_port_attack_crash-3905. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1795976-port-attack-crash/+merge/356454 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1795976-port-attack-crash. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ship_scheduling_2 into lp:widelands
"Widelands is too mature for the player base to find bugs like that acceptable in a release." Unless you have asked the player-base about the matter, the above statement is arbitrary. If you dare, you can conduct a poll on the issue. Projects much bigger and much more serious than Widelands have no problem releasing code that is not tested in every possible way, exactly to involve the user-base in the testing process (even more so when it comes to complex control flow). The endless loop you fixed after many months, would have been both reported and fixed much earlier if there was a release to expose it to users. Most importantly, the vast majority of modern user-bases has no more problem dealing with minor bugs, as long as they have an easy way to report them and the programmers are eager to fix them in soon-to-follow bug-fixing releases. Users can be as patient as the programmers, especially non-paying users. Of course all that may leave your "German mentality" untouched, and this is not the place for a theoretical discussion. Still motivation is no less important than patience, and Widelands needs developers at least as much as it needs users. Or so I was told, among other non-truths, like that there are more testers than developers. On the bright side, it is good to know the practical value of my contributions, before undertaking something as big as fixing the routing or the AI. -- https://code.launchpad.net/~widelands-dev/widelands/ship_scheduling_2/+merge/355510 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ship_scheduling_2. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
Continuous integration builds have changed state: Travis build 4111. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/440139202. Appveyor build 3906. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cmakepolicy-3906. -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmakepolicy. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
Compiling is fine here with the latest change: No Warning and no compile errors.
I do not really understand that stuff, but can't we just add a switch like that
(instead of setting the linker flags):
if(${CMAKE_VERSION} VERSION_GREATER "3.11")
cmake_policy(SET CMP0072 OLD)
endif()
I tested this and it works for me (deleted the build directory and compiled
again) ... Has to be commented well, though.
--
https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/cmakepolicy.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/fri-portraits into lp:widelands
@Klaus: I don´t think using real portraits would work. There are very few portraits of Frisians. Aldgisl is about as close as we can get, and he was a Western Frisian duke (so he´d fit to Reebaud, who seems more Western than Northern frisian anyway), but there are few others. In the middle ages, it was mainly rich kings, dukes and the like that had themselves painted. But Northern Frisian society knew no distinction between higher and lower classes – nearly everyone was a peasant, even the (Ratsherren) and (Deichgrafen). Also, as far as I know most frisians who were rich enough to have themselves painted lived on Strand and other places the sea since reclaimed, so those portraits were destroyed by the flood of 1634. That´s why only few older paintings of (northern) Frisians exist. And I don´t want to edit real photographs for Widelands; I tried that but couldn´t produce anything useful. @GunChleoc: This branch doesn´t need to wait for b21, right? Since it´s a graphics-only change. -- https://code.launchpad.net/~widelands-dev/widelands/fri-portraits/+merge/356221 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fri-portraits. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy_2 into lp:widelands
kaputtnik has proposed merging lp:~widelands-dev/widelands/cmakepolicy_2 into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1772079 in widelands: "CMake 3.11: warning about FindOpenGL"
https://bugs.launchpad.net/widelands/+bug/1772079
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy_2/+merge/356609
Another attempt to fix CMP warnings. Lets see what Travis and Appveyor think of
this :-)
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/cmakepolicy_2 into lp:widelands.
=== modified file 'CMakeLists.txt'
--- CMakeLists.txt 2018-09-26 13:41:40 +
+++ CMakeLists.txt 2018-10-11 19:24:20 +
@@ -1,7 +1,14 @@
project (widelands)
cmake_minimum_required (VERSION 2.8.7)
-cmake_policy(SET CMP0054 NEW)
+
+if(${CMAKE_VERSION} VERSION_GREATER "3.10")
+cmake_policy(SET CMP0072 OLD)
+endif()
+
+if(${CMAKE_VERSION} VERSION_GREATER "3.0")
+cmake_policy(SET CMP0054 NEW)
+endif()
include("${CMAKE_SOURCE_DIR}/cmake/WlFunctions.cmake")
@@ -140,6 +147,7 @@
message(STATUS "Not using AddressSanitizer.")
endif(OPTION_ASAN)
+
if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
wl_add_flag(WL_COMPILE_DIAGNOSTICS "-Weverything")
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
@kaputtnik the changes I made set the policy to NEW. The trick that it. did were the additional linker options. But it broke travis builds and will throw errors on system that use old cmake version. Your proposed switch sets it to the old (deprecated) behavior. Would you be so nice and test the latest change I pushed? This change only sets the policy to NEW, if it is known to the Cmake version in use. This should work with travis, but we have to wait, though. -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmakepolicy. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
Btw. AFAIK when using the new behavior, it seems mandatory to set the linker-flags. See here: https://stackoverflow.com/questions/23729213/link-error-when-trying-to-build-a-simple-opengl-program -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmakepolicy. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
@kaputtnik you could try to remove the linker flags bit by bit and see which one is really necessary to compile without linker errors. -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmakepolicy. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy_2 into lp:widelands
Continuous integration builds have changed state: Travis build 4113. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/440355897. Appveyor build 3908. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cmakepolicy_2-3908. -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy_2/+merge/356609 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/cmakepolicy_2 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/cmakepolicy into lp:widelands
Continuous integration builds have changed state: Travis build 4114. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/440360936. Appveyor build 3909. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cmakepolicy-3909. -- https://code.launchpad.net/~widelands-dev/widelands/cmakepolicy/+merge/356018 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cmakepolicy. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
