A patch from a beginner

[chindit]

Hi everybody.

Today, I'm very proud. I've made my first patch. Snif…

I'm not a pro at coding and it's the first time I collaborate to the code of an opensource project, so, I don't want to do any mistake.

I've decided to solve a bug witch allows you to create a conditional order with «less than 0» reliability. If you do that, conditional jump will be skipped because reliability will never be lower than 0.

To replicate the bug:
Create 3 stations and one (or more) vehicules with these orders:
1)Go to A
2)Go to B
3)CONDITIONAL : Jump to 1 if Reliability is LESS THAN 0
4)Go to C

Reliability is never lower than 0 so… it's kinda stange, no? Vehicules never jump to order 1.

I've solved the problem with the patch joined to this post.

So, I've a few questions:
1)Is my patch correct?
2)If it's OK, how can I submit it?

I've compiled and tested it and it works fine.

Thanks a lot
Chindit

EDIT: Patch updated. Latest version also in this post.

[Simons Mith]

Thanks for looking at this, but having encountered the 'remaining lifetime' issue recently I thought I'd mention it to you.

Is this patch of yours also going to apply to load percentage less than zero - another impossiblity?
What about age?
Etc.?

(No criticism is intended, I just think altering one but leaving the others untouched bears thinking about.)


In particular, what about remaining lifetime? At the moment, remaining lifetime /can't/ go below zero although perhaps it should, so a jump that ought to trigger - e.g when a vehicle is 25 years old but had an operating life of 22 years - also never happens.


