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: Orientation of rail and road depots can be changed #9642

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@Kuhnovic
Copy link
Contributor

@Kuhnovic Kuhnovic commented Oct 22, 2021

Motivation / Problem

It's all to easy to place a rail depot in the wrong orientation. Railway station tiles can be "rotated" by simply building them again, I figured such a feature would be great for depots as well.

Description

If you create a depot, and you accidentally got the orientation wrong, then just select the correct orientation and click again. The existing depot is rotated, it is not destroyed/recreated, which means orders containing the depot stay intact.
I5uB4QsmEe

Update Tried it for ship depots as well, but this was problematic. I ended up removing it again.

Limitations

This only works on depots of the same (rail) type. This is by design.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
@FLHerne
Copy link

@FLHerne FLHerne commented Oct 22, 2021

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.

@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Oct 22, 2021

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 :)

@Kuhnovic Kuhnovic force-pushed the rotate_depots branch 3 times, most recently from a8a71c1 to 7141e53 Oct 22, 2021
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Oct 22, 2021

eOWKq5b7p5

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.

@Kuhnovic Kuhnovic changed the title Feature: Orientation of rail and road depots can be changed Feature: Orientation of rail, road and ship depots can be changed Oct 22, 2021
@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Oct 22, 2021

I see nothing wrong with the sliding ship depot.

@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Oct 23, 2021

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.

@LC-Zorg
Copy link

@LC-Zorg LC-Zorg commented Oct 23, 2021

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

@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Nov 12, 2021

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.

@Kuhnovic Kuhnovic changed the title Feature: Orientation of rail, road and ship depots can be changed Feature: Orientation of rail and road depots can be changed Nov 12, 2021
@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Nov 12, 2021

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

Copy link
Member

@LordAro LordAro left a comment

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?

src/rail_cmd.cpp Outdated Show resolved Hide resolved
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 6, 2022

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants