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

Orders: it does not jump order by remaining lifetime #6078

Closed
DorpsGek opened this issue Aug 9, 2014 · 11 comments
Closed

Orders: it does not jump order by remaining lifetime #6078

DorpsGek opened this issue Aug 9, 2014 · 11 comments
Labels

Comments

@DorpsGek
Copy link

@DorpsGek DorpsGek commented Aug 9, 2014

telk5093 opened the ticket and wrote:

It seems that conditional order jumping by remaining lifetime works wrong.

The situations are like below. I gave orders (in a bus) like this:
(1) Go non-stop to Bus station 1
(2) Go non-stop to Bus station 2
(3) Jump to order 1 when Remaining lifetime (years) is more or equal to 0
(4) Go non-stop to the nearest Road Vehicle Depot

And let the lifetime of bus is 25 years.

Then logically the bus must go to the nearest depot when its lifetime is over.
But it does not go to the depot until its lifetime is even 45 years!

If I change Order (3) into "Jump to order 1 when Remaining lifetime (years) is more than 0", not "more or equal to", then it works normally.

Would you mind if I ask you to check this?

Reported version: 1.4.1
Operating system: All


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

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Aug 9, 2014

frosch wrote:

The remaining lifetime is clamped at 0. It is never negative. So, it should not go to the depot after 45 years either.

Actually, all conditions only use non-negative values.


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

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Aug 9, 2014

telk5093 wrote:

Oh, I didn't know that. Thanks for your comments.


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

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Sep 13, 2014

Alberth wrote:

It may be useful to warn the user of such issues, changing the bug report into an enhancement request.


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

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Sep 13, 2014

telk5093 wrote:

Thank you Alberth :)


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

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Feb 2, 2015

jogi wrote:

I implemented a patch which checks for Conditions that are always true or always false. If such a Condition is detected, the player is warned. However, the Player is not prevented of doing anything.

I'm not sure if I caught all conditions, but I think I've got the most common ones right...

Attachments


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

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Feb 7, 2015

jogi wrote:

This is a second Version.

Differences to v1 are:
- More conditions are found (I'm leaning towards all)
- It also checks the condition whether the vehicle needs servicing. If breakdowns and maintenance is disabled, it will warn you.
- some minor tweaks to conform to the coding guidelines

Attachments


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

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Feb 8, 2015

Alberth wrote:

More warnings are probably better. Maybe the red error window is not always appropriate in this case, not sure (but don't have an alternative currently).

The VERY minor issues are with the coding style:
- "if" conditions take boolean values, so "_settings_game.order.no_servicing_if_no_breakdowns == true" can be written as "_settings_game.order.no_servicing_if_no_breakdowns" (don't compare with 'true' or 'false').
- "//Deliberate Passthrough" is written as "/* FALL THROUGH */", check some other files. Alternatively, you can rewrite that switch to a "if", but that's just personal preference.

The big issue is the place where you check and throw error messages to the user.

Cmd* functions perform the actual command, and are executed at every machine that takes part in the game. In single-player, that is just your machine. In multi-player, that is at every machine in the game. That means you throw error messages to all users in the game, including all other players, and the server, which may not even have a display.


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

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Feb 9, 2015

jogi wrote:

Thank you for your Feedback.

I have changed the checks to be in the DrawOrder function. Also, the rulechange with servicing is caught in the CheckOrders function. The rest is not checked there, because there is no way that it can change except when the player is in the order window and sees the message anyway.

Attachments


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

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Apr 13, 2018

If this patch meets standards, this would be a good pull request (the issue is specific, detailed, and the patch has already had review and revision).

Bug or enhancement? Dunno :) Better that it were fixed though, less user confusion.

@andythenorth

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Jan 5, 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 (in 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!

@andythenorth

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Jan 12, 2019

One last chance before I close this one:

Final patch from jogi applies and compiles cleanly: https://bugs.openttd.org/task/6078/getfile/10126/FS6078v3.patch

I won't test it, conditional order stuff breaks my brain.

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
3 participants
You can’t perform that action at this time.