wesnoth / wesnoth

An open source, turn-based strategy game with a high fantasy theme.
https://www.wesnoth.org/
GNU General Public License v2.0
5.46k stars 1k forks source link

Scenario editor, unit facing, game crash #9226

Open Matthias878 opened 3 weeks ago

Matthias878 commented 3 weeks ago

Game and System Information

Description of the bug

C++ error message, game crashes

Steps to reproduce the behavior

  1. Open Game
  2. Map Editor
  3. New Scenario
  4. place Dune Paragon (maybe any unit)
  5. Change facing of unit to North East (maybe more directions)
  6. try to press file in the top left

Expected behavior

Normally the dropdown menu should open

Screenshot (54)

Additional context

I have a addon that adds it's units for the editor, I don't know if that may be causing the issue, with the following code:

ifdef EDITOR

[binary_path] path=data/add-ons/New_Southern_Nations/ [/binary_path]

{~add-ons/New_Southern_Nations/macros/}

[units] {~add-ons/New_Southern_Nations/units/Elves/} {~add-ons/New_Southern_Nations/units/Ogre/} {~add-ons/New_Southern_Nations/units/Undead/} {~add-ons/New_Southern_Nations/units/} [/units]

endif

Wedge009 commented 3 weeks ago

Doesn't appear that unit or orientation matters. Confirmed in 1.18 and 1.19, on Linux.

wesnoth: src/editor/controller/editor_controller.cpp:667: virtual hotkey::ACTION_STATE editor::editor_controller::get_action_state(const hotkey::ui_command&) const: Assertion `un != get_current_map_context().units().end()' failed.
Wedge009 commented 3 weeks ago

From master: https://github.com/wesnoth/wesnoth/blob/56d05b166ba2c8687a70927d3d2de21430377ac2/src/editor/controller/editor_controller.cpp#L664-L669

Most recent change was 489dcebd0116c61ab25a65cbdcfa4df4394b53b6 for Wesnoth 1.13.11, by @Vultraz. I don't really know what get_action_state() is supposed to do.

jy02140486 commented 3 weeks ago

I test it with other units and the issue still happens then I break down this line get_current_mapcontext().units().find(gui->mouseoverhex()) into separated statements finding that the actual assert is in gui->mouseover_hex(), I think it's because the courser is on the tab not the map? and in this circumstance, case editor::UNIT_FACING should be closed? when clicking the file tab on top, the activemenu should not remain UNIT_FACING I guess?

Wedge009 commented 3 weeks ago

The issue doesn't occur if just placing a unit. The issue occurs if the orientation menu is opened at all, even if no selection is made.

mouseover_hex() does sound suspicious. If the code is doing what the names suggest, it does sound like it's looking for units in the hex where the mouse is present, no unit is found (of course), and then the assertion fails.

Wedge009 commented 3 weeks ago

Actually, I think the UNIT_FACING case should not be hit at all when clicking File. I am not familiar with any of this code, but it looks like active_menu_ might not be updated properly once the unit orientation sub-menu has closed.

Wedge009 commented 3 weeks ago

It's something to do with active_menu_ not being set properly. If I open the 'unit facing' menu, then open some other menu such as the 'Time of Day schedule', then click File -> no issue. However, if attempting to click File or Map straight after opening the 'unit facing' menu, then active_menu_ is still reading as UNIT_FACING for some reason, and then mouseover_hex() returns what looks like uninitialised co-ordinates, which leads to no units being found and so the assertion fails.

soliton- commented 3 weeks ago

Simplest fix would be to just not assert. Return ACTION_DESELECTED if no unit is found. The assert seems questionable anyway.

Otherwise need to figure out how this whole design is supposed to work. It certainly seems odd why this code runs at all when you open the menu. Clearly the facing case at least does not expect it.

Wedge009 commented 3 weeks ago

The assertion just seems to be attempting to catch the case where no unit is assigned to un (I really don't like these abbreviated variable names). Without knowing more about how the editor works, it seems to make sense even if it's not that useful. I agree that this UNIT_FACING case does not expect to be run if it's not actually the unit facing menu, which is why I wonder what's going on to cause this case to be hit.

While I had a break-point on the un assignment, it was being hit twelve times upon a call to the actual unit facing menu - I'm guessing twice for each orientation (six directions from a hexagon). I also wonder if that's meant to happen.