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

Remove leftover Discord implementation #2499

Merged
merged 9 commits into from
Apr 7, 2022

Conversation

Lpsd
Copy link
Member

@Lpsd Lpsd commented Jan 5, 2022

Fixes #1370

Removes the leftover Discord implementation - aside from CServerInfo / CQueryReceiver stuff introduced in the original PR, which is actually a benefit.

@AlexTMjugador
Copy link
Member

#1570 was opened to bring back the Discord integration, but I take @0x416c69 doesn't seem that active these days?

By the looks of it, I think that using the deprecated library instead of the latest, shiniest SDK can be a viable option. Discord would make even more developers unhappy if they made the old library stop working, as it is presumably being used in some big proprietary games: discord/discord-rpc#306 (comment). Hopefully, those big proprietary games are big and proprietary enough to make a migration to the new SDK a no-go, so I'd say Discord is compelled to align with the interests of GPL projects to maintain the status quo.

@Lpsd
Copy link
Member Author

Lpsd commented Jan 5, 2022

Not sure if they're still around or have plans to work on it - but we shouldn't have unused code sitting around.

@Deihim007 mentioned implementing it themselves, differently than #1570 (but explicitly no promises 😅)

@AlexTMjugador
Copy link
Member

AlexTMjugador commented Jan 5, 2022

Yeah, I agree with removing this old code if nobody is interested in maintaining it properly (and maybe even maintaining a discord-rpc fork if the need arises). Technical debt can only increase by keeping it around for no reason. Still, I personally find the current situation with Discord rich presence integration libraries quite unsatisfactory.

@0x416c69
Copy link
Contributor

0x416c69 commented Jan 5, 2022

I take @0x416c69 doesn't seem that active these days?

I do not have an interest in doing the clean up for the git commits and at that time I got very busy to do so. My PR was complete and flawless and ready to be merged.

By the looks of it, I think that using the deprecated library instead of the latest, shiniest SDK can be a viable option.

Originally I've used the newer SDK and actually I had a complete working version of it (it's still in the PRs somewhere) but because of how unstable it was and it caused so many crashes (As reported by @Dutchman101 ) and also the license issue (MTASA license does not allow to make use of it AFAIK) I had to refactor and clean up the old SDK and implement the rich presence over it which was decided and done as planned. To this day so many people are still using the old library and as I stated earlier the repository I've made for it is gonna be maintained and if the protocol of the IPC is changed I'm gonna implement it and make it work again but discord will be unlikely to change the protocol as it is unnecessary and would break compatibility.

I still don't understand why has no developer taken interest in my PR despite so many people wanting this feature on the MTASA community. Anyways as I said my PR is complete and I no longer wish to contribute further, close it if you wish.

@Dutchman101
Copy link
Member

Dutchman101 commented Jan 5, 2022

@0x416c69

I still don't understand why has no developer taken interest in my PR despite so many people wanting this feature on the MTASA community. Anyways as I said my PR is complete and I no longer wish to contribute further, close it if you wish.

Last week we discussed this (PR #1570) internally, to figure out why it stalled and got no code review attention.. it was kinda sad, because of the hype surrounding Discord Rich integration within MTA (there was a lot of developer attention, which suddenly dropped when you had to redo it). However, as a result of the discussion, maybe it will get some eyes soon.. so let's hold up before closing it

If it turns out to be well-designed and not really introduce bugs or more pending work, who knows it can do without your activity, or that another dev can easily jump in for small patches to anything that may arise during code review or right after implementation. Although that being the author is obviously better, we'll see about it?

@Lpsd
Copy link
Member Author

Lpsd commented Jan 5, 2022

Firstly, thank you for your contribution on #1570

In all honesty there's just not that many people around here confident enough, or with the knowledge, to test and review your PR. For those that can, it then boils down to whether they are interested in it or not (luck of the draw). Unfortunately lots of other great PRs have collected dust this way, it isn't exclusive to #1570

Regardless, I think this cleanup is necessary in master before reviewing #1570 (will then need rebasing or whatever).

I'd be happy to look into your PR, however if you're not around to contribute further do we have permission to use it entirely (i.e fork and continue changes ourselves)?

@0x416c69
Copy link
Contributor

0x416c69 commented Jan 5, 2022

to figure out why it stalled and got no code review attention

I was in contact with @qaisjp on discord and he instructed me what to do as he told on my PR but then I got busy and I couldn't get back to this so it could be part of the reason why.

or that another dev can easily jump in for small patches to anything that may arise during code review or right after implementation

Hoping to see that one day.
I just no longer have an interest in doing it. I'm more interested in modding and expanding the game itself now and I will hopefully make some PRs on it someday.

For those that can, it then boils down to whether they are interested in it or not (luck of the draw). Unfortunately lots of other great PRs have collected dust this way,

Of course, this is the case for many non-profit open source projects, I understand. But so many people and devs were really interested at the time but I guess they lost interest just as I did.

if you're not around to contribute further do we have permission to use it entirely (i.e fork and continue changes ourselves)?

Sure, go ahead.

@Lpsd
Copy link
Member Author

Lpsd commented Jan 9, 2022

I've reverted the enum removals in latest commit (they would cause desync). This is ready to review.

@Lpsd
Copy link
Member Author

Lpsd commented Jan 16, 2022

I've made the bitstream changes - ready to review x3

Copy link
Contributor

@patrikjuvonen patrikjuvonen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested lightly, works. I scrolled through the changes.

@Deihim007 Deihim007 mentioned this pull request Mar 22, 2022
18 tasks
@Pirulax
Copy link
Contributor

Pirulax commented Mar 22, 2022

LGTM.
Do the requested changes from Botder's review, and once he gives the green light it can be merged IMHO.
Worst case scenario we'll pull it back from the nightly.

@Lpsd Lpsd mentioned this pull request Mar 29, 2022
@Lpsd
Copy link
Member Author

Lpsd commented Apr 2, 2022

Requested changes have been made - ready for final review

@Lpsd Lpsd merged commit 9708440 into multitheftauto:master Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Our Discord integration violates the GPL
8 participants