widelands

dedicated server broken due to calls to g_gr

Bug #1322869 reported by Nasenbaer
16
This bug affects 2 people
Affects Status Importance Assigned to Milestone
widelands
Fix Released
Undecided
Unassigned
widelands build19-rc1

Bug Description

user fu-fu wrote in our forums, that the dedicated server module is segfaulting once the game is started and indeed it is.
The problem (until now - didn't came further) is the preloading of the animations of bobs and immovables. During loading the bobs and immovables, the animations are now supposed to be loaded as well. However this should only be the case if Widelands is run with user interface, as else this does not make a lot of sense obviously.

Now I am a bit unsure how to treat this problem. Attached you find the first part of a fix I was working on. For me it looks a lot like a hack. Generally it would be better to not at all call the animation loader, however in that case profile.cc is bailing out as some sections have not be used...

Well I hoped to get a fast fix ready, but I stopped at this point as I was
* unsure as described above
* not seeing an end until now (everytime I fixed one place where g_gr is called, the next appeared), so I thought once more whether there wasn't a better way to fix this?
* ... I still am rare on time. :(

Can anyone of the graphic code guys please take a look at this?

Related branches

lp://staging/~widelands-dev/widelands/dedicated_out_of_main
Tino: Approve
GunChleoc: Approve
Diff: 3599 lines (+457/-1848)
25 files modified
src/CMakeLists.txt (+0/-3)
src/io/CMakeLists.txt (+0/-13)
src/io/dedicated_log.cc (+0/-342)
src/io/dedicated_log.h (+0/-90)
src/logic/editor_game_base.cc (+1/-5)
src/logic/game.cc (+56/-108)
src/logic/game.h (+13/-7)
src/logic/game_settings.h (+0/-11)
src/main.cc (+0/-57)
src/map_io/map_building_packet.cc (+1/-2)
src/network/CMakeLists.txt (+0/-1)
src/network/internet_gaming.cc (+21/-22)
src/network/netclient.cc (+47/-163)
src/network/netclient.h (+0/-2)
src/network/nethost.cc (+102/-593)
src/network/nethost.h (+1/-8)
src/network/network_protocol.h (+7/-35)
src/sound/sound_handler.h (+0/-1)
src/ui_fsmenu/launch_mpg.cc (+0/-10)
src/ui_fsmenu/loadgame.cc (+164/-179)
src/ui_fsmenu/mapselect.cc (+39/-113)
src/wlapplication.cc (+2/-77)
src/wlapplication.h (+1/-1)
src/wlapplication_messages.cc (+0/-1)
src/wui/chat_msg_layout.cc (+2/-4)
Revision history for this message
Nasenbaer (nasenbaer) wrote :
Revision history for this message
SirVer (sirver) wrote :

I think it makes more sense to implement a dummy g_gr that has all methods, but they do not do anything. Having ifs() all over the codebase is not ideal.

That entails: make all methods in Graphic virtual and add virtual dummys for most other classes in there that will returned instead.

The proper fix is of course to have the logical being fully observable and never calling the GUI, then you would just never launch the GUI - but that is a lot of work.

Revision history for this message
Nasenbaer (nasenbaer) wrote :

I agree with you that the dummy g_gr would mostlikely be the easiest solution. However that dummy would still be forced to e.g. readout certain data, as Widelands does check_used() checks on those data, which is just wasted CPU and time... :(

Concerning "the proper fix" - it was that way in Build 17, so unfortunally this is more or less a regression :-/.

Revision history for this message
SirVer (sirver) wrote :

> readout certain data, as Widelands does check_used() checks on those data, which is just wasted CPU and time... :(

not sure what you want to say with that? checking that values of configuration files are used is certainly a good thing to do - it will prevent future bugs. If you do not care for the dummy implementation, add a 'mark all keys as read' call to Section and LuaTable.

> Concerning "the proper fix" - it was that way in Build 17, so unfortunally this is more or less a regression :-/.

No, we never had logic and UI separated. Try building logic/* economy/* into one library. If this works without needed headers or libraries from ui_basic or wui/ or graphics/ we would be there, but that never works and will not work for a long time.

concerning the regression: untested code regresses easily, especially one that relies on if/else checks in many places of the code. Add a regression test to the testsuite and it will not regress again.

Revision history for this message
SirVer (sirver) wrote :

Setting to incomplete for bug sweeping.

Changed in widelands:
status: Confirmed → Incomplete
GunChleoc (gunchleoc)
Changed in widelands:
status: Incomplete → Confirmed
Revision history for this message
SirVer (sirver) wrote :

Then we should have a test for dedicated as well somewhere.

SirVer (sirver)
Changed in widelands:
assignee: nobody → SirVer (sirver)
assignee: SirVer (sirver) → nobody
assignee: nobody → SirVer (sirver)
Revision history for this message
SirVer (sirver) wrote :

--dedicated is gone, so this bug is gone too.

Changed in widelands:
status: Confirmed → Fix Committed
assignee: SirVer (sirver) → nobody
GunChleoc (gunchleoc)
tags: added: graphics
removed: graphic
GunChleoc (gunchleoc)
Changed in widelands:
status: Fix Committed → Fix Released
Revision history for this message
GunChleoc (gunchleoc) wrote :

Fixed in build19-rc1.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Subscribing...

Other bug subscribers

Related blueprints

Remote bug watches

Bug watches keep track of this bug in other bug trackers.