I like this in principle, definitely would have saved me some annoyance.

For consistency the same could be applied to ship depots, if one tile of the new depot is over the old one.

Have you considered keeping the same depot when overbuilding? As I understand it, this change will cause any order lists including the original depot to gain '(Invalid Order)', and not allow overbuilding if any vehicle is in the depot.
For the most common use case ("oops I built the wrong angle") that doesn't matter, but preserving depot identity and contents might be useful when altering track layout later. Not sure it would be useful enough to be worth implementing tbh.

Code looks good to me for what it does. Pity about the duplication, but that's not this patch's fault. Regression test failure looks like it just needs the expectations updating.

I like this in principle, definitely would have saved me some annoyance.

For consistency the same could be applied to ship depots, if one tile of the new depot is over the old one.

Have you considered keeping the same depot when overbuilding? As I understand it, this change will cause any order lists including the original depot to gain '(Invalid Order)', and not allow overbuilding if any vehicle is in the depot. For the most common use case ("oops I built the wrong angle") that doesn't matter, but preserving depot identity and contents might be useful when altering track layout later. Not sure it would be useful enough to be worth implementing tbh.

Code looks good to me for what it does. Pity about the duplication, but that's not this patch's fault. Regression test failure looks like it just needs the expectations updating.

I tried to err on the side of caution by simply automating what the user would do: destroy the depot and create it again. That way I'm pretty certain that I don't mess anything up regarding signals/reservation/vehicles-in-depot etc. You definitely have a point, but I see the situation you describe is a bit of an edge case. I think most people will just use this feature to rectify an initial placement mistake. Then again, everybody has it's own style and habits when it comes to openttd...

I'll try to fix the regression test later this weekend :)

Implemented the functionality for ship depots as well. Not entirely sure if we should allow them to "slide" forward as seen in the GIF, but this can easily be fixed if needed.

I see nothing wrong with the sliding ship depot.

Have you considered keeping the same depot

@FLHerne I figured out how to turn the depot without destroying it, at least for road and train depots. No more void orders. I tried it for ship depots as well, but if the depot position changes while a ship is heading for the depot, it messes with the ship controller and the asserts start flying. Obviously it was not created with moving depots in mind :P. I couldn't find a good solution, so I stuck to the simple but crude "destroy and rebuild" method for ship depots.

I see nothing wrong with the sliding ship depot.

If it changes the depot ID, I see a problem with it - it doesn't concern me, but I know it will be a problem for others.

1. The player may accidentally move the depot and destroy his ships orders
2. The player may move the depot on purpose, unaware that he is destroying his ships orders

Depots may be far enough away from the ship's path that they will never be serviced.
It can be a big problem finding all ships that had this depot in their orders to fix those orders.

If the above problem cannot be avoided, it is better not to add this feature to the shipyards - PR will still be very good. :) But if it succeeds, it will be great :D

I decided to revert my changes for the ship depot, I couldn't get it work reliably. Since ship depots only have 2 orientations to begin with, the chances of the user messing are also smaller.

I don't think I've ever placed a ship depot in the wrong orientation, because it's so obvious when aligned incorrectly.

Would be nice if the regression test could be extended to actually test that you can overbuild depots - perhaps shouldn't be too much effort to figure out the existing code?

I tried to add some test cases for rotating depots but it breaks the regression tests. I simply can't find the reason why it's failing. Any help with this would be greatly appreciated :)

I think it's fine, it's just that the extra BuildRoadDepot call has increased the ticks slightly, so certain values like vehicle running cost(?) and positions(?) are subtly different. Just update the reference file as necessary :)

I think it's fine, it's just that the extra BuildRoadDepot call has increased the ticks slightly, so certain values like vehicle running cost(?) and positions(?) are subtly different. Just update the reference file as necessary :)

Are you sure? I can understand that a list order becomes slightly different, but this change in particular looks a little scary to me:

9268: -     Should be:          -5947
9268: +     Should be:          -5946'

This originates from these lines in regression/main.nut:

bank = AICompany.GetBankBalance(AICompany.COMPANY_SELF);

print("  SellVehicle(13):      " + AIVehicle.SellVehicle(13));
print("  IsInDepot():          " + AIVehicle.IsInDepot(12));
print("  IsStoppedInDepot():   " + AIVehicle.IsStoppedInDepot(12));
print("  StartStopVehicle():   " + AIVehicle.StartStopVehicle(12));
print("  IsInDepot():          " + AIVehicle.IsInDepot(12));
print("  IsStoppedInDepot():   " + AIVehicle.IsStoppedInDepot(12));
print("  SendVehicleToDepot(): " + AIVehicle.SendVehicleToDepot(12));
print("  IsInDepot():          " + AIVehicle.IsInDepot(12));
print("  IsStoppedInDepot():   " + AIVehicle.IsStoppedInDepot(12));

bank_after = AICompany.GetBankBalance(AICompany.COMPANY_SELF);

print("  --Accounting--");
print("    GetCosts():         " + accounting.GetCosts());
print("    Should be:          " + (bank - bank_after));

I interpreted this as "some money is lost somewhere", it might be rounding error, it might be something more serious.

Scope creep FTW! I've added a feature to automatically determine the orientation of a depot when it's built. More details in the description above.

I reverted my changes for the regression tests so we can at least have green builds and playtesting for now.

Code looks good and my tests all work.

Should the player have to pay for rotating an existing depot? We seem to have set some sort of precedent in #9852 to not pay for these sorts of things. I don't have a strong opinion either way, but I do like being consistent. 🙂

Code looks good and my tests all work.

Should the player have to pay for rotating an existing depot? We seem to have set some sort of precedent in #9852 to not pay for these sorts of things. I don't have a strong opinion either way, but I do like being consistent. 🙂

I had a look at #9852 and I actually think it's pretty consistent the way it is now. For railway stations you only pay when the orientation is changed. The same is happens for rail/road depots: you get an STR_ERROR_ALREADY_BUILT error the depot is already in the current orientation, and you don't pay anything.

Heh, I misread the other PR. Thanks.