Common/physics engine

[Omnielement]

Add a physic engine. It can be useful for making bayou crates in Yggdrasil.

[github-actions]

Holistic Review

Motivation: The physics engine (PBEngine) adds a much-needed foundation for interactive physics objects (bayou crates, moving platforms, etc.) that the mod currently lacks. This is genuine new functionality consistent with the project's content expansion goals.

Approach: The code follows a reasonable architecture (broad-phase → narrow-phase → constraint solving with Gauss-Seidel iteration) and implements standard 2D rigid-body physics with convex polygon/sphere/capsule collision. However, the integration with tModLoader is incomplete: several critical server/client correctness issues, a dead-code bug in tile collision depth reporting, and a heavy-handed global On_Collision.TileCollision hook that risks breaking vanilla entity behavior.

Summary: ⚠️ Needs Changes. This PR provides a solid physics engine core but has blocking issues that will crash or misbehave on dedicated servers and produce incorrect tile collision responses. The author should fix the !Main.dedServ guards, the dead-code bug in GetPolygonTileCollisionInfo, and the wrong import before merging. This is a large PR (~7800 lines) and a human reviewer should also verify the physics correctness.

---

Detailed Findings

❌ Dead code breaks tile collision depth reporting — TileCollisionUtils.GetPolygonTileCollisionInfo

File: Sources/Everglow.Function/Physics/PBEngine/GameInteraction/TileCollisionUtils.cs (lines 113-122)

There is an unconditional return true; on line 116 before the depth comparison at lines 117-122, rendering the latter dead code. The method always returns depth1=0, normal1=Vector2.Zero when a collision is detected. This means the caller (BoxCollider.TestCollisionCondition, line 180) receives zero depth, so the nonlinear projection step in PhysicsSimulation.Resolve never separates colliding bodies from tiles. The collision is detected but the positional correction does nothing — objects will appear to overlap terrain without push-out.

❌ Missing !Main.dedServ guards across multiple files

Per AGENTS.md (Server/Client Correctness checklist): "Client-only logic guarded by !Main.dedServ." Multiple hooks and ModPlayer methods in this PR access client-only APIs without guards and will crash on dedicated servers:

• PhysicsWorldSystem.On_Player_PlayerFrame — Hooks On_Player.PlayerFrame (runs on both client and server), directly modifies Main.LocalPlayer.velocity. Additionally, on the client side, this hook fires for every player's frame update but only modifies the local player's velocity, causing incorrect velocity changes in multiplayer when remote players call PlayerFrame.
• PhysicsPlayer.PreUpdateMovement() — Iterates Main.projectile, Main.npc, accesses Main.LocalPlayer, Main.mouseLeft, Main.keyState without server guard.
• PhysicsPlayer.PostUpdate() — Accesses Main.LocalPlayer without server guard.
• Diamond_Physics_Item.HoldItem() and PhysicsBody_TestSystem.HoldItem() — Call Ins.VFXManager.Add(...) (a client-only registration service) without !Main.dedServ guard.

All of the above need to be wrapped in if (!Main.dedServ) or placed in a separate client-side ModSystem.

❌ Wrong using statement in CollisionEvent2D.cs

File: Sources/Everglow.Function/Physics/PBEngine/Collision/CollisionEvent2D.cs, line 2

using Microsoft.CodeAnalysis.CSharp.Syntax; imports a Roslyn internal compiler namespace that has no place in game runtime code. This appears to be an accidental IDE import. Should be removed.

❌ Heavy-handed On_Collision.TileCollision global hook

File: Sources/Everglow.Function/Physics/PhysicsWorldSystem.cs, lines 22-23, 33-101

PhysicsWorldSystem.Load() hooks On_Collision.TileCollision which is the vanilla tile collision function for all entities (players, NPCs, projectiles). This replaces vanilla collision behavior unconditionally. The hook creates dummy physics objects and runs the physics engine's broad-phase, then calls the original — meaning every entity in the game now pays a physics engine overhead. This could break vanilla behavior for mod bosses, special projectiles, or custom movement. Consider a more targeted approach, such as only overriding collision for physics-engine-aware entities.

❌ CollisionEvent2D.CreateMirrorEvent() — missing field copies

File: Sources/Everglow.Function/Physics/PBEngine/Collision/CollisionEvent2D.cs, lines 53-66

The copy constructor (lines 37-47) copies NormalVelOld and Depth, but CreateMirrorEvent() omits both. A mirrored event should preserve Depth and NormalVelOld for symmetry in contact solving.

