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
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

telk5093 wrote:

Thank you Alberth :)


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

@DorpsGek
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 7, 2018
@andythenorth
Copy link
Contributor

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 andythenorth added patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay bug Something isn't working and removed enhancement from FlySpray bug Something isn't working labels Apr 13, 2018
@frosch123 frosch123 removed the Core label Apr 14, 2018
@andythenorth andythenorth added the stale Stale issues label Jan 5, 2019
@andythenorth
Copy link
Contributor

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
Copy link
Contributor

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.

@andythenorth andythenorth removed patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay stale Stale issues labels Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Development

No branches or pull requests

3 participants