Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kdtree.hpp crash when Oil Rig is removed #7481

Closed
spnda opened this issue Apr 6, 2019 · 5 comments
Closed

kdtree.hpp crash when Oil Rig is removed #7481

spnda opened this issue Apr 6, 2019 · 5 comments
Assignees
Labels

Comments

@spnda
Copy link
Contributor

@spnda spnda commented Apr 6, 2019

Version of OpenTTD

Newest Master (OpenTTD 20190406-master-gdef5d99e0e)

Expected result

The game should carry on normally.

Actual result

The game crashes unexpectedly.
Sometimes even, multiple Error windows appear before the game closes itself.

Steps to reproduce

  1. Start any new game.
  2. Build any route with a train or bus and let a vehicle run on it.
  3. Turn off "full animations".
  4. Let the game run for 50-100 (or more) years in fast forward.

The game will crash with either of the two message after some time:

Assertion failed at line 213 of d:\projects\c++\openttd\src\core\kdtree.hpp: next != INVALID_NODE

Assertion failed at line 2965 of d:\projects\c++\openttd\src\window.cpp: HasModalProgress() || IsLocalCompany()

Log files

crash.png:

crash

crash.log:

hktree.hpp error => crash.log
window.cpp error => crash.log
Crash with multiple error windows => crash.log

@spnda spnda changed the title Changing elements of subtree while fast forwarding crashes the game Fast forwarding very fast can cause window.cpp/kdtree.hpp crash Apr 6, 2019
@LordAro

This comment has been minimized.

Copy link
Member

@LordAro LordAro commented Apr 6, 2019

(gdb) bt
#0  0x00007ffff5327d7f in raise () from /usr/lib/libc.so.6
#1  0x00007ffff5312672 in abort () from /usr/lib/libc.so.6
#2  0x00007ffff5312548 in __assert_fail_base.cold.0 () from /usr/lib/libc.so.6
#3  0x00007ffff5320396 in __assert_fail () from /usr/lib/libc.so.6
#4  0x0000555555b50d58 in Kdtree<ViewportSignKdtreeItem, int (*)(ViewportSignKdtreeItem const&, int), int, int>::RemoveRecursive (
    this=this@entry=0x55555713cd00 <_viewport_sign_kdtree>, element=..., node_idx=<optimized out>, node_idx@entry=26, 
    level=level@entry=4) at /home/lordaro/dev/openttd/src/core/kdtree.hpp:187
#5  0x0000555555b50c5c in Kdtree<ViewportSignKdtreeItem, int (*)(ViewportSignKdtreeItem const&, int), int, int>::RemoveRecursive (
    this=this@entry=0x55555713cd00 <_viewport_sign_kdtree>, element=..., node_idx=<optimized out>, node_idx@entry=25, 
    level=level@entry=3) at /home/lordaro/dev/openttd/src/core/kdtree.hpp:187
#6  0x0000555555b50c5c in Kdtree<ViewportSignKdtreeItem, int (*)(ViewportSignKdtreeItem const&, int), int, int>::RemoveRecursive (
    this=this@entry=0x55555713cd00 <_viewport_sign_kdtree>, element=..., node_idx=<optimized out>, node_idx@entry=24, 
    level=level@entry=2) at /home/lordaro/dev/openttd/src/core/kdtree.hpp:187
#7  0x0000555555b50c5c in Kdtree<ViewportSignKdtreeItem, int (*)(ViewportSignKdtreeItem const&, int), int, int>::RemoveRecursive (
    this=this@entry=0x55555713cd00 <_viewport_sign_kdtree>, element=..., node_idx=<optimized out>, node_idx@entry=16, 
    level=level@entry=1) at /home/lordaro/dev/openttd/src/core/kdtree.hpp:187
#8  0x0000555555b50c5c in Kdtree<ViewportSignKdtreeItem, int (*)(ViewportSignKdtreeItem const&, int), int, int>::RemoveRecursive (
    this=this@entry=0x55555713cd00 <_viewport_sign_kdtree>, element=..., node_idx=<optimized out>, level=level@entry=0)
    at /home/lordaro/dev/openttd/src/core/kdtree.hpp:187
#9  0x0000555555b50dbe in Kdtree<ViewportSignKdtreeItem, int (*)(ViewportSignKdtreeItem const&, int), int, int>::Remove (
    this=0x55555713cd00 <_viewport_sign_kdtree>, element=...) at /home/lordaro/dev/openttd/src/core/kdtree.hpp:407
#10 0x0000555555b5fbc8 in Station::~Station (this=0x55555a676f30, __in_chrg=<optimized out>)
    at /home/lordaro/dev/openttd/src/station.cpp:167
#11 0x0000555555b5fcf9 in Station::~Station (this=0x55555a676f30, __in_chrg=<optimized out>)
    at /home/lordaro/dev/openttd/src/station.cpp:90
#12 0x00005555559be073 in Industry::~Industry (this=0x555559fb0210, __in_chrg=<optimized out>)
    at /home/lordaro/dev/openttd/src/industry_cmd.cpp:154
#13 0x00005555559be2c8 in IndustryMonthlyLoop () at /home/lordaro/dev/openttd/src/industry_cmd.cpp:2800
#14 0x000055555593d267 in OnNewMonth () at /home/lordaro/dev/openttd/src/date.cpp:240
#15 0x000055555593d6d5 in IncreaseDate () at /home/lordaro/dev/openttd/src/date.cpp:298
#16 0x0000555555a8084a in StateGameLoop () at /home/lordaro/dev/openttd/src/openttd.cpp:1382
#17 0x0000555555a80a8e in GameLoop () at /home/lordaro/dev/openttd/src/openttd.cpp:1471
#18 0x0000555555beb6cf in VideoDriver_SDL::MainLoop (this=0x5555571ed5f0) at /home/lordaro/dev/openttd/src/video/sdl_v.cpp:762
#19 0x0000555555a7f274 in openttd_main (argc=<optimized out>, argv=<optimized out>) at /home/lordaro/dev/openttd/src/openttd.cpp:857
#20 0x00007ffff5314223 in __libc_start_main () from /usr/lib/libc.so.6
#21 0x000055555588f79e in _start () at /home/lordaro/dev/openttd/src/industry_cmd.cpp:2944
@LordAro LordAro added the bug label Apr 6, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor

@nielsmh nielsmh commented Apr 7, 2019

Industry being removed, and a station being removed as a result of that... might be an oil rig station removal.

@JGRennison

This comment has been minimized.

Copy link
Contributor

@JGRennison JGRennison commented Apr 8, 2019

A few thoughts on this issue (without actually testing anything, so caveats apply):

In ViewportSignKdtreeItem::MakeStation there is the line:
if ((st->facilities & FACIL_AIRPORT) && st->airport.type == AT_OILRIG) pt.y -= 16 * ZOOM_LVL_BASE;
This branch will be taken when an oil rig is created/updated, but will not be taken when the oil rig is removed, as FACIL_AIRPORT is cleared in DeleteOilRig.
The add and remove coordinates will not match, and so this could trigger an error of the form above.

The general use of RemapCoords2 appears to be potentially problematic as the kd tree coordinate depends on the GetSlopePixelZ height of the station xy tile, however changes to the height and/or foundation state of this tile do not result in the kd tree being updated. Therefore if GetSlopePixelZ returns different values when the station is added to and removed from the kd tree, an error of the form above could result.

@LordAro LordAro changed the title Fast forwarding very fast can cause window.cpp/kdtree.hpp crash kdtree.hpp crash when Oil Rig is removed Apr 11, 2019
@LordAro LordAro added the regression label Apr 11, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor

@nielsmh nielsmh commented Apr 25, 2019

I can neither reproduce any errors related to oil rig removal nor any to stations where the heights/slopes have changed since construction.

@JGRennison

This comment has been minimized.

Copy link
Contributor

@JGRennison JGRennison commented Apr 25, 2019

The crash will only occur if the discrepancy between the element coordinate and the removal coordinate is such that RemoveRecursive looks into the wrong sub-tree when descending.
If use of the removal coordinate ends up at the correct leaf-node then all is well even if the removal coordinate is a bit off, as operator== doesn't compare the coordinate itself. Most of the time this will be the case so you would need to be somewhat (un)lucky for anything to go wrong.

For reference, my fix is here: JGRennison/OpenTTD-patches@070160a, it seems to have stopped the kdtree-related bug reports at my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.