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

Add flea market options #64

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ripchord
Copy link

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!

Copy link
Owner

@guillaumearm guillaumearm left a 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)
Copy link
Owner

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

Copy link
Owner

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

Comment on lines +18 to +19
const fleaConfig = this.db.getTables().globals.config.RagFair;
fleaConfig.minUserLevel = config.flea.min_level;
Copy link
Owner

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.

Comment on lines +29 to +30
const fleaConfig = this.db.getTables().globals.config.RagFair;
fleaConfig.enabled = checkAccessVia(access_via, offraidPosition);
Copy link
Owner

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);
Copy link
Owner

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.

@ripchord
Copy link
Author

ripchord commented Dec 25, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants