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

Feature/friends subsystem #32

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

Conversation

MikhailSorokin
Copy link
Contributor

@MikhailSorokin MikhailSorokin commented Jul 10, 2020

Pull Request Template

Checklist:

  • The code follows the style guidelines of this project and the Epic Coding Standard
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have checked my code and corrected any misspellings

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. Any change needs to be discussed before proceeding.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • [ X] New feature (non-breaking change which adds functionality)

A draft of what the friends subsystem will look like. Queries now work and I've tested the calls every 3 seconds, so its working fine. However, I want to subscribe the friends to presence and friend status changes, i.e. - when friend presence changes - update the list and when new friends are added, update the list, etc.

So, you can take a look and add comments as I finish up that part of the friends interface.

MikhailSorokin and others added 12 commits July 7, 2020 20:55
…ever see the names being printed. Tried to fix it by adding more attributes and updating the tuple, which should work but the actual display name is never being returned. Have to figure that out later.
…receiving the names of queried people now though.
…me displayed of the user. Turns out there was a problem in OnlineSubsystemEpicTypes.h that was using the product user id instead of the account user id.
…ole purpose of demonstrating the bug fix for this issue.
…vents working for when friends are added and when presence changes for friends (offline/online).
…y presence method. Currently not done though because we need to detect when the query and the presence info is done.
@Ruhrpottpatriot Ruhrpottpatriot mentioned this pull request Jul 10, 2020
7 tasks
@@ -5,6 +5,7 @@
#include "OnlineSubsystemUtils.h"
#include "OnlineSubsystemEpicTypes.h"
#include "Utilities.h"
#include "Private/Utilities.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Please check if you and your team have configured the build process correctly. You shouldn't need the Private part of the include path.

…ibing to updates gets presence called x amount of times, which may be a problem with subscribing to listen to each user's events.
… I had done the past couple of weeks. May re-open the bug query issue to refix some things.
@MikhailSorokin
Copy link
Contributor Author

Tested presence and friends and they both work now where you can receive updates of when friends go online/offline. I imagine this will also work with sessions, but not tested yet since the Session Interface still seems buggy. I will do some further cleanup before this is suitable for review.

@MikhailSorokin
Copy link
Contributor Author

Technically this fixes bugs in the user + presence interfaces too, but I don't know how you want me to address that, whether it is in this issue or in a different one.

@Ruhrpottpatriot
Copy link
Owner

If it's not too much work better open a new PR, makes for better changelogs (which I automatically generate with GitHub Changelog Generator). otherwise leave this here.

@MikhailSorokin
Copy link
Contributor Author

Ready for review again.

}
}
//TODO - I don't know if this is right to just access 0 but I can't get the num anywhere else - maybe we loop through all local players?
TSharedPtr<FUniqueNetId const> localUser = subsystem->GetIdentityInterface()->GetUniquePlayerId(0);
Copy link
Owner

Choose a reason for hiding this comment

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

The identity interface has the FPlatformUserId GetPlatformUserIdFromUniqueNetId(const FUniqueNetId&) const; method you can use. Any FPlatformUserId is just a TypeDef'd int32

Copy link
Contributor Author

@MikhailSorokin MikhailSorokin Aug 4, 2020

Choose a reason for hiding this comment

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

Ok - problem with this though. I don't have access to the local user FUniqueNetId

Copy link
Owner

@Ruhrpottpatriot Ruhrpottpatriot Aug 4, 2020

Choose a reason for hiding this comment

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

I just looked into the OSS code to see what they actually want with this method. It's bugged as it is missing a parameter, which tells which user id's to query presence for. So going by the comments in Epics code and the method header, User is the aforementioned local user and the missing UserIds parameter would be the users to query presence for.

Since we're missing a parameter we have to assume that a call to this method intends to query all available presence information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we don't have this. You want to be able to use this for any user that you want to get information about. So you have to pass in the remote user as the parameter. A hack fix I can do is just loop through all the local players - similar to what is in the GetPlatformUserId function, and just query loop through valid ones using GetUniquePlayerId.

};

/**Mmap of local user idx to friends */
TMap<int32, FEpicFriendsList> FriendsLists;
Copy link
Owner

Choose a reason for hiding this comment

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

Use TMultiMap instead.

Copy link
Contributor Author

@MikhailSorokin MikhailSorokin Aug 4, 2020

Choose a reason for hiding this comment

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

Why? SteamFriendsSubsystem does it like this and I don't see any problems with it. I only need one type - the TArray of shared refs - the struct is there for readability.

Copy link
Owner

@Ruhrpottpatriot Ruhrpottpatriot Aug 4, 2020

Choose a reason for hiding this comment

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

With TMultiMap You can write: TMultiMap<int32, TSharedRef<FOnlineFriendEpic>> FriendsLists; and then use methods like MultiFind to get the values for a key. It does the same thing you do, but without the need for an extra struct or nested TArray. It also should be slightly faster and is more space efficient and gives us the option (if we really need it) to add custom key and value functions for equality.

It's not totally necessary, but in my eyes it simply looks cleaner, as this is the intended use for a MultiMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, maybe I can add it later as an enhancement. Would like to see what other issues people experience or want added. For instance, one of the enhancements I was considering adding was a way to just pass in "Online" or "Offline" and then filter out all of those friends that have offline or online. This is something that could only be done in EAS since no other service provides this, but I didn't deem it necessary for now. I could definitely use a MultiMap for then

@MikhailSorokin MikhailSorokin force-pushed the Feature/FriendsSubsystem branch from e677c24 to 098c410 Compare August 4, 2020 04:03
@MikhailSorokin MikhailSorokin changed the title DRAFT - Feature/friends subsystem Feature/friends subsystem Aug 4, 2020
…ing properly. Here were the changes: (1) - made some updates to presence in order to cache information in the query presence call and not duplicate information and (2) - some changes in friend interface that will allow IsFriend and GetFriend to return properly as a hack fix since I don't understand shared references and why data would be garbage collected over time.
@MikhailSorokin
Copy link
Contributor Author

Updated again with latest comment explaining the changes. It now works better - without duplication of some information in presence.

@MikhailSorokin
Copy link
Contributor Author

I also tested by updating the presence on and off on accounts and display name + presence switching worked on an epic account I had with 6 friends.

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.

3 participants