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

Request: Persistent storage for vehicles #6908

Closed
George-VB opened this issue Sep 16, 2018 · 13 comments
Closed

Request: Persistent storage for vehicles #6908

George-VB opened this issue Sep 16, 2018 · 13 comments

Comments

@George-VB
Copy link

@George-VB George-VB commented Sep 16, 2018

https://newgrf-specs.tt-wiki.net/wiki/Storages reports, that only Industries support persistent storage yet

It would be nice to have them for vehicles too. Currently xUSSR set makes a huge calculation to choose the right graphics for every engine. These calculations could be done once in can_attach_wagon or start_stop CBs, and them the final value would be easily used when graphics is chosen.

Please provide persistent storage for vehicles

@George-VB

This comment has been minimized.

Copy link
Author

@George-VB George-VB commented Sep 16, 2018

@frosch123

This comment has been minimized.

Copy link
Member

@frosch123 frosch123 commented Sep 20, 2018

If someone wants to implement this, some notes:

  • Currently NewGRF vehicles use a functional programming approach, where multiple properties are evaluated via independent callbacks. In most cases it does not matter in which order and when the callback are called, which allows caching.
  • Persistent storage are an imperative programming approach. Callbacks that can write persistent storage are prone to break stuff, if they are called in different order or cases, or with different caching. So they dead-lock OpenTTD into never changing any behaviour with them.

To mitigate this, write access to persistent storage should be restricted to a single callback, probably a new one. The execution order and times need to be clearly defined, especially in relation to train user-data (callback 36 property 25, var 42).
All other callbacks should likely be called after these two.

It also needs to be specified when persistent storage is copied or reset:

  • Vehicle cloning
  • Vehicle autoreplace
  • Train rearrangement
  • ...

Though when reading the original requirement in the top post, the suggested storage is not as pesistent as it is for industries. It rather reads like resetting storage when rearranging/refitting/servicing vehicles. So vehicles could not use this "persistent" storage to track run-distance over their lifetime etc.

@Eddi-z

This comment has been minimized.

Copy link
Contributor

@Eddi-z Eddi-z commented Jan 5, 2019

IMHO, this should work similar to the random bits, i.e.

  • there are "triggers" (bitmask, property) when the recalculation callback should be run (e.g. on empty vehicle, on servicing, on railtype change, regular-interval ...)
  • could use the same way to store as the random bits, or even share the space
  • could be run at the same time as the random trigger callback
@andythenorth

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Jan 5, 2019

Assuming that this actually is a performance issue (do we know that?), then I am +1 to Eddi's suggestion.

If there's no evidence that this is a performance issue, then I'm going to close it within 2019.

@George-VB

This comment has been minimized.

Copy link
Author

@George-VB George-VB commented Jan 20, 2019

This is a performance issue.

@Eddi-z

This comment has been minimized.

Copy link
Contributor

@Eddi-z Eddi-z commented Jan 20, 2019

point i forgot in the above list:

  • we also got the userbits, so this could be an alternate method to access those
@andythenorth

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Jan 21, 2019

This is a performance issue.

Is there are any profiling data or more detail for this? Helps diagnose the underlying issue. Similar to #4934 or https://www.tt-forums.net/viewtopic.php?f=29&t=58021

@nielsmh

This comment has been minimized.

Copy link
Contributor

@nielsmh nielsmh commented Jan 21, 2019

I don't know how feasible it would be to implement like this, but if the the cache could be implemented as a call of another callback, which takes a list of dependent input values as argument list, and this callback is only allowed to use the the arguments passed. The client is then able to cache the result of that callback reliably.

That would perhaps require a NewGRF version bump?

@Eddi-z

This comment has been minimized.

Copy link
Contributor

@Eddi-z Eddi-z commented Jan 21, 2019

My current idea about this looks like this:

  • Add a callback to set the value of prop 25 (userbits, currently 8 bits, could be extended to 15?)
  • Add a property to set the flags when that callback should be called (same meaning as random trigger flags)
  • Add additional variables to complement var 42 (OR over all vehicles in the consist), e.g.
    • AND over all vehicles
    • this vehicle only

none of this should require a NewGRF version bump

@George-VB

This comment has been minimized.

Copy link
Author

@George-VB George-VB commented Feb 10, 2019

  • we also got the userbits, so this could be an alternate method to access those
    then we need some way to specify, when we can change them
@PeterN

This comment has been minimized.

Copy link
Member

@PeterN PeterN commented Feb 10, 2019

Persistent storage should be changed only at the point where a consist is altered? And unlike other variables, it should be saved in the savegame, I think.

Property 25 is utterly static currently. We already have persistent storage so it makes sense to use that.

@George-VB

This comment has been minimized.

Copy link
Author

@George-VB George-VB commented Feb 10, 2019

Currently it would be enough if it is recalculated in depot when attaching a vehicle to consist (value should be recalculated for every vehicle in consist after a new vehicle is attached)

Currently I can't check the value in var 25 (userbits) in other callbacks, like vehicle graphics, sound_effects, ect.

@stale

This comment has been minimized.

Copy link

@stale stale bot commented Apr 11, 2019

This issue has been automatically marked as stale because it has not had any activity in the last two months.
If you believe the issue is still relevant, please test on the latest nightly and report back.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Apr 11, 2019
@stale stale bot closed this Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.