⚠️ SpringConstraint.Apply() body entirely commented out

File: Sources/Everglow.Function/Physics/PBEngine/Constraints/SpringConstraint.cs, lines 62-154

The Apply(float deltaTime) method — intended to solve the spring constraint's positional correction — has its entire body commented out. Only ApplyForce() is active. This means the spring constraint's position-level correction (penalty force through compliance) does not converge properly during Gauss-Seidel iterations.

⚠️ Unused variables

• GeometryUtils.cs line 249: bool anyOutside = false; declared but never read (only appears in commented code on line 262).
• Diamond_Physics_Item.cs lines 26-28: SphereCollider rigBody created but never used; a new SphereCollider is immediately constructed on the next line.

⚠️ GAUSS_SEIDEL_ITERS should be a const

File: Sources/Everglow.Function/Physics/PBEngine/PhysicsSimulation.cs, line 22

The property is a getter-only expression-bodied member that always returns 8. This should be a const int GAUSS_SEIDEL_ITERS = 8; to avoid any JIT overhead.

⚠️ Everglow.Core.csproj — unnecessary Remove rules

The diff adds 15 lines of <... Remove="Physics\Everglow.Commons.Physics.PBEngine\**" /> to the Core project, but this path does not exist in Sources/Everglow.Core. The Core project has EnablePathGenerator=false and excludes all tML references — by design it should not care about PBEngine files. These lines are harmless but confusing.

⚠️ No unit tests

PR introduces ~7800 lines of physics simulation code with no corresponding unit tests. Per AGENTS.md testing guidelines, MSTest tests in Sources/Everglow.UnitTests should have been added for the core physics logic (collision detection, constraint solving, rigid body integration) — these are pure math/geometry operations that are ideal for unit testing.

💡 Large commented-out code blocks

Several files contain extensive commented-out code that should be cleaned up:

• BoxCollider.cs: ~180 lines of old collision code (lines 324-507)
• PhysicsSimulation.cs: ~30 lines of old nonlinear projection code (lines 288-316)
• PhysicsPlayer.cs: ~20 lines of debug code (lines 56-59, 149-158, 187-255)

💡 No LocalizationCategory on Example module items

Diamond_Physics_Item and PhysicsBody_TestSystem are ModItem subclasses but do not override LocalizationCategory. Per AGENTS.md, all ModItem/ModNPC/ModProjectile should set LocalizationCategory using a LocalizationUtils.Categories.* constant for proper localization export.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement human review.

Generated by Everglow Holistic Review for #326 · ⊞ 13.7K

[github-actions]

❌ 错误引入 Roslyn 内部命名空间 — Microsoft.CodeAnalysis.CSharp.Syntax 是编译器/分析器的内部类型,不是运行时依赖,不应出现在游戏代码中。此 using 注释在开发阶段被意外导入,应移除。

Suggested change

using Microsoft.CodeAnalysis.CSharp.Syntax;

[github-actions]

❌ 缺少 !Main.dedServ 守卫 — On_Player_PlayerFrame 在第 146/148/153 行多处直接访问 Main.LocalPlayer.velocity ∈ 此钩子在服务端也会触发(On_Player.PlayerFrame 是 Hook,双端运行),但服务端没有真正的 Main.LocalPlayer,会导致异常或状态损坏。

同时,此方法对每个玩家的 PlayerFrame 事件都修改 Main.LocalPlayer.velocity,多人游戏中远程玩家的帧会错误覆盖本地玩家的速度。

需要整体包裹 if (!Main.dedServ) 守卫。

[github-actions]

⚠️ 未使用的变量 —— 第 26 行创建了 SphereCollider rigBody,但从未使用(第 28 行又新建了一个 SphereCollider)。应移除。

Suggested change

var rigBody = new SphereCollider(12);

[github-actions]

❌ 缺少 !Main.dedServ 守卫 — PreUpdateMovement 遍历 Main.projectile(L71)/Main.npc(L91) 并依赖 Main.LocalPlayer(L32/L63)、Main.mouseLeft(L183)、Main.keyState(L196) 等客户端专属 API。ModPlayer 钩子在专用服务端也会执行,这些 API 不可用。

需要整个 PreUpdateMovement 方法包裹 if (!Main.dedServ)。

[github-actions]

❌ 死代码与碰撞深度丢失 — 第 116 行的 return true 是无条件提前返回,之后第 117-122 行的深度比较与法线赋值成为死代码。结果:GetPolygonTileCollisionInfo 返回 true 时 depth1 恒为 0、normal1 恒为 Vector2.Zero,导致调用方(BoxCollider.TestCollisionCondition 第 180 行)拿到错误的碰撞深度和法线,瓷砖碰撞的反馈位移无法正确计算。

应移除第 117 行附近的死循环代码,并在 return true 前正确赋值 depth1/normal1。

Suggested change

	{
	if (depth > maxDepth)
	{
	maxDepth = depth;
	maxNormal = Vector2.Dot(maxNormal, polyCenter - centerTile) < 0 ? -normal : normal;
	}

[github-actions]

⚠️ Apply() 方法体全为空 — 第 62-154 行 Apply(float deltaTime) 的整个方法主体被注释掉,实际不执行任何操作。约束解算的核心步骤缺失。当前只有 ApplyForce() 生效,约束系统不完整。

[github-actions]

⚠️ 未使用的变量 — bool anyOutside = false; 被声明但从未读取(仅出现在注释代码 L262 中)。应移除。

Suggested change

	bool anyOutside = false;

[github-actions]

💡 大量注释代码 — 第 324-507 行包含约 180 行被完整注释掉的旧碰撞检测实现。应清理以避免混淆。如果保留作为未来参考,建议以 // Obsolete: <说明> 简短标注后移除,依赖 git 历史回溯。

[github-actions]

Holistic Review

Motivation: Still valid. The physics engine foundation remains needed for interactive objects and this incremental update does not affect the motivation for the change.

Approach: The incremental changes are exclusively formatting/encoding normalization — adding trailing newlines where missing, removing UTF-8 BOM bytes, and fixing garbled UTF-8 Chinese text. These are all consistent with the .editorconfig requirement ("LF、UTF-8(无 BOM)、文件末尾加新行") and represent correct cleanup. The substantive code concerns raised in the previous review have not been addressed.

Summary: ⚠️ Needs Human Review. The incremental update only contains trailing-newline and encoding fixes which are correct but non-functional. The blocking issues from the prior review (missing !Main.dedServ guards, dead code in TileCollisionUtils, wrong using in CollisionEvent2D, the heavy-handed On_Collision.TileCollision hook, CreateMirrorEvent missing field copies, and commented-out SpringConstraint.Apply) remain unaddressed.

• review 4783249151 — commit 0461cd73b, verdict was ⚠️ Needs Changes, now ⚠️ Needs Changes; assessment unchanged.

---

Detailed Findings

✅ Encoding normalization — all correct

The incremental patch fixes several formatting issues consistent with .editorconfig:

• Adds trailing newlines to CollisionGraph.cs, TileCollisionUtils.cs, PhysicsSimulation.cs, PhysicsWorldSystem.cs, Diamond_Physics_Item.cs, PhysicsBody_TestSystem.cs, and Everglow.Core.csproj.
• Removes UTF-8 BOM from SweepAndPrune.cs.
• Fixes garbled UTF-8 Chinese text in PBEngine/README.md, Example/README.md, Example/VFX/Default.fx, and Utilities/README.md.

No issues introduced by these changes. The UTF-8 BOM removal from SweepAndPrune.cs (a fully commented-out file) is safe and the trailing-newline additions are edge-case cleanups that do not alter runtime behavior.

⚠️ Previous findings remain unaddressed

None of the ❌ or ⚠️ findings from the prior review have been fixed:

• TileCollisionUtils.GetPolygonTileCollisionInfo — the unconditional return true; on line 116 still dead-codes the depth comparison at lines 117–122.
• Multiple files still lack !Main.dedServ guards (PhysicsWorldSystem, PhysicsPlayer, Diamond_Physics_Item, PhysicsBody_TestSystem), which will crash on dedicated servers.
• CollisionEvent2D.cs still imports Microsoft.CodeAnalysis.CSharp.Syntax (line 2) and CreateMirrorEvent() still omits Depth and NormalVelOld.
• SpringConstraint.Apply() body remains entirely commented out (lines 62–154).
• The On_Collision.TileCollision global hook still runs for all entities.
• GAUSS_SEIDEL_ITERS is still an expression-bodied member instead of const.
• No unit tests for the physics logic.

These are noted for the reviewer's convenience; per incremental review rules they are not re-flagged as new findings.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement human review.

Generated by Everglow Holistic Review for #326 · ⊞ 20.9K