Assertion failed: (w->widgets[i].type != WWT_LAST), function widget_invalidate, file ../src/openrct2/interface/window.c, line 994.

[sgielen]

OS: Mac OS X 10.12.5
Version: master
Commit/Build: 1aed331 + 1 irrelevant commit of my own (#5892)

I didn't do anything specific when this assertion triggered. IIRC a peep just walked past a drink stand, I clicked it, it was thinking "I'm not thirsty", and a second later this happened. Perhaps this peep was just thinking about something to do, but maybe it is completely unrelated.

I don't know if this is easily reproducible either. Happened during a singleplayer game. Maybe the stack trace helps:

Application Specific Information:
Assertion failed: (w->widgets[i].type != WWT_LAST), function widget_invalidate, file ../src/openrct2/interface/window.c, line 994.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libsystem_kernel.dylib 0x00007fffb50b9d42 __pthread_kill + 10
1 libsystem_pthread.dylib 0x00007fffb51a7457 pthread_kill + 90
2 libsystem_c.dylib 0x00007fffb501f420 abort + 129
3 libsystem_c.dylib 0x00007fffb4fe6893 __assert_rtn + 320
4 libopenrct2.dylib 0x000000010c93987d widget_invalidate + 189
5 libopenrct2.dylib 0x000000010c96ec0c peep_pick_ride_to_go_on + 1356
6 libopenrct2.dylib 0x000000010c965e0b sub_68F41A + 1755
7 libopenrct2.dylib 0x000000010c965387 peep_update_all + 151
8 libopenrct2.dylib 0x000000010c91f6a4 game_logic_update + 196
9 libopenrct2.dylib 0x000000010c91f4f7 game_update + 599
10 libopenrct2.dylib 0x000000010c98cc0e rct2_update + 238
11 libopenrct2.dylib 0x000000010ce68517 OpenRCT2::Context::RunFixedFrame() + 183
12 libopenrct2.dylib 0x000000010ce68029 OpenRCT2::Context::RunGameLoop() + 153
13 libopenrct2.dylib 0x000000010ce676ea OpenRCT2::Context::Launch() + 746
14 libopenrct2.dylib 0x000000010ce66b89 OpenRCT2::Context::RunOpenRCT2(int, char**) + 57
15 openrct2 0x000000010c8b0c4c main + 236
16 libdyld.dylib 0x00007fffb4f8b235 start + 1

[Gymnasiast]

The stack trace help a bit, but we really need a way to reproduce.

[sgielen]

I loaded my save and was clicking around a bit, but couldn't reproduce. But, looking from the stack trace and the relevant code, it's apparently possible for peep_pick_ride_to_go_on to call widget_invalidate on a widget whose type is WWT_LAST, even though that is asserted to be impossible. Perhaps that could help in finding some way to reproduce, by making such a situation much more likely, e.g. by artificially calling peep_pick_ride_to_go_on much more often than usual? (I've only looked at the code for a few minutes, so I have no idea whether that's practical)

[janisozaur]

The reproducer is checking sick peep's thoughts tab, there is an issue about it already.

[janisozaur]

See #3055 and #1911

[w-flo]

Not sure if I should file a new issue since this is a separate cause for hitting the same assertion (edit: might actually be the same cause, the peep_pick_ride_to_go_on function was apparently renamed and converted to a method – if so, OP probably looked at one of the tabs other than the main tab while the guest randomly decided to visit a ride, so the action was changed to "Heading for X" while the action label was not visible).

My game hits this failed assertion when looking at any tab other than the main tab in a guest window, while the guest is heading to a ride (guest_heading_to_ride_id) and arriving at the queue.

When the guest arrives at the queue, widget_invalidate is called for WC_PEEP__WIDX_ACTION_LBL in line 2143 of Guest.cpp. That's probably an issue because the guest "action" label is not visible at the time? The same call to widget_invalidate is in 3 other places in that file. I guess there is an out of bounds widgets array access in non-debug builds that don't have the assertion?

Dirty fix that seems to work (but I guess there are better ways, at least defining that #define in a different place): https://gist.github.com/w-flo/f2699cb0ad94adb95595b63fa59013b7

[duncanspumpkin]

I'd rather we created a new method in the peep window for invalidation.

[w-flo]

Is widget_invalidate even required after changing guest_heading_to_ride_id and calling window_event_invalidate_call()? I commented out those 4 widget_invalidate() calls in Guest.cpp, and in a few manual tests didn't notice any issues like stale text in the action label. Maybe debug builds (or something else specific to my setup) force a full screen redraw each frame and that's why I don't see any difference? Or something else causes the window / widget to invalidate?

I'm not sure about the right way to call a new method in the peep window from Guest.cpp. It looks like direct calls to peep window methods are not the right way to do it (because nothing from openrct2-ui is #included, there's probably a reason for that like headless mode). There are some events that could be used to trigger the action label invalidation, but I'm not sure if any of the available events are the right choice for this.

Maybe the easy way out, if an invalidate call is required, would be window_invalidate(w) instead of widget_invalidate(w, WC_PEEP__WIDX_ACTION_LBL)? Obviously that invalidates the whole window instead of only the label, so that's a small performance hit.

Disclaimer: I'm just digging around the source code and have no idea what I'm doing.
Edit: I used the wrong function name.

[duncanspumpkin]

Now that I think about it more I think the way it is meant to work is that there is no call to widget_invalidate and the widget is invalidated by a separate function.

[w-flo]

I looked at the vehicle code to see how the issue is solved there (e.g. when the vehicle speed changes, the vehicle status line needs to be invalidated).

In openrct2/ride/Vehicle.cpp whenever the vehicle status changes, vehicle_invalidate_window(vehicle) is called, like this (line 5323):

    vehicle->status = VEHICLE_STATUS_CRASHED;
    vehicle_invalidate_window(vehicle);

That function broadcasts an INTENT_ACTION_INVALIDATE_VEHICLE_WINDOW (I thought that's purely an android thing, but apparently not) passing the updated vehicle as a param:

void vehicle_invalidate_window(rct_vehicle* vehicle)
{
    auto intent = Intent(INTENT_ACTION_INVALIDATE_VEHICLE_WINDOW);
    intent.putExtra(INTENT_EXTRA_VEHICLE, vehicle);
    context_broadcast_intent(&intent);
}

Now that Intent is received by openrct2-ui/WindowManager.cpp and parsed in line 371:

            case INTENT_ACTION_INVALIDATE_VEHICLE_WINDOW:
            {
                rct_vehicle* vehicle = static_cast<rct_vehicle*>(intent.GetPointerExtra(INTENT_EXTRA_VEHICLE));
                [..]
                w = window_find_by_number(WC_RIDE, vehicle->ride);
                [..] // check if window is open for the vehicle and return if not
                window_invalidate(w);
                break;
            }

Is there an effort going on to remove direct window_* calls and other UI stuff from the "core" (src/openrct2) part of the code and use only Intent broadcasts or other means of indirect calls, like the window event system? If so, the proper way to solve this issue would probably be to add another Intent like INTENT_ACTION_INVALIDATE_PEEP_WINDOW (or maybe GUEST_WINDOW) and broadcast that intent when the guest "action" status changes.

The possible performance hit of invalidating the full window instead of only the "action" label seems small, and the vehicle window gets fully invalidated, too. The guest action label changes rarely compared to vehicle speed. There's another performance hit for the Intent system though, because broadcasting an intent implies some overhead (appears to be a bunch of method/function calls, stack allocations and one std::map heap allocation) and it would have to be broadcasted every time any guest "action" changes, so the WindowManager intent handler can check if a window for that guest is currently open and then do absolutely nothing in most cases (when no window is open).

I'd say the easy fix for this would be to change widget_invalidate(w, WC_PEEP__WIDX_ACTION_LBL) to window_invalidate(w) in openrct2/peep/Guest.cpp, and someone who invented the Intent system / has some plans for the general OpenRCT2 architecture should think about the proper way forward for this.

[duncanspumpkin]

From memory there used to be a field in the peep structure that indicated dirty widgets. It was up to the open window to check for dirty widgets against that and clear them if required. I would have to have a look to see if we removed that or if there is an alternative way.

[w-flo]

Ah yes, there is peep->window_invalidate_flags. It appears to be a bit under-used though. For example, PEEP_INVALIDATE_PEEP_2 is only set, but never checked. And PEEP_INVALIDATE_PEEP_STATS is only set and unset, but never checked either. INVENTORY and THOUGHTS are checked, but only when the window is getting resized.

So I'm not sure those flags work the way they are intended to work? I feel like something invalidates guest windows on a regular basis no matter what.

Anyway, it's an uint8_t, and currently there are 5 bits used for different invalidate flags, so I guess a new PEEP_INVALIDATE_PEEP_OVERVIEW could be added that invalidates the overview tab and unsets the flag if the flag is set and the overview tab is visible in the window_guest_overview_update callback.

Edit: I will give this a try and send a pull request if that's okay (probably next week, unless someone else wants to fix it first). But I'm not sure how to test it, because as I said, the label updates just fine without any invalidate calls on my setup.