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

Jump to bottom

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
@nielsmh
Labels
bug Something isn't working regression It used to work, and now it's broken.

Comments

@spnda
Copy link
Contributor

spnda commented Apr 6, 2019 •
edited

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
Copy link
Member

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 Something isn't working label Apr 6, 2019
@nielsmh
Copy link
Contributor

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
Copy link
Contributor

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 It used to work, and now it's broken. label Apr 11, 2019
@nielsmh
Copy link
Contributor

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
Copy link
Contributor

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.

@LordAro LordAro mentioned this issue Apr 25, 2019
Closed
@nielsmh nielsmh mentioned this issue Apr 26, 2019
Closed
nielsmh added a commit to nielsmh/OpenTTD that referenced this issue Apr 26, 2019
@nielsmh
Fix OpenTTD#7481: Just remove oil rig stations right away, don't clea…
1bd503e 
…n them first
@nielsmh nielsmh closed this as completed in d1ef13f May 3, 2019
@James103 James103 mentioned this issue Nov 29, 2019
Closed
@nielsmh nielsmh mentioned this issue Dec 1, 2019
Merged
douiwby pushed a commit to douiwby/OpenTTD that referenced this issue Apr 16, 2020
@nielsmh @douiwby
Fix OpenTTD#7481: Just remove oil rig stations right away, don't clea…
dd46d1e 
…n them first
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nielsmh nielsmh

Labels
bug Something isn't working regression It used to work, and now it's broken.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nielsmh @JGRennison @LordAro @spnda