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

Fix #7847: Use ViewportSign coordinates for sign Kdtree coordinates #7849

Merged
merged 6 commits into from Dec 1, 2019

Conversation

@nielsmh
Copy link
Contributor

nielsmh commented Nov 30, 2019

Fix #7847 by ensuring the same coordinates are used for a station sign regardless of how the landscape changes below it after the coordinates were first determined.

By keeping track of whether the ViewportSign is valid for Kdtree use (and only ever registering the station viewport sign when the sign object is valid) a lot of code can be simplified, and it should prevent a lot of similar bugs too.

I think this is the real solution to all Kdtree bugs regarding station signs. I think something similar needs to be done for town names and player signs too.

…d() and ViewportSignKdtreeItem::MakeStation()
nielsmh added 5 commits Nov 30, 2019
…or stations

Keeping track of whether the ViewportSign data are valid for Kdtree usage allows moving more housekeeping to Station::UpdateVirtCoord and Waypoint::UpdateVirtCoord and helps simplify a lot of other code.
…ions

It's the more correct approach, it was only not usable before due to inconsistencies with when sign positions were updated.
@nielsmh nielsmh force-pushed the nielsmh:fix-7847 branch from 63f91da to ded51b7 Dec 1, 2019
@nielsmh nielsmh changed the title Fix #7847: Use ViewportSign coordinates for sign Kdtree coordinates for stations Fix #7847: Use ViewportSign coordinates for sign Kdtree coordinates Dec 1, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Dec 1, 2019

Tested #7371 report, PSG 201 save remains loadable without crashing.

Tested #7440 report, is still able to create and then remove a town in scenario editor.

Tested #7481 and #7530 via the save provided in the latter, seems to pass the oil rig deletion.

@glx22
glx22 approved these changes Dec 1, 2019
Copy link
Contributor

glx22 left a comment

Looks good to me

@nielsmh nielsmh merged commit 9900af3 into OpenTTD:master Dec 1, 2019
8 checks passed
8 checks passed
OpenTTD CI #20191201.4 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@nielsmh nielsmh deleted the nielsmh:fix-7847 branch Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.