Added 3 New LBP3 Categories - #1109
Conversation
|
I wrote a fairly simple recommendation system for the server which is VERY similar to how Sony used to handle it. I left my comments in there so you can easily read what everything does and why its there. If anything else is needed to change in this PR please let me know! I just figured I'd stack it on top of this pr as i built this on top of my already existing work. |
|
Is there a reason you're using |
|
Also, I forgot to thank you on here for the recommendation, as using /play was a far better way of tracking when a user actually enters a community level. I appreciate it! |
|
Also, this is my last commit to the PR unless an issue has been found.. which I'm sure there will 100% be issues, just feel free to let me know, and ask any questions if needed! |
Increased the limit of recently played slots from 20 to 30.
FeTetra
left a comment
There was a problem hiding this comment.
There is a lot more to review, but I'm going to leave this for now.
…hController.cs Co-authored-by: FeTetra <166051662+FeTetra@users.noreply.github.com>
…hController.cs Co-authored-by: FeTetra <166051662+FeTetra@users.noreply.github.com>
…velsCategory.cs Co-authored-by: FeTetra <166051662+FeTetra@users.noreply.github.com>
…velsCategory.cs Co-authored-by: FeTetra <166051662+FeTetra@users.noreply.github.com>
Slendy
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
Here are a few more things that should be addressed that aren't covered in the requested changes:
- Comments should have a space between the two backslashes and the first character of the comment (
//comment goes hereshould be// comment goes here) - Indentation of code and comments in certain places is off
This could honestly be split into separate PRs since from what I can tell the recently played and recommended categories have nothing to do with each other, but it's fine given how dead development is now.
| { | ||
| RecentlyPlayedEntity? recentlyPlayed = await this.RecentlyPlayed.FirstOrDefaultAsync(r => r.UserId == userId); | ||
|
|
||
| long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); |
There was a problem hiding this comment.
| long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | |
| long now = TimeHelper.TimestampMillis; |
| @@ -0,0 +1,1602 @@ | |||
| // <auto-generated /> | |||
There was a problem hiding this comment.
This entire file doesn't need to be included and can be deleted
| public bool ShouldSerializeGameFilter() => !string.IsNullOrWhiteSpace(this.GameFilter); | ||
|
|
||
| public bool ShouldSerializeDateFilterType() => !string.IsNullOrWhiteSpace(this.DateFilterType); | ||
|
|
||
| public bool ShouldSerializeIncludePlayed() => this.IncludePlayed.HasValue; | ||
|
|
||
| public bool ShouldSerializeTeamPicked() => !string.IsNullOrWhiteSpace(this.TeamPicked); | ||
|
|
||
| public bool ShouldSerializeBlacklisted() => !string.IsNullOrWhiteSpace(this.Blacklisted); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Instead of a bunch of ShouldSerialize you should use
[DefaultValue("")]on any nullable fields that you don't want to show up if they are empty. If the field is an object then you can use null instead of the empty double quotes
example:
[DefaultValue("")]
[XmlElement("gameFilter")]
public string? GameFilter { get; set; }There's a couple places that this should be fixed but I'm not going to comment on all of them for brevity.
| public bool ShouldSerializeGameFilter() => !string.IsNullOrWhiteSpace(this.GameFilter); | |
| public bool ShouldSerializeDateFilterType() => !string.IsNullOrWhiteSpace(this.DateFilterType); | |
| public bool ShouldSerializeIncludePlayed() => this.IncludePlayed.HasValue; | |
| public bool ShouldSerializeTeamPicked() => !string.IsNullOrWhiteSpace(this.TeamPicked); | |
| public bool ShouldSerializeBlacklisted() => !string.IsNullOrWhiteSpace(this.Blacklisted); | |
| } |
| return this.BadRequest(); | ||
| } | ||
|
|
||
| if (token.GameVersion == GameVersion.LittleBigPlanet3 && slotType == "user") |
There was a problem hiding this comment.
The slotType check isn't necessary because story levels are excluded via the check on line 36
| if (token.GameVersion == GameVersion.LittleBigPlanet3 && slotType == "user") | |
| if (token.GameVersion == GameVersion.LittleBigPlanet3) |
| } | ||
|
|
||
| public IQueryable<ScoredSlot> GetScoredItems(DatabaseContext database, GameTokenEntity token, SlotQueryBuilder queryBuilder) | ||
| { |
There was a problem hiding this comment.
I think this should be refactored into 2 separate functions for actually calculating the user's search scores, and then fetching the levels based on the scores should take place here.
|
|
||
| long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | ||
|
|
||
| //Level at the top of the recently played category for the user |
There was a problem hiding this comment.
This comment doesn't really make sense, is it supposed to go somewhere else?
| { | ||
| Slot = s, | ||
| ThumbsUp = database.RatedLevels.Count(r => r.SlotId == s.SlotId && r.Rating == 1), | ||
| }) |
There was a problem hiding this comment.
| { | |
| Slot = s, | |
| ThumbsUp = database.RatedLevels.Count(r => r.SlotId == s.SlotId && r.Rating == 1), | |
| }) | |
| { | |
| Slot = s, | |
| ThumbsUp = database.RatedLevels.Count(r => r.SlotId == s.SlotId && r.Rating == 1), | |
| }) |
Fix indentation here
| public override string[] Sorts { get; } = | ||
| { | ||
| "relevance", | ||
| }; |
There was a problem hiding this comment.
| public override string[] Sorts { get; } = | |
| { | |
| "relevance", | |
| }; | |
| public override string[] Sorts { get; } = ["relevance",]; |
This change should be implemented in all Category classes where possible
| { | ||
| Slot = s, | ||
| Hearts = database.HeartedLevels.Count(r => r.SlotId == s.SlotId), | ||
| }) |
There was a problem hiding this comment.
| { | |
| Slot = s, | |
| Hearts = database.HeartedLevels.Count(r => r.SlotId == s.SlotId), | |
| }) | |
| { | |
| Slot = s, | |
| Hearts = database.HeartedLevels.Count(r => r.SlotId == s.SlotId), | |
| }) |
Fix indentation
| return database.Slots.Where(_ => false); | ||
|
|
||
| List<int> slotIds = recentlyPlayed.SlotIds | ||
| .Take(30) |
There was a problem hiding this comment.
This should also use the config value instead of hardcoding it
sudokoko
left a comment
There was a problem hiding this comment.
A couple things I noticed, on top of Slendy's review.
| SlotQueryBuilder queryBuilder) | ||
| { | ||
| RecentlyPlayedEntity? recentlyPlayed = database.RecentlyPlayed | ||
| .AsNoTracking() |
There was a problem hiding this comment.
You're using .AsNoTracking() in some places and not others, and additionally - I don't believe we've really ever marked database calls as explicitly untracked.
| .AsNoTracking() |
| { | ||
| public int MaxNeighbors { get; set; } = 250; | ||
| public int MinimumNeighborOverlap { get; set; } = 2; | ||
| public int MaxCandidatePool { get; set; } = 1000; |
There was a problem hiding this comment.
| public int MaxCandidatePool { get; set; } = 1000; |
From what I can tell, this isn't used anywhere. I don't know if you intended for it to be.
| IQueryable<ScoredSlot> recommendations = | ||
| from slot in database.Slots | ||
| .AsNoTracking() | ||
| .Where(queryBuilder.Build()) | ||
| .Where(slot => !database.VisitedLevels.Any(visitedLevel => | ||
| visitedLevel.UserId == token.UserId && | ||
| visitedLevel.SlotId == slot.SlotId)) | ||
|
|
||
| join score in scores | ||
| on slot.SlotId equals score.SlotId | ||
|
|
||
| let hearts = database.HeartedLevels.Count(heartedLevel => | ||
| heartedLevel.SlotId == slot.SlotId) | ||
|
|
||
| let likes = database.RatedLevels.Count(rating => | ||
| rating.SlotId == slot.SlotId && | ||
| rating.Rating == 1) |
There was a problem hiding this comment.
This query seems to do a lot of unbounded work and performs multiple COUNT operations per slot. Could have performance issues at scale.
Perhaps this is where you intended to use CategoryConfiguration.Instance.Recommended.MaxCandidatePool to limit the candidate set before performing expensive aggregates?
Could also pre-aggregate the likes and hearts in separate grouped queries instead of calling COUNT inside of the projection.
.Take(config.MaxCandidatePool)
| #nullable disable | ||
|
|
||
| this.PlayerCount = RoomHelper.Rooms.Count(r => r.Slot.SlotType == SlotType.User && r.Slot.SlotId == this.SlotId); | ||
| Dictionary<int, int> playerCounts = RoomHelper.GetUserLevelPlayerCounts(); |
There was a problem hiding this comment.
If PrepareSerialization is invoked for multiple slots in parallel, this could result in a performance issue with how this method is implemented. See comment in GameUserSlot.cs.
|
|
||
| public static Dictionary<int, int> GetUserLevelPlayerCounts() | ||
| { | ||
| lock (RoomLock) |
There was a problem hiding this comment.
Continued from the comment on GameUserSlot.cs - method acquires a lock and builds a full dictionary each time. Consider caching the counts for the duration of the request or exposing an API where the caller computes the counts once and reuses them for multiple calls.
I added support for LBP3's recently played category.
Essentially, LBP2 tracks which levels you've played client side, and just reports it back whenever you click on "Recently Played". LBP3 on the original servers would handle all of this server sided. This essentially works by recording the recently played community levels via the games "gameState" request, then it stores the recently played slot ID's and the timestamps per each user. I set a limit so that it keeps a maximum of 20 recently played levels, when you replay a level that was already on the list, it changes the timestamp to have it re-appear at the top of the list. The game also only applies to users who are playing on LBP3. If you play LBP1 or LBP2, it will not change this list whatsoever.