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

Cargo callback "profit" probably broken #209

Closed
TheDude-gh opened this issue Apr 24, 2021 · 9 comments
Closed

Cargo callback "profit" probably broken #209

TheDude-gh opened this issue Apr 24, 2021 · 9 comments

Comments

@TheDude-gh
Copy link

@TheDude-gh TheDude-gh commented Apr 24, 2021

Cargo callback "profit" behaves strangely.

The problem is that when returning negative values, the cargo payment is for some values actually positive. Not sure if that is intended, but seems counter-intuitive.

Also cargo property "price_factor" is strange. Wiki states it is float type, but does not tells any lmit. In the NFO it is DWORD, so basically UINT32.
Anyway, its behaviour is not linear. When its value is increased lineary, the cargo payment sometimes drops back to lower values.

@frosch123
Copy link
Member

@frosch123 frosch123 commented Apr 24, 2021

Please add some example code.

Loading

@frosch123
Copy link
Member

@frosch123 frosch123 commented Apr 24, 2021

About the callback, see OpenTTD/OpenTTD#9095 (was wrong)

Loading

@TheDude-gh
Copy link
Author

@TheDude-gh TheDude-gh commented Apr 24, 2021

Example code here.

cargo-profit.zip

Not sure about #9095. It makes sense tried it, but it still does not work, there must be something else.

One thing is it seems to me NML is parsing the proft return values incorrectly.
As I understood from NFO wiki, bit 15 is return flag, bit 14 is sign flag, and there si unsigned 14 bits for value, so when I code return value like 0xC064, what I get is -100.
But when I code -100 to NML, the result return value in HEX code is 0xFF9C, which is of course correct for signed integers, but it does not return expected values, the openttd source does not treat it like this. Variable callback is uint16.
I ran some debugging in the GetTransportedGoodsIncome function and the values are different than what I would have expected.

Honestly I don't see what is wrong at the moment.

Loading

@PeterN
Copy link
Member

@PeterN PeterN commented Apr 24, 2021

C064 is wrong for -100. It would be correct if the result is simply flipped, but it's not.
FF9C is correct for -100. FF9C -> 3F9C - 4000 -> -0064 => -100 decimal.

Loading

@TheDude-gh
Copy link
Author

@TheDude-gh TheDude-gh commented Apr 24, 2021

Ok, I agree. But still. Returning -100 in NML profit callback results in positive cargo payment even with the #9095 fix.

Loading

glx22 added a commit to glx22/nml that referenced this issue Apr 24, 2021
glx22 added a commit to glx22/nml that referenced this issue Apr 24, 2021
@TheDude-gh
Copy link
Author

@TheDude-gh TheDude-gh commented Apr 25, 2021

Fix #209 seems it might improve the problem.

I can't try it, because there is not new NML binary and compiling from python is not my thing.

For more consideration though, see attached zip.

cargo-profit2.zip

In commented nfo/cargo-profit.nfo is NFO code as I would write it for the same thing I am trying in NML. It's quite simple and works fine.

In commented nfo/cargo-profit.nml-decoded.nfo is the decoded code from GRF coded from the NML source.

Here you can see NML adds lot of clutter code, but ok, if it works, it works.
What is strange to me is the multiplication by 329 and division by 256.
Ok, it corresponds with the NML python source, but why is it there? It actually alters the return values from NML grf code by some 1.29. So when I write to my code -100, what I get is -129.
And even if that has some reason, why would you do this value alteration in GRF code and not alter it inside NML compilation and putting that final value to GRF?

Loading

frosch123 added a commit to frosch123/nml that referenced this issue Jun 5, 2021
…hen there was no unit.

The 'profit' callback in NML is supposed to be used like this:
  cargo_income = callback_result * cargo_amount * price_factor
To do something similar as the default-payment, you can use
  callback_result = distance * time_penalty
with 'time_penalty' in range 0..1.

The old code got confused about 'price_factor' because NML applies a unit conversion in the property, and applied this unit conversion a second time.
frosch123 added a commit to frosch123/nml that referenced this issue Jun 5, 2021
…hen there was no unit.

The 'profit' callback in NML was supposed to be used like this:
  cargo_income = callback_result * cargo_amount * price_factor
But since NML defines 'price_factor' as income per 10 units over 20 tiles without time-penalty (=255/256), this definition results in a very weird meaning of 'callback_result'.
NML applied some unit conversion to the callback result, which was supposed to revert this 20*10*255/256 scaling, but this only made it harder to use.

By removing the unit conversion in the callback, the 'callback_result' can now be defined as relative scaling of the 'price_factor'.
To do something similar as the default-payment, you can now use
  callback_result = distance * time_penalty
with 'time_penalty' in range 0..1.
@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Aug 16, 2021

Returning negative values as positive sometimes? Is this 15 bit callback result dropping the signed bit?

Loading

@glx22
Copy link
Contributor

@glx22 glx22 commented Aug 16, 2021

Not here @andythenorth, this callback is a proper 15bit one, with sign bit in bit 14, but nml applies some weird unit conversion.

Loading

@sevenfm
Copy link

@sevenfm sevenfm commented Sep 18, 2021

This was reported before on forum, cargo calback in NML doesn't match in result with the game's code, increasing profit by 10-30%, depending on distance:
https://www.tt-forums.net/viewtopic.php?t=83132

Loading

@glx22 glx22 closed this in 870a6d9 Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants