Conversation
guillaumearm
left a comment
There was a problem hiding this comment.
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)
| sessionId, | ||
| ); | ||
|
|
||
| if (config.flea && config.flea.access_via) |
There was a problem hiding this comment.
typescript won't compile here because flea is missing in the config type
There was a problem hiding this comment.
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.
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.
db mutated here, see my review comment.
|
|
||
| this.pathToTarkovController.tradersController.initTraders(this.config); | ||
|
|
||
| this.pathToTarkovController.fleaController.initFlea(this.config); |
There was a problem hiding this comment.
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: ***@***.*** ***@***.***> >
|
|
Hey @ripchord , I hope you're fine. I just want to let you know that I will work on this feature very soon. |
|
Oh amazing 😊
I’ve been distracted with the festive season and then customising (and hosting!) an Empyrion server for a mate… a lot less stressful than Tarkov haha but I am sure I’ll be bored again soon.
Also saw all the 3.10.3 changes, updates to Fika, etc, so thought I would keep out of the way for the time being. I am delighted that you are picking it up because I am only a hobby modder and have never used TS before, nor done any netcode stuff 😊 It’s great that you have time to implement because now it will be done properly!
Cheers.
Jon
From: Guillaume ARM ***@***.***>
Sent: Tuesday, 14 January 2025 9:25 AM
To: guillaumearm/PathToTarkov ***@***.***>
Cc: ripchord ***@***.***>; Mention ***@***.***>
Subject: Re: [guillaumearm/PathToTarkov] Add flea market options (PR #64)
Hey @ripchord <https://github.com/ripchord> , I hope you're fine.
I just want to let you know that I will work on this feature very soon.
—
Reply to this email directly, view it on GitHub <#64 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF3BADAQK5T2AQFDHMT5KRD2KRDNBAVCNFSM6AAAAABUD2SJICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOBYGQ2DGNRVHA> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AF3BADFH4VULGL65DEQALST2KRDNBA5CNFSM6AAAAABUD2SJICWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTU2JCCAU.gif> Message ID: < ***@***.***> ***@***.***>
|
|
Hi legend,
Hope you’re well!
I’m looking forward to the change to C# for SPT server-side modding because I’m much more familiar with that, than TS. I’ll probably jump back in at that point and see if I can actually be helpful to the community 😊
Loving the latest PTT with transits; have a rather brutal config where all inter-map travel is transit only, traders and (very small) stash accessed only through an exfil central to each map which has regen disabled. Eventually I want to use with Jehree’s “pop-up hideout” mods, so I’m hoping integration with the radio exfil bit of Home Comforts isn’t too hard (i.e. exfils to reset map, or to log out for the day.)
Keep up the great work, hope I can contribute at some point!
Cheers,
Jon.
From: Guillaume ARM ***@***.***>
Sent: Wednesday, 25 December 2024 2:47 AM
To: guillaumearm/PathToTarkov ***@***.***>
Cc: ripchord ***@***.***>; Mention ***@***.***>
Subject: Re: [guillaumearm/PathToTarkov] Add flea market options (PR #64)
@guillaumearm requested changes on this pull request.
Hello @ripchord <https://github.com/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 <https://github.com/guillaumearm/PathToTarkov/tree/next> 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)
_____
In src/path-to-tarkov-controller.ts <#64 (comment)> :
@@ -244,6 +250,12 @@ export class PathToTarkovController {
sessionId,
);
+ if (config.flea && config.flea.access_via)
typescript won't compile here because flea is missing in the config type
—
Reply to this email directly, view it on GitHub <#64 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF3BADAYMXSYVG55OFVHRHD2HGFXJAVCNFSM6AAAAABUD2SJICVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRSGEYDQMBVGM> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AF3BADC6H3MIC3Q7WKUIX5L2HGFXJA5CNFSM6AAAAABUD2SJICWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUWKRIJK.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!