feature/party persistence and offline leveling support#718
feature/party persistence and offline leveling support#718sven-n merged 45 commits intoMUnique:masterfrom
Conversation
# Conflicts: # src/Persistence/Initialization/Updates/UpdateVersion.cs
# Conflicts: # src/GameLogic/GameContext.cs # src/GameLogic/OfflineLeveling/OfflineLevelingManager.cs # src/GameLogic/Properties/PlugInResources.resx
# Conflicts: # src/AttributeSystem/StatAttribute.cs
# Conflicts: # src/GameLogic/OfflineLeveling/OfflineLevelingManager.cs
# Conflicts: # src/GameLogic/OfflineLeveling/OfflineLevelingIntelligence.cs # src/GameLogic/OfflineLeveling/OfflineLevelingPlayer.cs
# Conflicts: # src/GameLogic/OfflineLeveling/OfflineLevelingIntelligence.cs
| <ItemGroup Condition="'$(ci)'!='true'"> | ||
| <PackageReference Include="BuildWebCompiler2022" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(ci)'!='true' and '$(OS)'=='Windows_NT'"> |
There was a problem hiding this comment.
@sven-n I added this check as a QOL for Linux users, let me know if this could cause problems in Windows, I can remove.
| <ProjectReference Include="..\ItemEditor\MUnique.OpenMU.Web.ItemEditor.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| <Target Name="CompileSass" BeforeTargets="Build" Condition="'$(ci)'!='true' and '$(OS)'!='Windows_NT'"> |
There was a problem hiding this comment.
@sven-n Also a QOL feature for Linux. Build the assets with sass. It's ignored if its not installed.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the offline leveling system by integrating comprehensive party support, enabling offline players to seamlessly participate in parties, receive assistance from members, and manage their pets. It also introduces an auto-rejoin feature for parties and a more sophisticated conditional skill system for combat, improving the overall robustness and functionality of the offline leveling experience. Additionally, administrative oversight of offline sessions has been added to the Admin Panel. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the game's party and offline leveling systems. Key changes include the introduction of a new PartyManager and OfflinePartyMember to allow players to disconnect and reconnect to their parties, with offline members being represented by snapshots. The offline leveling (MuHelper) system has been extensively refactored to improve skill selection, buff application (including party buffs), pet control, and resource management, with new handlers for buffs, pets, and conditional skills. The admin panel now features a dedicated section to monitor and manage active offline leveling sessions. Additionally, packet definitions for cash shop requests have been updated, and numerous code quality improvements, such as structured logging, refined monster AI target resolution, and updated copyright headers, have been applied across the codebase.
sven-n
left a comment
There was a problem hiding this comment.
Man, that's a huge PR. I just came to the half and wanted to submit the review, so you don't have to wait too long for feedback.
src/GameLogic/NPC/Monster.cs
Outdated
| /// </summary> | ||
| /// <param name="player">The player to check.</param> | ||
| /// <returns>True if this monster is attacking the player; otherwise, false.</returns> | ||
| public bool IsAttackingPlayer(Player player) |
There was a problem hiding this comment.
Why is this required?
If it's not needed, you can make CurrentTarget protected again.
| { | ||
| var strategy = this._player.GameContext.PlugInManager | ||
| .GetStrategy<short, ITargetedSkillPlugin>(skillEntry.Skill!.Number) | ||
| ?? new TargetedSkillDefaultPlugin(); |
There was a problem hiding this comment.
Can we cache the TargetedSkillDefaultPlugin in a class field?
| var buffs = this._buffHandler.ConfiguredBuffIds; | ||
| if (buffs.Any(id => id > 0)) |
There was a problem hiding this comment.
Why is it even checking the buffs? When there is no skill, it can't attack anyway, can it?
There was a problem hiding this comment.
In this new version, if there are no buffs and no attack skills configured, the offline player uses normal attacks. If there are buffs configured, but no attack skills, it's treated as a buff specialized class, like EE, ASM or EBK for buffs only.
| { | ||
| var strategy = this._player.GameContext.PlugInManager.GetStrategy<short, ITargetedSkillPlugin>((short)skill.Number) | ||
| var strategy = this._player.GameContext.PlugInManager.GetStrategy<short, ITargetedSkillPlugin>(skill.Number) | ||
| ?? new TargetedSkillDefaultPlugin(); |
There was a problem hiding this comment.
Reuse a cached TargetedSkillDefaultPlugin?
|
|
||
| var ids = this.GetConfiguredComboSkillIds(); | ||
| if (ids.Count == 0) | ||
| if (ids.Count < 3) |
There was a problem hiding this comment.
3 is a magic number here. Theoretically, a SkillComboDefinition can have more or less than 3 skills, too.
Add a named constant, at least.
| await member.ApplyRegenerationAsync(this._player, healSkill).ConfigureAwait(false); | ||
|
|
||
| // Notify party of health change | ||
| await member.InvokeViewPlugInAsync<Views.Party.IUpdatePartyListPlugIn>(p => p.UpdatePartyListAsync()).ConfigureAwait(false); |
There was a problem hiding this comment.
Shouldn't Party.HealthUpdateElapsed handle this already?
There was a problem hiding this comment.
Removed to implement the default behavior.
src/Interfaces/LocalizedString.cs
Outdated
| ? null | ||
| ? string.Empty |
There was a problem hiding this comment.
Why is it needed? It complicates some places which check the result with ??
There was a problem hiding this comment.
I updated because there were 2 failing tests, this is one of them. I'll adjusts the tests accordingly.
/// Tests that <see cref="LocalizedString.GetTranslation(CultureInfo,bool)"/> returns an empty span when the underlying value is <see langword="null"/>.
/// </summary>
[Test]
public void GetTranslation_ReturnsEmptySpan_WhenValueIsNull()
{
var localized = new LocalizedString(null!);
var result = localized.GetTranslation(new CultureInfo("en"));
Assert.IsEmpty(result);
}
src/GameLogic/Player.cs
Outdated
| public string Name => this.SelectedCharacter?.Name ?? string.Empty; | ||
|
|
||
| /// <inheritdoc /> | ||
| public Guid CharacterId => this.SelectedCharacter?.Id ?? Guid.Empty; |
There was a problem hiding this comment.
Why is this needed? The character can be identified by name, it's unique, too.
My idea was to hide the Guids in game logic where possible, because that's an aspect of persistence.
There was a problem hiding this comment.
Refactored to use character name.
sven-n
left a comment
There was a problem hiding this comment.
Okay, I reviewed the rest. This are the last things I found 😅
| } | ||
| catch (Exception ex) | ||
| { | ||
| this.Logger.LogError(ex, "Failed to initialize offline player for {Name}.", this.Name); | ||
| this.Logger.LogError(ex, "Failed to initialize offline player for {AccountLoginName}.", this.AccountLoginName); | ||
| return false; |
There was a problem hiding this comment.
Can we log this? The ToString returns account name and character name.
Alternatively, log to the Player.Log as it's specific to a player instance, too.
| // All players in the range of the player are getting experience. | ||
| // This might not be like in the original server, where observing the killed monster counts, | ||
| // but at this stage, the monster already has cleared his observers. | ||
| this._distributionList.AddRange(this.PartyList.OfType<Player>().Where(p => p == killer || killer.Observers.Contains(p))); |
There was a problem hiding this comment.
Please reintroduce the _distributionList. I added it to prevent allocations of new lists after every kill. As far as I remember, the _distributionLock is there to lock the access to this list.
src/GameLogic/PartyManager.cs
Outdated
| /// </summary> | ||
| public sealed class PartyManager : IPartyManager | ||
| { | ||
| private readonly System.Collections.Concurrent.ConcurrentDictionary<string, Party> _partyByCharacterName = new(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
StringComparer.OrdinalIgnoreCase might be wrong. Afaik, the character name is case-sensitively unique, which means there can be a character test and Test on the same server.
|
I'm not sure how to prevent Codacy warning on Dispose _healthUpdateCts even though it's already doing that. |
Just ignore false-positives from codacy 😅 |
Conflict resolved. AwardExperienceAsync was modified to add master experience rate when I modified the method to calculate XP in a separate method. |

Offline Leveling Party Support
Summary
This PR allows the offline leveling player to function within a party, receiving/using heals and buffs.
New Features
Core Changes
Manually tested scenarios