matthijs wrote:

This should probably be written for my new memory pools (# 13) right away, adding dependency.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment7

Bjarni wrote:

assigning this one to blathijs as the only thing left is to convert it to the new pool system
it appears that nobody had any complains about anything else

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment24

Bjarni wrote:

ok, the pool system will not be ready for 0.4.5, so we should go ahead and commit this one

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment28

matthijs wrote:

Looks good. Only thing I don't understand is
- { 'PLYR', Save_PLYR, Load_PLYR, CH_ARRAY | CH_LAST},
+ { 'PLYR', Save_PLYR, Load_PLYR, CH_ARRAY },
What does CH_LAST do and why do you remove it? If this really is needed/what you want, you should go ahead and commit it.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment48

Darkvater wrote:

CH_LAST tells the saveload system that that block is the last one. Removing it will introduce serious trouble if you do not add it at the last position after it.
See saveload.c:760

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment54

Darkvater wrote:

I think the enginerenew functions should go to engine.[ch] from players.c, at least the implementation.
Load_ERNW could be: SlObject(GetEngineRenew(index), _engine_renew_desc);
Some spaces: RemoveAllEngineReplacement() for loop
Usage of InitializeEngineReplacement() is non-consistent, eg should be used in RemoveAllEngineReplacement() for example
Initialize is with a 'z' not an 's', in emoveAllEngineReplacement settings|for seperate words.

That's my comments/suggestions from reading through the diff, donnu if it actually works :P

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment55

peter1138 wrote:

Putting the replacement functions in engine.c would means engine.h needs to know about the player struct, which is a bummer...

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment56

matthijs wrote:

How about changing the interface of functions like GetEngineReplacement to accept a memory pool instead of a Player*, since the einge_replacement field is the only one used from the Player struct. Optionally, you could build functions like GetEngineReplacementForPlayer() to wrap those function but take a Player* argument instead. This keeps all the code where it should belong, IMO.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment57

peter1138 wrote:

Because there is only one pool, shared between users.

And also, currently there is no pool, which stops me moving stuff around before changing it to a pool...

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment58

matthijs wrote:

Ah, right. Didn't read well enough. Anway, the point still holds.

Why not pass p->engine_replacement around instead of p? (ie, an EngineRenew* instead of a Player*).

And the moving around is meant as an extra to this patch. It might be better to commit this one first, and then move stuff?

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment59

Darkvater wrote:

Exactly, what does an engine (replacement) array have to know about players? Just pass the ID and be done with it.

On a slightly different note.

The old engine_replace array is removed from the savegame but it is still loaded. I thought the point was that it didn't need be supported, so just removing it completely. Yes, that also means that you will not be able to load nightlies. Same goes for the v->station_visited_length or something. If you remove it nicely you can't load those. Ok, it's a bitch, but that's what nightlies are for.

Or what we can do is make a nice nightly, and the next day it is totally removed. Thus you can load your old nigthlies with the last nightly and save them there. After that you can load them in the next version just fine. Comments?

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment60

matthijs wrote:

I've moved functions from players.c to engine.c, building the neccesary wrappers in players.c.

While doing this, I've introduced a new type, EngineRenewList, which stores the first element of the per-player EngineRenew linked list. I was hoping that by doing this we could keep the EngineRenew struct completely private to engine.c, which nearly worked. The only problem was that the saveload code needs to know about the struct to convert int to reference and v.v.

I've also removed the InitialiseEngineReplacement() function, since it was only called in two places that could just execute the one line the function used to contain directly. Introduced a InitializeEngines() function to init the enginerenew pool.

Regarding the savegame thing, the current code doesn't load the renew array from older savegame, but just throws it away (hence the nullstruct). This means all older games don't have their renewarrays loaded, but the games themselves will still load. We can always later on just remove support for some savegame revisions (might be a nice plan anyway, clean out support for all revisions that were not released?) For now, this looks like a good solution IMHO.

Were there other patches that required a savegame bump, so we can commit them at the same time?

Attachments

• replacepool5.diff (26.04 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment66

peter1138 wrote:

Whoops, hadn't noticed you'd done this :-)

I don't see any point in converting from the current engine replacements to the pool system, as it's quite a bit of effort for something that isn't really necessary. I will give this patch a test in the morning.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment75

peter1138 wrote:

Just updated it to r3393: http://195.112.37.102/ottd/replacepool6.diff

This comment was imported from FlySpray: https://bugs.openttd.org/task/3#comment76

peter1138 closed the ticket.

Reason for closing: Implemented

This comment was imported from FlySpray: https://bugs.openttd.org/task/3