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

Calling VehicleEnterDepot for a train frees the reservation of the depot #5859

Closed
DorpsGek opened this issue Jan 11, 2014 · 6 comments
Closed

Calling VehicleEnterDepot for a train frees the reservation of the depot #5859

DorpsGek opened this issue Jan 11, 2014 · 6 comments

Comments

@DorpsGek
Copy link

@DorpsGek DorpsGek commented Jan 11, 2014

juanjo opened the ticket and wrote:

This is just a small simplification of CheckTrainStayInDepot().

There is no need to reserve the depot for servicing in a depot. Moreover, servicing a train automatically frees the reservation.

Attachments

Reported version: Version?
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/5859
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Jan 11, 2014

juanjo wrote:

I forgot to add the description of the task. This is not a bug but a codechange and the patch is applied against r26234. Sorry.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5859#comment12911
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Feb 11, 2015

planetmaker wrote:

Are you sure it needs not reservation on the depot tile, thus that it cannot collide with a train leaving it?


This comment was imported from FlySpray: https://bugs.openttd.org/task/5859#comment13770
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Sep 2, 2017

andythenorth wrote:

Closed and reopened - frosch disagreed with my closure reasons.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5859#comment14728
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Sep 2, 2017

juanjo wrote:

I'm sorry I didn't answer. I have checked the patch again.

Comparison before-after:

Situation: Checking if it can leave the depot. Train is inside the depot and next order tries to re-enter the same depot.

Before:
Entering the if-statement, always leads to a "return true".
If depot has no reservation, VehicleEnterDepot is called.
If depot has no reservation, a SetReservation on tile is called. But with no effect, because VehicleEnterDepot is called immediately, clearing the reservation of the depot.
The comment says we need a reservation, but it is not true (it is just a hidden workaround).

After:
Entering the if-statement, always leads to a "return true".
If depot has no reservation, VehicleEnterDepot is called.
So... the lines inside the if-statement could be read as "service the train only if there is no reservation (there is no other train in the middle)".

Both versions do the same, but the second version is clearer: Only re-enter if no other train has a reservation of the depot, because calling VehicleEnterDepot would remove the reservation of the second train (causing a bug).

Of course, we could move some lines from VehicleEnterDepot to rail_cmd.cpp just before VehicleEnterDepot is called. This way, we wouldn't need the "check depot has no reservation" workaround.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5859#comment14734
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Sep 2, 2017

juanjo wrote:

The issue description is wrong.
"Servicing a train automatically frees the reservation" should have been "calling VehicleEnterDepot for a train frees the reservation of the depot".


This comment was imported from FlySpray: https://bugs.openttd.org/task/5859#comment14735
@andythenorth

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Jan 9, 2019

Thanks for this. There's been no activity on this for some time, and as it stands, it doesn't look likely that it will go any further. Since OpenTTD moved to GitHub, we use pull requests rather than patches, as they are a much more productive workflow.

I'm planning to close this soon (within 7 days), as we try to keep the issue count low for OpenTTD, it helps us focus on things that are important and fun.

If you would like to continue with this patch, the best way would be to move the patch to your own GitHub fork, update it for the current OpenTTD master, and then create a pull request. For more information, please see our CONTRIBUTING.md.

We are also happy to discuss directly on the issue, or in #openttd irc, including help to get this into a pull request. Thanks for your contribution!

J0anJosep added a commit to J0anJosep/OpenTTD that referenced this issue Jan 12, 2019
LordAro added a commit that referenced this issue Jan 12, 2019
@LordAro LordAro closed this Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.