Code review comment for lp:~widelands-dev/widelands/rendertarget_ints

Revision history for this message
Notabilis (notabilis27) wrote :

The diff looks okay to me and a short test game hasn't shown any optical glitches. But... ;-)

Three minor remarks code-wise:
- Is there a reason that Bob::calc_drawpos() (and derived methods) are still returning floats? Since it returns the pixel position on the screen I guess it is always an integer anyway.
- UI::correct_for_align() and UI::center_vertically() are somehow related from what they are doing but are offering quite different interfaces. Since I found an older TODO concerning the Align-enum I guess this might be already scheduled to change at some time.
- correct_for_align(kCenter) is called but correct_for_align(kLeft) is omitted. This might be a problem with right-to-left languages (or strange corner cases I could not come up with right now). Could also already be scheduled for change.

Another problem is a crash I encountered in my test game. It happened randomly and I couldn't reproduce it, so it might be also in trunk. But since it is graphic/blitting related I post the backtrace here for now.

Cmd_EnemyFlagAction::execute player(1): flag->owner(2) number=1
Cmd_EnemyFlagAction::execute player(1): flag->owner(2) number=1
Cmd_EnemyFlagAction::execute player(1): flag->owner(2) number=1
Forcing flag at (45, 31)

Thread 1 "widelands" received signal SIGSEGV, Segmentation fault.
0x0000555555de1828 in std::equal_to<unsigned int>::operator() (this=0x555556ad91b8 <Gl::State::instance()::binder+56>, __x=@0x7fffffff9100: 27123, __y=<error reading variable>)
    at /usr/include/c++/6/bits/stl_function.h:356
356 { return __x == __y; }
(gdb) bt
#0 0x0000555555de1828 in std::equal_to<unsigned int>::operator() (this=0x555556ad91b8 <Gl::State::instance()::binder+56>, __x=@0x7fffffff9100: 27123, __y=<error reading variable>)
    at /usr/include/c++/6/bits/stl_function.h:356
#1 0x00005555563b0eab in std::__detail::_Equal_helper<unsigned int, std::pair<unsigned int const, unsigned int>, std::__detail::_Select1st, std::equal_to<unsigned int>, unsigned long, false>::_S_equals (__eq=..., __extract=..., __k=@0x7fffffff9100: 27123, __n=0x608f5b40) at /usr/include/c++/6/bits/hashtable_policy.h:1331
#2 0x00005555563b0b0a in std::__detail::_Hashtable_base<unsigned int, std::pair<unsigned int const, unsigned int>, std::__detail::_Select1st, std::equal_to<unsigned int>, std::hash<unsigned int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Hashtable_traits<false, false, true> >::_M_equals (
    this=0x555556ad91b8 <Gl::State::instance()::binder+56>, __k=@0x7fffffff9100: 27123, __c=27123, __n=0x608f5b40) at /usr/include/c++/6/bits/hashtable_policy.h:1702
#3 0x00005555563b0687 in std::_Hashtable<unsigned int, std::pair<unsigned int const, unsigned int>, std::allocator<std::pair<unsigned int const, unsigned int> >, std::__detail::_Select1st, std::equal_to<unsigned int>, std::hash<unsigned int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node (this=0x555556ad91b8 <Gl::State::instance()::binder+56>, __n=27123, __k=@0x7fffffff9100: 27123, __code=27123)
    at /usr/include/c++/6/bits/hashtable.h:1420
#4 0x00005555563b0072 in std::_Hashtable<unsigned int, std::pair<unsigned int const, unsigned int>, std::allocator<std::pair<unsigned int const, unsigned int> >, std::__detail::_Select1st, std::equal_to<unsigned int>, std::hash<unsigned int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_node (this=0x555556ad91b8 <Gl::State::instance()::binder+56>, __bkt=27123, __key=@0x7fffffff9100: 27123, __c=27123)
    at /usr/include/c++/6/bits/hashtable.h:634
#5 0x00005555563af71e in std::__detail::_Map_base<unsigned int, std::pair<unsigned int const, unsigned int>, std::allocator<std::pair<unsigned int const, unsigned int> >, std::__detail::_Select1st, std::equal_to<unsigned int>, std::hash<unsigned int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true>, true>::operator[] (this=0x555556ad91b8 <Gl::State::instance()::binder+56>, __k=@0x7fffffff9100: 27123) at /usr/include/c++/6/bits/hashtable_policy.h:591
#6 0x00005555563aefe5 in std::unordered_map<unsigned int, unsigned int, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<std::pair<unsigned int const, unsigned int> > >::operator[] (this=0x555556ad91b8 <Gl::State::instance()::binder+56>, __k=@0x7fffffff9100: 27123) at /usr/include/c++/6/bits/unordered_map.h:904
#7 0x00005555563ae47e in Gl::State::do_bind (this=0x555556ad9180 <Gl::State::instance()::binder>, target=33985, texture=27123)
    at /widelands/rendertarget_ints/src/graphic/gl/utils.cc:196
#8 0x00005555563ae3b6 in Gl::State::bind (this=0x555556ad9180 <Gl::State::instance()::binder>, target=33985, texture=27123)
    at /widelands/rendertarget_ints/src/graphic/gl/utils.cc:180
#9 0x000055555636751d in BlitProgram::draw (this=0x555556ad8e60 <BlitProgram::instance()::blit_program>, arguments=std::vector of length 416, capacity 512 = {...})
    at /widelands/rendertarget_ints/src/graphic/gl/blit_program.cc:166
#10 0x0000555556222689 in RenderQueue::draw_items (this=0x555556ad6a60 <RenderQueue::instance()::render_queue>, items=std::vector of length 455, capacity 1024 = {...})
    at /widelands/rendertarget_ints/src/graphic/render_queue.cc:226
#11 0x0000555556222585 in RenderQueue::draw (this=0x555556ad6a60 <RenderQueue::instance()::render_queue>, screen_width=1920, screen_height=1080)
    at /widelands/rendertarget_ints/src/graphic/render_queue.cc:213
#12 0x0000555555e7f71a in Graphic::refresh (this=0x555556b44630) at /widelands/rendertarget_ints/src/graphic/graphic.cc:206
#13 0x000055555607e130 in UI::Panel::do_run (this=0x55555f50c240) at /widelands/rendertarget_ints/src/ui_basic/panel.cc:197
#14 0x0000555555dda8a8 in UI::Panel::run<UI::Panel::Returncodes> (this=0x55555f50c240) at ../src/ui_basic/panel.h:96
#15 0x0000555555eea120 in Widelands::Game::run (this=0x7fffffff99b0, loader_ui=0x7fffffff9810, start_game_type=Widelands::Game::NewNonScenario, script_to_run="", replay=false,
    prefix_for_replays="single_player") at /widelands/rendertarget_ints/src/logic/game.cc:525
#16 0x0000555555dd4dae in WLApplication::new_game (this=0x555556b03a90) at /widelands/rendertarget_ints/src/wlapplication.cc:1243
#17 0x0000555555dd4368 in WLApplication::mainmenu_singleplayer (this=0x555556b03a90) at /widelands/rendertarget_ints/src/wlapplication.cc:1103
#18 0x0000555555dd3d5f in WLApplication::mainmenu (this=0x555556b03a90) at /widelands/rendertarget_ints/src/wlapplication.cc:1008
#19 0x0000555555dd0758 in WLApplication::run (this=0x555556b03a90) at /widelands/rendertarget_ints/src/wlapplication.cc:445
#20 0x0000555555dced0d in main (argc=1, argv=0x7fffffffd918) at /widelands/rendertarget_ints/src/main.cc:49

« Back to merge proposal