Isolate PreferredJobModel

[Be1zebub]

Added column server to darkp_playermodels.
Now changing the preferred model on server A will not affect the others.

also check job exists & model column not outdated - to display actual info in ui & network less data.

[FPtje]

Thanks for the PR! I think it's a good idea to store preferred models per server. One tricky thing with this code is that the sqlite table needs to be compatible with both the old code and the new code. This is because different servers will have different versions of the code.

I think that is covered with your current way of making the table and then altering it. Thanks! There are some requests, but overall I think this is quite a nice PR!

[FPtje]

When testing this, I found out that the IF NOT EXISTS construct does not exist in SQLite. You have to PRAGMA table_info(...) to find out whether the column exists.

[FPtje]

There's another issue. After the table is migrated to have three columns, the old version of this query will fail with this error: table darkp_playermodels has 3 columns but 2 values were supplied.

Because SQLite errors are silent (the query just returns false, and you have to print sql.LastError() to get the error, job models are silently no longer saved.

This would not have a problem if the query was REPLACE INTO darkp_playermodels(jobcmd, model) VALUES(%s, %s);, but this is not something we can fix in hindsight 🤔

[FPtje]

It looks like it's going to be very challenging to make this code backwards compatible: when players join servers with the old version, saving preferred job models would sadly break.

Since the old table has a spelling error (darkp instead of darkrp), maybe we can get away with creating a new table darkrp_playermodels which has column SERVER TEXT NOT NULL, and then also get data from darkp_playermodels if it exists. What do you think?

[FPtje]

One thing I realized as well: with jobcmd still being the primary key, two different servers with the same jobcmd would not be able to have two rows in the table.

[FPtje]

Alright, I made a last change to the PR, to make an entirely new table for the jobs. It will still use models from the old table, but those will be overwritten by the new table.

I also ran some manual tests to make sure it works right. This code is remarkably tricky to get working perfectly. Those invisible SQLite errors are a real stinger.

Let me know what you think. If this is alright with you I'll merge it 👍

[Be1zebub]

I thought about transferring the old data to a new table, but this method is also good.
lgtm, but i think something like this would better:

-- pseudo code
if oldTable.exists() then
    for _, row in ipairs(oldTable.select()) do
        newTable.insert(row)
    end
    
    oldTable.delete()
end

[FPtje]

That's an interesting idea, though I worry that it would break preferred models if players go back to a server running the old code. If the table gets deleted, servers running an older DarkRP will no longer be able to read it.

DarkRP has always had the issue of servers not caring to update, which means for some things, backwards compatibility needs to be kept indefinitely. By only reading from the table, nothing breaks when hopping between servers with the latest version and the old one.

[Be1zebub]

makes sence.
is there a reason to store db on clientside and send it to the server, and not vice versa?

this line looks like hack

DarkRP/gamemode/modules/base/sv_jobmodels.lua

Line 18 in 8762674

gamemode.Call("PlayerSetModel", ply)

[FPtje]

That's a really good question. I'm not quite sure 😅
Maybe my idea back then was that most servers would at least have the basic jobs in common, and that sharing the model setting across servers would then make sense. Nowadays it doesn't anymore though.