-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add flea market options #64
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ripchord.
First of all, thank you for taking the time to contribute to the PPT project, I really appreciate that :-)
There is few issues in this PR:
1. This is not multiplayer compatible (Fika)
because of the mutation of the database, the change will be global for all players, so some sort of "desync" can occurs (where for example you have access to the flea when you shouldn't)
I'll need to take a deeper look into the spt-server code to find out which class method I can patch to do the actual thing.
2. The config is not dynamic
The config is directly passed to the flea controller, so this can break integration with future features or external mods that rely on ptt api.
To have a dynamic processed config, you have to be sure to get it using the getConfig
method on the PathToTarkovController when you need it (you'll need the sessionId
of the concerned user to get the correct config)
Note: The sessionId
is the same id as your profile.json file name.
3. You have to rebase your work on the next
branch
I made a lot of changes for PTT v6 and new code changes should be based on the current development state. Let me know if you need some help here.
If you need to contact me, we can continue talking here or you can reach me on the fika discord where there is dedicated thread for Path To Tarkov.
Don't be discouraged by my review, we can try to solve those issues together ;-) (It's also a good learning opportunity for you if you are new to open-source contribs)
@@ -244,6 +250,12 @@ export class PathToTarkovController { | |||
sessionId, | |||
); | |||
|
|||
if (config.flea && config.flea.access_via) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typescript won't compile here because flea
is missing in the config type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be sure those commands work on your machine:
npm install
npm run build
const fleaConfig = this.db.getTables().globals.config.RagFair; | ||
fleaConfig.minUserLevel = config.flea.min_level; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db mutated here, see my review comment.
const fleaConfig = this.db.getTables().globals.config.RagFair; | ||
fleaConfig.enabled = checkAccessVia(access_via, offraidPosition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db mutated here, see my review comment.
@@ -185,6 +185,8 @@ class PathToTarkov implements IPreSptLoadMod, IPostSptLoadMod { | |||
|
|||
this.pathToTarkovController.tradersController.initTraders(this.config); | |||
|
|||
this.pathToTarkovController.fleaController.initFlea(this.config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the config is directly passed here, see my review comment. I can guide you through a correct implementation here.
Thanks heaps for all the feedback, as mentioned this is my first ever foray into collaborating on Github and my first experience with TypeScript (!) In fact I don’t work n the field at all so coding is “a hobby,” an entry level one at that.
I have spent a few thousand hours modding Space Engineers but that’s all C#, runtime-compile but very different from SPT.
I will go through all your comments and learn as much as I can. Once again, very much appreciate you taking the time.
Have a most wonderful Christmas!
Cheers,
Jon
From: Guillaume ARM ***@***.***>
Sent: Wednesday, 25 December 2024 2:48 AM
To: guillaumearm/PathToTarkov ***@***.***>
Cc: ripchord ***@***.***>; Mention ***@***.***>
Subject: Re: [guillaumearm/PathToTarkov] Add flea market options (PR #64)
@guillaumearm commented on this pull request.
_____
In src/path-to-tarkov-controller.ts <#64 (comment)> :
@@ -244,6 +250,12 @@ export class PathToTarkovController {
sessionId,
);
+ if (config.flea && config.flea.access_via)
be sure those commands work on your machine:
npm install
npm run build
—
Reply to this email directly, view it on GitHub <#64 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF3BADFEDDGU35GADRWXYHD2HGF5HAVCNFSM6AAAAABUD2SJICVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRSGEYTSMJSGI> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AF3BADEYBOFCE7ED65OKR632HGF5HA5CNFSM6AAAAABUD2SJICWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUWKR55E.gif> Message ID: ***@***.*** ***@***.***> >
|
Adds flea market options in config file:
"flea": {
min_level: number, Minimum level required to access flea market
access_via: string[] Offraid positions, same format of trader.access_via
}
If any config options are not found, will leave as default (minimum level 15, available everywhere.)
This is my first ever attempt at a Git code contribution so sincere apologies if I've messed up any protocols!