Skip to content
This repository has been archived by the owner on Apr 26, 2021. It is now read-only.

checks if destiny is running. if not, clears the activity #49

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

checks if destiny is running. if not, clears the activity #49

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 5, 2019

looks like visual studio messed with stuff a bit so this isnt as easy to compare...

@mrphlip
Copy link

mrphlip commented Jul 5, 2019

This should be optional, as it would make the tool only usable by PC players - console players want the tool to run even though they don't have a Destiny2.exe process...

@ghost
Copy link
Author

ghost commented Jul 5, 2019

Yep. you can add a config for that.

Like, if a user puts in a battletag then ask if they want to use this instead?

@ghost
Copy link
Author

ghost commented Jul 5, 2019

I moved some of the code around so if you have a flag for this in your configs, you can switch between the two.

@ghost
Copy link
Author

ghost commented Jul 5, 2019

This project might have code to get it working for PS4 players: https://github.com/Frankity/Ps4Rpc

There is probably something similar for xbox. If it requires the user to authenticate to get the info the ROI probably isnt high enough though.

@louis-bompart
Copy link
Collaborator

louis-bompart commented Jul 5, 2019

As much as I'd like to support Playstation and Xbox exactly the same, the fact they're running the game on a different platform is ofc a big limiter there.

So, what'll be done for now is this for PC. To do so, we can use the membershipInformation that we get in the config. (@Akumati1435 please consider this in the upcoming work you'll do on this)

What'll be done for other platforms will be investigated later (@mrphlip please create an issue to follow up on that).

I'm doing a deep dive review on your code now @Akumati1435 (fair warning, I'm quite a demanding reviewer, I preach for quality over feature)

@louis-bompart louis-bompart self-requested a review July 5, 2019 05:48
@ghost
Copy link
Author

ghost commented Jul 5, 2019

like i said, i am not good with node.js. you really should just get an experienced node.js dev to turn what I wrote into a production quality version. I've written 10s of thousands of lines of c# code to automated my discord community but the bots I made with node.js are extremely basic.

@louis-bompart
Copy link
Collaborator

👍 Alright, we'll work from what you've done here then, tho, your contribution is much appreciated, I just try to keep the code as clean as it is right now :)

@louis-bompart louis-bompart added the help wanted Extra attention is needed label Jul 5, 2019
@louis-bompart louis-bompart requested a review from pjuhel July 5, 2019 06:47
@louis-bompart
Copy link
Collaborator

Alright! I took me a full day of thinking (in the background), but I think I have a good proposition.

When you'll start up the app, during the setting up phase, a new question will be asked to the user. It'll be something like:

Would you like us to monitor if Destiny 2 is running on this computer to start the service?

If answered yes, we will do something based on this Pull Request, we will start polling Bungie API when we detect Destiny 2 being launched.
However, we will remove most part of the logic for an NPM package https://www.npmjs.com/package/ps-list, to have less code to maintain.

If answered no, and that's what shall interest you @mrphlip we will continuously poll the API, but with a delay that'll increase exponentially.

It'll start at 15s, the basic value, then double to 30s, then 1m, 2m, 5m, 15m, 30m, 1h. When we reach the two hours, what I'd name a final countdown/ Doomsday clock will start. We will keep polling every hour. But, 24hrs later after we reached the 1hr delay, the program will ~~ commit seppuku ~~ silently shut down.

What do you think about that?

@ghost
Copy link
Author

ghost commented Jul 7, 2019

I originally made this with ps-list, but after seeing how abysmal the performance was i switched off it. If you benchmark that vs the method I copy/pasted off stackoverflow, you'll notice a massive difference. I've heard ps-list takes up a lot of ram too for unknown reasons.

If you have the program poll once an hour towards the end of it for console peasants, won't that mean up to an hour of the script not showing the discord integration? It seems like you can safely clear the game activity after some time, but you should still be monitoring the API for new activity at the same interval.

Also, not sure if you have error handling for when bungie throws up, but it throws up a lot. For my stuff I keep a list of error codes to retry for in my configs. It has a min retry time, a max retry time, and a number of retries to attempt. It scales exponentially until it hits the max time, then continues to use the max time until the max number of retries is hit.

@louis-bompart
Copy link
Collaborator

louis-bompart commented Jul 7, 2019 via email

@louis-bompart
Copy link
Collaborator

louis-bompart commented Jul 7, 2019 via email

@ghost
Copy link
Author

ghost commented Jul 7, 2019

The API can handle it. Just know the error codes that tell you to back off and you're fine.

To put that into perspective...
I've got 10 threads pulling character data for my clan and another 8 for activity data. These jobs sometimes even overlap and I don't have issues. The website I run uses the same VM and that hits the API too. Because I have so many clans, the only way to get some of the webpages to load in a reasonable amount of time is to thread the crap out of them.

I'm slamming the API and I don't have problems. 1 call every 15 seconds is nothing.

@louis-bompart
Copy link
Collaborator

louis-bompart commented Jul 7, 2019 via email

@ghost
Copy link
Author

ghost commented Jul 9, 2019

Every minute seems good. I dont think anyone would complain about it not showing their status correctly for a minute. Heck it might even help because until the user gets to orbit the information is wrong.

If you switch to running the script as a service, the auto shut off might interfere with things.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants