Skip to content

Naive support for incoming mob damage - #564

Merged
AngeloTadeucci merged 1 commit into
MS2Community:masterfrom
gugarosa:master
Sep 7, 2025
Merged

Naive support for incoming mob damage#564
AngeloTadeucci merged 1 commit into
MS2Community:masterfrom
gugarosa:master

Conversation

@gugarosa

@gugarosa gugarosa commented Sep 4, 2025

Copy link
Copy Markdown
Contributor

What changed

Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs

  • After handling keyframe attack points, the server now:
    • Resolves targets:
      • Uses AI-provided attackTargets when present.
      • Falls back to a range query via attack.Range.GetPrism(actor.Position, actor.Rotation.Z) and Field.GetTargets(...) when empty.
    • Adds resolved targets to cast.Targets.
    • Calls actor.TargetAttack(cast) to reuse the established damage flow (DamageCalculator, HP updates via StatsPacket.Update, SkillDamagePacket.Damage, and effect triggers).

Why this fixes it

  • Previously, mobs broadcasted attack “Target” events, but never actually applied damage server‑side. Now, at each attack point, damage is calculated and applied the same way as player attacks.

Summary by CodeRabbit

  • Bug Fixes

    • NPC attacks reliably register hits and apply damage/effects, even when targets aren’t pre-specified.
    • Attacks now respect target caps, avoiding unintended multi-hit or no-hit scenarios.
    • Damage and hit notifications are consistently broadcast to players.
  • Improvements

    • Standardized server-side handling of NPC attack targeting for more predictable combat.

@coderabbitai

coderabbitai Bot commented Sep 4, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Adds server-side target resolution in SkillCastAttack for NPC attacks. When no targets are provided, computes targets via range prism and field queries, caps by TargetCount, records target ObjectIds, and routes through actor.TargetAttack to reuse the existing damage/effects pipeline. Adds a missing utility using directive.

Changes

Cohort / File(s) Summary
NPC attack target resolution and pipeline reuse
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs
Import util namespace. In SkillCastAttack: resolve targets if none provided using attack range and field queries; cap by TargetCount; populate cast.Targets with ObjectIds; invoke actor.TargetAttack(cast) to compute and broadcast damage/effects.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor NPC as NPC Actor
    participant SS as SkillState.SkillCastAttack
    participant R as Attack.Range
    participant F as Field
    participant TA as actor.TargetAttack
    participant DP as Damage/Eff. Pipeline
    participant B as Broadcaster

    NPC->>SS: SkillCastAttack(attack, attackTargets)
    alt attackTargets provided
        SS->>SS: resolvedTargets = attackTargets
    else No targets provided
        SS->>R: GetPrism(actor.Position, attack.Params)
        R-->>SS: Area prism
        SS->>F: GetTargets(prism, filters)
        F-->>SS: resolvedTargets
    end
    SS->>SS: Cap to TargetCount (>=1)
    SS->>SS: cast.Targets += target.ObjectId
    SS->>TA: TargetAttack(cast)
    TA->>DP: Calculate damage/effects
    DP-->>TA: Results
    TA->>B: Broadcast results
    B-->>NPC: Notifications
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • Zintixx
  • AngeloTadeucci

Poem

I twitch my whiskers, hop with glee,
Targets found where none should be—
A prism glints, the field agrees,
Then damage flows on data breeze.
Carrots up for cleaner strikes;
Reused paths make nimble bikes—
Thump-thump! The server likes.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs (1)

79-84: Set Position/Direction when falling back to resolved targets

When no attackTargets are provided, cast.Position/cast.Direction aren’t set, which may affect effects/orientation-sensitive logic. Initialize them from the first resolved target.

Apply this diff:

-            int limit = attack.TargetCount > 0 ? attack.TargetCount : 1;
+            int limit = attack.TargetCount > 0 ? attack.TargetCount : 1;
+            // Initialize orientation for damage/effects when using fallback targets
+            cast.Position = actor.Position;
+            cast.Direction = Vector3.Normalize(resolvedTargets[0].Position - actor.Position);
             for (int i = 0; i < resolvedTargets.Count && i < limit; i++) {
                 IActor target = resolvedTargets[i];
                 cast.Targets.TryAdd(target.ObjectId, target);
             }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f9713ff and ed498b5.

📒 Files selected for processing (1)
  • Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs (2)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (3)
  • IActor (217-226)
  • IActor (228-237)
  • TargetAttack (179-211)
Maple2.Server.Game/Model/Field/Actor/IActor.cs (3)
  • IActor (27-27)
  • IActor (28-28)
  • TargetAttack (30-30)
🔇 Additional comments (4)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs (4)

5-5: Confirm necessity of new using

If GetPrism is an extension in Maple2.Server.Game.Util, keep this import; otherwise, drop it to avoid an unused import.


86-88: Good reuse of the existing damage/effects pipeline

Routing mob damage through actor.TargetAttack(cast) keeps behavior consistent with players and avoids duplicating logic.


69-88: Restrict SkillCastAttack to NPCs: MovementState.SkillCast invokes SkillState.SkillCastAttack for all actors—including players—so this block risks double-applying damage. Wrap the damage logic in an NPC-only guard (e.g. if (actor is MobActor)) or otherwise confirm it never runs for player actors.


71-77: Guard attack.Range before GetPrism & ensure correct target‐count handling

Add a null check to avoid NRE when attack.Range is missing, and convert a zero attack.TargetCount into an explicit “unlimited” limit if GetTargets treats 0 as “no results.” Verify your metadata and GetTargets implementation to confirm whether 0 means unlimited or should be handled differently.
Apply this diff:

-        if (resolvedTargets.Count == 0) {
-            // Fallback: query targets from attack range
-            Maple2.Tools.Collision.Prism prism = attack.Range.GetPrism(actor.Position, actor.Rotation.Z);
-            foreach (IActor target in actor.Field.GetTargets(actor, new[] { prism }, attack.Range.ApplyTarget, attack.TargetCount)) {
-                resolvedTargets.Add(target);
-            }
-        }
+        if (resolvedTargets.Count == 0 && attack.Range != null) {
+            // Fallback: query targets from attack range
+            Maple2.Tools.Collision.Prism prism = attack.Range.GetPrism(actor.Position, actor.Rotation.Z);
+            int effectiveLimit = attack.TargetCount > 0 ? attack.TargetCount : int.MaxValue;
+            foreach (IActor target in actor.Field.GetTargets(actor, new[] { prism }, attack.Range.ApplyTarget, effectiveLimit)) {
+                resolvedTargets.Add(target);
+            }
+        }

Comment on lines +79 to +88
if (resolvedTargets.Count > 0) {
int limit = attack.TargetCount > 0 ? attack.TargetCount : 1;
for (int i = 0; i < resolvedTargets.Count && i < limit; i++) {
IActor target = resolvedTargets[i];
cast.Targets.TryAdd(target.ObjectId, target);
}

// Reuse existing pipeline to calculate and broadcast damage/effects
actor.TargetAttack(cast);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Prevent cross-attack-point target accumulation (duplicate damage across APs)

cast.Targets is mutated and never cleared, so subsequent attack points may re-hit prior targets unintentionally. Clear the targets after applying damage, or add/remove only the newly added ids within this AP.

Apply this diff (simple/safe clear after damage):

-            actor.TargetAttack(cast);
+            // Ensure per-AP targeting to avoid double-hits on later APs
+            actor.TargetAttack(cast);
+            cast.Targets.Clear();

If other systems rely on cumulative targets, instead track only newly added ids and remove them post-attack.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (resolvedTargets.Count > 0) {
int limit = attack.TargetCount > 0 ? attack.TargetCount : 1;
for (int i = 0; i < resolvedTargets.Count && i < limit; i++) {
IActor target = resolvedTargets[i];
cast.Targets.TryAdd(target.ObjectId, target);
}
// Reuse existing pipeline to calculate and broadcast damage/effects
actor.TargetAttack(cast);
}
if (resolvedTargets.Count > 0) {
int limit = attack.TargetCount > 0 ? attack.TargetCount : 1;
for (int i = 0; i < resolvedTargets.Count && i < limit; i++) {
IActor target = resolvedTargets[i];
cast.Targets.TryAdd(target.ObjectId, target);
}
// Ensure per-AP targeting to avoid double-hits on later APs
actor.TargetAttack(cast);
cast.Targets.Clear();
}
🤖 Prompt for AI Agents
In Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs around
lines 79 to 88, cast.Targets is being appended to each attack point and never
cleared, causing targets to accumulate across attack points and receive
duplicate damage; after calling actor.TargetAttack(cast) clear only the
per-attack targets (e.g., cast.Targets.Clear() or remove the IDs added for this
AP) so subsequent APs start with an empty target set; if cumulative behavior is
required elsewhere, instead track the newly added IDs in a temporary list and
remove them from cast.Targets after actor.TargetAttack(cast).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (13)
Maple2.Tools/Dotenv.cs (2)

27-29: Handle escapes and inline comments robustly.
Current quote stripping doesn’t unescape sequences and won’t strip inline comments when unquoted (e.g., FOO=bar # note).

Apply:

-            if ((value.StartsWith("\"") && value.EndsWith("\"")) || (value.StartsWith("'") && value.EndsWith("'"))) {
-                value = value.Substring(1, value.Length - 2);
-            }
+            bool isQuoted = value.Length >= 2 &&
+                            ((value[0] == '"' && value[^1] == '"') || (value[0] == '\'' && value[^1] == '\''));
+            if (isQuoted) {
+                value = value[1..^1]
+                    .Replace("\\n", "\n")
+                    .Replace("\\r", "\r")
+                    .Replace("\\t", "\t")
+                    .Replace("\\\"", "\"")
+                    .Replace("\\'", "'");
+            } else {
+                int hash = value.IndexOf('#');
+                if (hash >= 0) value = value[..hash].TrimEnd();
+            }

31-35: Clarify empty-var override semantics.
You currently override when existing is empty. If “empty means intentionally set by host,” switch to null-only.

Option A (do not override when empty):

-            string? existing = Environment.GetEnvironmentVariable(key);
-            if (string.IsNullOrEmpty(existing)) {
+            string? existing = Environment.GetEnvironmentVariable(key);
+            if (existing is null) {
                 Environment.SetEnvironmentVariable(key, value);
             }

If the current behavior is intended, add a brief comment stating that empty is treated as “not set”.

Maple2.Server.Game/Program.cs (1)

57-82: Solid exponential backoff with jitter; add a per-RPC deadline and de-dupe delay calc.
Prevent a single RPC from hanging too long and avoid duplicating delay math.

Apply:

-while (true) {
+while (true) {
     try {
-        response = worldClient.AddChannel(new AddChannelRequest {
+        response = worldClient.AddChannel(new AddChannelRequest {
             GameIp = Target.GameIp.ToString(),
             GrpcGameIp = Target.GrpcGameIp,
             InstancedContent = overrideInstanced || Target.InstancedContent,
-        });
+        }, deadline: DateTime.UtcNow.AddSeconds(2));
         if (response != null && response.GamePort != 0 && response.GrpcPort != 0) {
             break; // Success with valid ports
         }
         // Received an invalid allocation (likely due to a race). Retry.
-        attempt++;
-        int baseDelayMs = (int)Math.Min(30000, 1000 * Math.Pow(2, Math.Min(attempt, 6)));
-        int jitterMs = Random.Shared.Next(250, 1000);
-        int delayMs = baseDelayMs + jitterMs;
+        int delayMs = ComputeBackoff(++attempt);
         Log.Warning("World returned invalid ports. Retry {Attempt} in {DelayMs}ms", attempt, delayMs);
         await Task.Delay(delayMs);
-    } catch (RpcException) {
-        attempt++;
-        int baseDelayMs = (int)Math.Min(30000, 1000 * Math.Pow(2, Math.Min(attempt, 6)));
-        int jitterMs = Random.Shared.Next(250, 1000);
-        int delayMs = baseDelayMs + jitterMs;
+    } catch (RpcException) {
+        int delayMs = ComputeBackoff(++attempt);
         Log.Warning("World not ready yet. Retry {Attempt} in {DelayMs}ms", attempt, delayMs);
         await Task.Delay(delayMs);
     }
 }

Add (outside this hunk):

static int ComputeBackoff(int attempt) {
    int baseDelayMs = (int)Math.Min(30_000, 1000 * Math.Pow(2, Math.Min(attempt, 6)));
    int jitterMs = Random.Shared.Next(250, 1000);
    return baseDelayMs + jitterMs;
}

Optional: also honor cancellation (ApplicationStopping) to exit the loop on shutdown.

scripts/stop_servers.ps1 (1)

15-21: Expose a stop timeout parameter.
Allows graceful vs. fast stops, works with v1/v2.

Apply:

-param([string[]]$Service)
+param([string[]]$Service, [int]$Timeout = 10)
@@
-  Compose stop @Service
+  Compose stop -t $Timeout @Service
@@
-  Compose stop
+  Compose stop -t $Timeout
Maple2.Server.World/Service/WorldService.Migrate.cs (2)

36-51: Simplify channel selection and avoid double endpoint checks.
You validate active endpoint twice (Lines 38–40 and again at 53–55). Keep a single check after selection.

Apply:

-                if (request.InstancedContent && channelClients.TryGetInstancedChannelId(out int channel)) {
-                    if (!channelClients.TryGetActiveEndpoint(channel, out _)) {
-                        throw new RpcException(new Status(StatusCode.Unavailable, "No available instanced game channel"));
-                    }
-                } else if (request.HasChannel && channelClients.TryGetActiveEndpoint(request.Channel, out _)) {
+                if (request.InstancedContent && channelClients.TryGetInstancedChannelId(out int channel)) {
+                    // channel selected; active check occurs below
+                } else if (request.HasChannel && channelClients.TryGetActiveEndpoint(request.Channel, out _)) {
                     channel = request.Channel;
                 } else {
                     // Fall back to first available non-instanced channel, then instanced if none
                     channel = channelClients.FirstChannel();
                     if (channel == -1) {
                         if (!channelClients.TryGetInstancedChannelId(out channel)) {
                             throw new RpcException(new Status(StatusCode.Unavailable, "No available game channels"));
                         }
                     }
                 }

Optional: replace channelClients.Count == 0 with channelClients.FirstChannel() == -1 to reflect the actual non‑instanced availability check used later.


53-55: Unify error message for inactive/unknown channel.
Consider specifying instanced/non-instanced in the message for clarity.

Example:

-    throw new RpcException(new Status(StatusCode.Unavailable, $"Channel {channel} not found"));
+    throw new RpcException(new Status(StatusCode.Unavailable, $"Channel {channel} not available (inactive or missing)"));
Maple2.Server.World/Containers/ChannelClientLookup.cs (3)

123-125: Be robust to hostnames in gameIp (avoid Parse-only path).

IPAddress.Parse(gameIp) will throw for hostnames (e.g., "localhost" or DNS names). Prefer TryParse + DNS fallback.

-            IPAddress ipAddress = IPAddress.Parse(gameIp);
+            IPAddress ipAddress = IPAddress.TryParse(gameIp, out var parsed)
+                ? parsed
+                : Dns.GetHostAddresses(gameIp).First(a => a.AddressFamily == System.Net.Sockets.AddressFamily.InterNetwork);
             IPEndPoint gameEndpoint = new IPEndPoint(ipAddress, newGamePort);

143-147: Use Task.Run for async delegate; StartNew + async loses exceptions and the CTS isn’t tracked.

Task.Factory.StartNew with an async-returning delegate fire-and-forgets a nested task and can drop exceptions. Also, the CancellationTokenSource is not retained, so you can’t cancel monitors.

-                var cancel = new CancellationTokenSource();
-                Task.Factory.StartNew(() => MonitorChannel(activeChannel, cancel), cancellationToken: cancel.Token);
+                var cancel = new CancellationTokenSource();
+                _ = Task.Run(() => MonitorChannel(activeChannel, cancel), cancel.Token);

If you want controlled shutdown, store cancel on the Channel and cancel on disposal. For example (outside this hunk):

// In Channel class
public CancellationTokenSource? MonitorCts { get; init; }

And set it when creating activeChannel, then cancel it when removing the channel.


157-159: Attempt cap off-by-one.

attempts++ precedes the check, so this allows 101 tries. Use >= 100.

-            if (attempts > 100) {
+            if (attempts >= 100) {
                 logger.Error("Failed to allocate a channel after {Attempts} attempts", attempts);
                 return (0, 0, -1);
             }
scripts/start_servers.ps1 (3)

37-46: Treat 'unhealthy' explicitly to aid diagnostics.

Right now only 'exited' emits logs. Add handling for 'unhealthy' to surface recent logs.

-    } elseif ($status -match '^(running|starting|created)$') {
+    } elseif ($status -match '^(running|starting|created)$') {
       # still waiting
-    } elseif ($status -match 'exited') {
+    } elseif ($status -eq 'unhealthy') {
+      Write-Warning "$Service is unhealthy. Showing last logs:"
+      try { Compose logs --no-color --tail=200 $Service } catch { }
+      # keep waiting; timeout logic below will handle exit
+    } elseif ($status -match 'exited') {
       Write-Warning "$Service exited unexpectedly. Showing last logs:"
       try { Compose logs --no-color --tail=200 $Service } catch { }
       if ($Soft) { return $false } else { throw "$Service exited" }
     }

119-121: Include web in the suggested tail command.

Useful for first-run troubleshooting.

-$joined = ($started + @('world','login')) -join ' '
+$joined = ($started + @('world','login','web')) -join ' '

12-20: Optional: support a project name to avoid compose namespace collisions.

Add -p $ProjectName pass-through when provided.

-param(
+param(
   [int[]]$NonInstancedChannels,
   [switch]$IncludeInstanced,
-  [switch]$NoBuild
+  [switch]$NoBuild,
+  [string]$ProjectName
 )
@@
 function Compose {
   param([Parameter(ValueFromRemainingArguments=$true)][string[]]$Args)
-  if ($UseV2) { & docker compose @Args } else { & docker-compose @Args }
+  if ($UseV2) {
+    if ($PSBoundParameters.ContainsKey('ProjectName') -and $ProjectName) { & docker compose -p $ProjectName @Args } else { & docker compose @Args }
+  } else {
+    if ($PSBoundParameters.ContainsKey('ProjectName') -and $ProjectName) { & docker-compose -p $ProjectName @Args } else { & docker-compose @Args }
+  }
 }
compose.yml (1)

21-39: MySQL port is published to host; ensure this is intended.

Exposing 3306 is fine for local dev but risky otherwise. Consider removing the port mapping or limiting to 127.0.0.1 in non-dev.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ed498b5 and a8142b3.

📒 Files selected for processing (7)
  • Maple2.Server.Game/Program.cs (1 hunks)
  • Maple2.Server.World/Containers/ChannelClientLookup.cs (1 hunks)
  • Maple2.Server.World/Service/WorldService.Migrate.cs (2 hunks)
  • Maple2.Tools/Dotenv.cs (2 hunks)
  • compose.yml (2 hunks)
  • scripts/start_servers.ps1 (1 hunks)
  • scripts/stop_servers.ps1 (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Maple2.Server.Game/Program.cs (2)
Maple2.Server.Core/Constants/Target.cs (2)
  • Target (5-77)
  • Target (29-76)
Maple2.Server.World/Service/WorldService.GamePorts.cs (1)
  • Task (7-15)
🔇 Additional comments (5)
Maple2.Tools/Dotenv.cs (1)

15-17: Good: comment handling now ignores leading whitespace.
This fixes common ".env" patterns like " # comment".

scripts/stop_servers.ps1 (1)

9-14: Nice compose v1/v2 shim.
Good portability and strict mode.

Maple2.Server.World/Service/WorldService.Migrate.cs (1)

31-35: Reasonable fallback when no non-instanced channels are active.
This avoids hard failures when an instanced channel exists.

compose.yml (2)

78-84: Ensure healthcheck uses grpc_health_probe or installs bash
Ripgrep shows no bash installation or grpc_health_probe in any Dockerfile; the current bash -c 'exec 3<>/dev/tcp/…' will fail if bash isn’t present. Either add bash to each image or replace the healthcheck with:

healthcheck:
  test: ["CMD","grpc_health_probe","-addr=127.0.0.1:21001"]
  interval: 5s
  timeout: 3s
  retries: 30
  start_period: 5s

99-114: Healthcheck port is correct
Target.GrpcLoginPort is set to 21000 in Core/Constants/Target.cs and the Login service binds gRPC on port 21000 internally; no changes needed.

Comment on lines 128 to 120
bool isDocker = Environment.GetEnvironmentVariable("DOTNET_RUNNING_IN_CONTAINER") == "true";
if (isDocker) {
// When running in Docker, use the provided grpcGameIp as hostnames
grpcUri = new Uri($"http://{grpcGameIp}:{newGrpcChannelPort}");
} else {
// Outside of Docker, parse the IP addresses normally
IPAddress grpcIpAddress = IPAddress.Parse(grpcGameIp);
grpcUri = new Uri($"http://{grpcIpAddress}:{newGrpcChannelPort}");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Don't assume grpcGameIp is an IP outside Docker; parsing will throw on hostnames.

Unify URI construction and allow DNS names in all environments.

-            bool isDocker = Environment.GetEnvironmentVariable("DOTNET_RUNNING_IN_CONTAINER") == "true";
-            if (isDocker) {
-                // When running in Docker, use the provided grpcGameIp as hostnames
-                grpcUri = new Uri($"http://{grpcGameIp}:{newGrpcChannelPort}");
-            } else {
-                // Outside of Docker, parse the IP addresses normally
-                IPAddress grpcIpAddress = IPAddress.Parse(grpcGameIp);
-                grpcUri = new Uri($"http://{grpcIpAddress}:{newGrpcChannelPort}");
-            }
+            // Allow IPs or hostnames uniformly
+            grpcUri = new Uri($"http://{grpcGameIp}:{newGrpcChannelPort}");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bool isDocker = Environment.GetEnvironmentVariable("DOTNET_RUNNING_IN_CONTAINER") == "true";
if (isDocker) {
// When running in Docker, use the provided grpcGameIp as hostnames
grpcUri = new Uri($"http://{grpcGameIp}:{newGrpcChannelPort}");
} else {
// Outside of Docker, parse the IP addresses normally
IPAddress grpcIpAddress = IPAddress.Parse(grpcGameIp);
grpcUri = new Uri($"http://{grpcIpAddress}:{newGrpcChannelPort}");
}
// Allow IPs or hostnames uniformly
grpcUri = new Uri($"http://{grpcGameIp}:{newGrpcChannelPort}");
🤖 Prompt for AI Agents
In Maple2.Server.World/Containers/ChannelClientLookup.cs around lines 128 to
136, the code currently parses grpcGameIp as an IP address outside Docker which
will throw for hostnames; instead unify URI construction so grpcGameIp is used
verbatim as the host in both cases (do not call IPAddress.Parse), ensure
grpcGameIp is non-empty and the port/newGrpcChannelPort is validated, and create
the Uri with string interpolation like
$"http://{grpcGameIp}:{newGrpcChannelPort}" for all environments so DNS names
are supported.

@AngeloTadeucci

Copy link
Copy Markdown
Collaborator

@gugarosa can you make a new PR with only the docker stuff?

@gugarosa

gugarosa commented Sep 6, 2025

Copy link
Copy Markdown
Contributor Author

@gugarosa can you make a new PR with only the docker stuff?

Of course, my bad. I ended up doing some additional work on top of the same branch. It is reverted now, only the initial change for damage is on this PR.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs (1)

86-88: Prevent cross-attack-point target accumulation (duplicate damage across APs).

cast.Targets is appended but never cleared; subsequent APs will re-hit prior targets. Clear after applying damage.

-            // Reuse existing pipeline to calculate and broadcast damage/effects
-            actor.TargetAttack(cast);
+            // Reuse existing pipeline to calculate and broadcast damage/effects
+            actor.TargetAttack(cast);
+            // Ensure per-AP targeting to avoid double-hits on later APs
+            cast.Targets.Clear();
🧹 Nitpick comments (1)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs (1)

81-84: Optionally guard against self-targeting.

If ApplyTarget or upstream lists ever include the attacker, skip it to avoid self-damage.

-                IActor target = resolvedTargets[i];
-                cast.Targets.TryAdd(target.ObjectId, target);
+                IActor target = resolvedTargets[i];
+                if (target.ObjectId != actor.ObjectId) {
+                    cast.Targets.TryAdd(target.ObjectId, target);
+                }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a8142b3 and a89fac7.

📒 Files selected for processing (1)
  • Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs (2 hunks)
🔇 Additional comments (3)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs (3)

5-5: LGTM: required using for range utilities.

The added using appears necessary for Range.GetPrism/ApplyTarget extensions.


73-75: Verify collision angle units. actor.Rotation.Z is in degrees; confirm that SkillMetadataRange.GetPrism (and underlying Rectangle/Trapezoid) interprets the angle parameter as degrees—not radians—to avoid skewed target prisms.


80-84: Honor TargetCount==0 as unlimited instead of forcing single‐target.

-    int limit = attack.TargetCount > 0 ? attack.TargetCount : 1;
-    for (int i = 0; i < resolvedTargets.Count && i < limit; i++) {
+    int limit = attack.TargetCount > 0
+        ? Math.Min(attack.TargetCount, resolvedTargets.Count)
+        : resolvedTargets.Count; // 0 ⇒ unlimited
+    for (int i = 0; i < limit; i++) {
         IActor target = resolvedTargets[i];
         cast.Targets.TryAdd(target.ObjectId, target);
     }

Verify that Field.GetTargets fallback correctly treats TargetCount==0 as unlimited.

@AngeloTadeucci
AngeloTadeucci merged commit 802964b into MS2Community:master Sep 7, 2025
4 checks passed
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.

2 participants