My preference would be to keep things as consistent as possible - and eliminate inconsistencies where possible too - and at present there seems to be an unwritten rule that none of these values are permitted to go negative (bear in mind you can't enter negative values to compare them against anyway), and percentages shouldn't go higher than 100%


But for vehicle lifetime, which is measured in years, the game might need another parameter (perhaps call it 'lifetime exceeded by (years)') in order to detect vehicles older than their operating life. Or else use 'lifetime' as a percentage, so that 0% = new, 100%=operating life, >100% = older than operating life. But that would mean comparing against percentages greater than 100%, also something the game doesn't normally do.


Even the simplest patches in this game can have unexpected ramifications. I certainly don't expect anyone to do all of these, but they are related issues that I feel ought to be considered in the light of your patch.

[Edits]

[chindit]

Thanks for looking at this, but having encountered the 'remaining lifetime' issue recently I thought I'd mention it to you.

Is this patch of yours also going to apply to load percentage less than zero - another impossiblity?
What about age?
Etc.?

Facepalm!
You're right!
I've changed the patch to reflect this.
Now, the game will react this way:
IF conditional order AND condition «LESS THAN 0».
If the value (doesn't matter which value) is 0, condition is valid.
So, «LESS THAN 0» would have the same effect as «EQUAL 0» when the value given (load percentage, remaining lifetime or other) is 0.

In particular, what about remaining lifetime? At the moment, remaining lifetime /can't/ go below zero although perhaps it should, so a jump that ought to trigger - e.g when a vehicle is 25 years old but had an operating life of 22 years - also never happens.

I don't want to change a lot in the game.
I've discovered the code yesterday and I don't know all its ramifications.

But for vehicle lifetime, which is measured in years, the game might need another parameter (perhaps call it 'lifetime exceeded by (years)') in order to detect vehicles older than their operating life. Or else use 'lifetime' as a percentage, so that 0% = new, 100%=operating life, >100% = older than operating life. But that would mean comparing against percentages greater than 100%, also something the game doesn't normally do.

I need to check how lifetime works, but I can't set an arbitrary limit at 100 for the «MORE THAN» condition because not every value came into % format.

I don't understand what you want to do with the vehicle lifetime. I'm currently checking what happend when using «Remaining Lifetime LESS THAN 0».

New patch is attached. You can test it.

[Simons Mith]

Yeah, the remaining lifetime bit and how best to address it is a bit of a puzzle. I certainly don't want to imply you should try to fix it. I have logged a bug (FS#6359), but TBH at the moment I don't think there is a good solution and I'm still mulling things over. A 'lifetime exceeded' or a 'max reliability' parameter (see the bug report) are the best possibilities I've thought of so far, but either would add yet another filter criterion to the orders list, which is far from ideal.

[chindit]

Yeah, the remaining lifetime bit and how best to address it is a bit of a puzzle. I certainly don't want to imply you should try to fix it. I have logged a bug (FS#6359), but TBH at the moment I don't think there is a good solution and I'm still mulling things over. A 'lifetime exceeded' or a 'max reliability' parameter (see the bug report) are the best possibilities I've thought of so far, but either would add yet another filter criterion to the orders list, which is far from ideal.

There is no problem with «Remaining lifetime» and «LESS THAN 0»
I've tried with a simple set of orders:

(Yes, it's in spanish)

My trucks, even at 26 years old for 15 respect the LESS THAN so, there is no problem for me.
So, you task FS#6359 seems to be fixed.

I'll see if it's possible (at least for me) to add a new filter like «Max reliability».

[Alberth]

You cannot interpret "<" as "<=". People may get confused, or get the wrong behavior.
Remaining lifetime 0 years may start (1 year - 1 day) before the real end (did you check that?).

If people write "<", they should get "<" behavior. (There might even be people that use this for disabling parts of the orders, who knows?)

There are two ways out of the situation you are trying to fix.
- Warn the user that he is writing useless conditions
- Refuse to accept the "<" and 0 combination.

Note that there are similar cases at the 100% side as well.
You may want to check FS#6078, as it also deals with these problems. Maybe there are other issues about it as well?



Edit: As for your patch, you should use TAB characters for leading whitespace
Lines containing only comment use /* */ comments
There are non-ascii characters in your comment line, which are not allowed by the C++ standard, afaik

[Eddi]

possibly you can throw a news message in the "check orders" routine that warns about unfulfillable (or tautologic) conditional jumps

[chindit]

«Faire et défaire, c'est toujours travailler!»

Sooooo…
I doesn't agree with Albert.
For me, if you say «LESS THAN 0 Reliability», you necessarily means «LESS OR EQUALS TO 0 Reliability».
But I'm new and I don't have my say.
So, I've made a new patch which will check if there is a conditional order and if this order is or not unfulfillable.
If the order is unfulfillable, a warning message will be shown.

You can adapt the message, I didn't had a lot of imagination.

[Alberth]

You would be happy if C++ changed the meaning of a comparison?

[chindit]

You would be happy if C++ changed the meaning of a comparison?

It's not related to C++.
It's just game mechanics.

But it doesn't really matter.
Patch is done and just add a new message.

[Eddi]

it's not related to C++. ANY context where logic is involved, 0 is NOT less than 0. 0 is EQUAL to 0. it's not up to the game to pop up a paperclip saying "do you need help with that?" it must assume that when you said "less than 0" you actually meant exactly that, an unfulfillable condition as long as 0 is the lowest number.

[Zhall]

"Pulls up the bleachers"

Continue gentlemen.

[chindit]

Ok ok, let's (almost) close this.
There is just one point I want to talk about: remaining lifetime.
Currently, it can't be negative but it should.
It's logical to think if a vehicle has a lifetime of 12 years and is in service for 15 years, it's remaining lifetime could be -3 years. So, "less than" 0.

And you haven't said what you though about my message's patch (which do not alterate anything)

[Eddi]

Ok ok, let's (almost) close this.
There is just one point I want to talk about: remaining lifetime.
Currently, it can't be negative but it should.
It's logical to think if a vehicle has a lifetime of 12 years and is in service for 15 years, it's remaining lifetime could be -3 years. So, "less than" 0.

i already gave my opinion to that in the other thread

And you haven't said what you though about my message's patch (which do not alterate anything)

i would probably refactor the if-condition into a switch, so the variable is read only once, and then compared to a set of values.

Code: Select all

switch(variable) {
  case BLA: case FOO: case BAR:
    set message;
    break;
  default:
    break;
}

[chindit]

i would probably refactor the if-condition into a switch, so the variable is read only once, and then compared to a set of values.

Good idea. I though about an array but OpenTTD doesn't seems to use STD that much and I didn't wan't to use it.
I've modified the code with a switch.

[chindit]

Latest update:

Message alert
If there is an unfulfillable condition, show an alert

Remaining lifetime
Now, Remaining Lifetime can be negative.

This is a fix for FS#6359
I've also posted a comment on the bug report.