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

Feature: Moveable depots #6328 #7051

Closed
wants to merge 6 commits into from
Closed

Feature: Moveable depots #6328 #7051

wants to merge 6 commits into from

Conversation

@Eddi-z
Copy link
Contributor

Eddi-z commented Jan 12, 2019

patch imported from flyspray
minor conflict resolved
savegame bump rebased (TODO: put correct version info) now resolved with new saveload enum
patch queue seems "unclean" (partial patch did not compile, whole queue does) now resolved by reordering some patches

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jan 12, 2019

Gave this 5 minutes of testing, initial impressions are that all worked as expected. Vehicle orders are preserved for up to 1 month, then invalidated if the depot isn't rebuilt by then. The search radius for 'preserve depot' seems to be about 8 tiles or so. In use, it's pretty neat.

FWIW, adf88 considered there was more to do on the original patch #6328 (comment)

However it might be useful as is.

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jan 12, 2019

From CI: 2019-01-12T22:39:23.2400344Z Running ai/regression/tst_regression... failed!

@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Jan 13, 2019

First regression error is at main.nut:1062, it's because the rail depot list is empty but it shouldn't as a rail depot as been built.

Second regression error is at main.nut:1167, it's because the road depot list actually contains the water depot list.

@Eddi-z Eddi-z force-pushed the Eddi-z:6328 branch from 0b622bb to aa2f4f5 Jan 13, 2019
@Eddi-z Eddi-z force-pushed the Eddi-z:6328 branch 2 times, most recently from 1232d3c to 2fc136e Feb 5, 2019
@@ -2092,7 +2078,7 @@ bool UpdateOrderDest(Vehicle *v, const Order *order, int conditional_depth, bool
v->IncrementRealOrderIndex();
} else {
if (v->type != VEH_AIRCRAFT) {
v->SetDestTile(Depot::Get(order->GetDestination())->xy);
v->SetDestTile(v->GetOrderDepotLocation(order->GetDestination()));
} else {

This comment has been minimized.

Copy link
@Eddi-z

Eddi-z Feb 5, 2019

Author Contributor

i'm not 100% sure i correctly resolved conflicts with 3e0e3cf and 81330b8 here

@PeterN PeterN added this to the 1.10.0 milestone Mar 4, 2019
@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Mar 21, 2019

@Eddi-z will you update this?

@Eddi-z Eddi-z force-pushed the Eddi-z:6328 branch 2 times, most recently from bf40b42 to df86ea9 Mar 21, 2019
@Eddi-z

This comment has been minimized.

Copy link
Contributor Author

Eddi-z commented Mar 21, 2019

So, i resolved the saveload part, but the viewport part is way over my head with the kdtree changes.

@Eddi-z Eddi-z force-pushed the Eddi-z:6328 branch from df86ea9 to 6e1e6ec Mar 21, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Apr 4, 2019

I rebased this branch and updated it to use Kdtree here: https://github.com/nielsmh/OpenTTD/tree/movable-depots
Since I rebased I can't make a PR, and I don't want to push to Eddi's branch without an ok first.

Update: Got an ok on IRC.

@nielsmh nielsmh force-pushed the Eddi-z:6328 branch from 6e1e6ec to 3146108 Apr 4, 2019
src/depot.cpp Outdated Show resolved Hide resolved
src/depot_cmd.cpp Outdated Show resolved Hide resolved
@Eddi-z Eddi-z force-pushed the Eddi-z:6328 branch from 3146108 to 5dbdcbe Apr 7, 2019
@Eddi-z Eddi-z force-pushed the Eddi-z:6328 branch from 5dbdcbe to 723e2e8 Apr 7, 2019
@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Apr 8, 2019

Found a crash:

  1. Build a depot
  2. Build an engine
  3. Sell the engine
  4. Bulldoze the depot
  5. Profit?

Something to do with Order backups...

@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Aug 17, 2019

Hi, there's been no activity on this for over 4 months. If no more effort will be put into this PR, it will be closed in the next week or so

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Oct 22, 2019

I think we can let this one slide into the darkness. Moving depots is a nice feature if you use escape depots or similar, but it's not essential.

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