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

Merged
merged 1 commit into from Jul 1, 2023

Conversation

Kuhnovic
Copy link
Contributor

@Kuhnovic Kuhnovic commented Oct 22, 2021

Motivation / Problem

It's all too 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.

... I took this concept even further and created an option to automatically determine the depot orientation. More information below.

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 1 Tried it for ship depots as well, but this was problematic. I ended up removing it again.

Update 2

I got the idea to have the option to automatically orient a rail or road depot when you place it, based on whether it could be connected to any nearby rail. But I dediced to remove this again to keep the PR clean, and because it wasn't really finished.

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
Contributor

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

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 Compare October 22, 2021 16:01
@Kuhnovic
Copy link
Contributor Author

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

I see nothing wrong with the sliding ship depot.

@Kuhnovic
Copy link
Contributor Author

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@LordAro
Copy link
Member

LordAro commented Feb 2, 2022

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

@Kuhnovic
Copy link
Contributor Author

Kuhnovic commented Feb 4, 2022

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.

@Kuhnovic Kuhnovic force-pushed the rotate_depots branch 2 times, most recently from 2a5cbfe to 3347ea9 Compare March 8, 2022 22:17
@Kuhnovic Kuhnovic changed the title Feature: Orientation of rail and road depots can be changed Feature: Auto depot orientation Mar 8, 2022
@Kuhnovic
Copy link
Contributor Author

Kuhnovic commented Mar 8, 2022

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.

@glx22 glx22 added the preview This PR is receiving preview builds label Mar 9, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-9642 March 9, 2022 01:37 Inactive
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm strongly +1 to overbuilding depots to rotate them, and less strongly +1 about the auto rotation. It is convenient, but the only similar thing done automatically is the automatic track and road connections from depots to existing networks, which I'm already not a fan of. To be clear, I'm not against auto orientation and would use it, it just feels somehow a bit surprising to me. 😄

src/rail_cmd.cpp Outdated Show resolved Hide resolved
src/rail_cmd.cpp Outdated Show resolved Hide resolved
src/rail_cmd.cpp Outdated Show resolved Hide resolved
src/rail_cmd.cpp Outdated Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
src/road_cmd.cpp Outdated Show resolved Hide resolved
src/road_cmd.cpp Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler added the work: needs more work This Pull Request needs more work before it can be accepted label Nov 9, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-9642 April 30, 2023 20:48 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-9642 June 11, 2023 09:37 Inactive
2TallTyler
2TallTyler previously approved these changes Jun 11, 2023
src/rail_map.h Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler dismissed their stale review June 12, 2023 07:50

Additional review comment by Rubidium

@2TallTyler 2TallTyler added the work: minor details This Pull Request has some minor details left to do label Jun 29, 2023
@DorpsGek DorpsGek temporarily deployed to preview-pr-9642 June 30, 2023 09:35 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-9642 June 30, 2023 09:38 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-9642 June 30, 2023 09:39 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-9642 June 30, 2023 20:04 Inactive
@2TallTyler 2TallTyler removed the work: minor details This Pull Request has some minor details left to do label Jun 30, 2023
@2TallTyler
Copy link
Member

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. 🙂

@Kuhnovic
Copy link
Contributor Author

Kuhnovic commented Jul 1, 2023

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.

@2TallTyler
Copy link
Member

Heh, I misread the other PR. Thanks.

@2TallTyler 2TallTyler merged commit 6169e7f into OpenTTD:master Jul 1, 2023
19 checks passed
mrmbernardi pushed a commit to mrmbernardi/OpenTTD that referenced this pull request Jul 2, 2023
J0anJosep added a commit to J0anJosep/OpenTTD that referenced this pull request Aug 25, 2023
J0anJosep added a commit to J0anJosep/OpenTTD that referenced this pull request Aug 25, 2023
J0anJosep added a commit to J0anJosep/OpenTTD that referenced this pull request Aug 25, 2023
JGRennison added a commit to JGRennison/Upstream-OpenTTD that referenced this pull request Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants