Skip to content

fix: 修复导航边界与路径平滑正确性 - #717

Open
zhibeigg wants to merge 1 commit into
TabooLib:dev/6.3.0from
zhibeigg:fix/703-navigation-correctness
Open

fix: 修复导航边界与路径平滑正确性#717
zhibeigg wants to merge 1 commit into
TabooLib:dev/6.3.0from
zhibeigg:fix/703-navigation-correctness

Conversation

@zhibeigg

@zhibeigg zhibeigg commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

原有问题

导航模块沿用了不完整的世界高度、水体和直线路径判定:

  • 高度查询与纵向扫描没有始终使用目标坐标及当前世界的真实最小/最大高度,现代负高度世界可能越界。
  • 节点哈希在部分坐标组合下发生碰撞,不同位置可能被当成同一节点。
  • 水和岩浆、可游泳位置及随机陆地候选筛选不够严格。
  • 路径平滑只验证抽象连线,未按实体碰撞箱、支撑方块、危险方块和窄通道检查整段路径。

典型触发场景与后果

  • Minecraft 1.18+ 负 Y 世界、自定义高度世界或接近上下边界寻路:读取越界、漏掉可行节点或生成非法高度。
  • 较远坐标寻路时节点哈希碰撞:开放/关闭集合错误覆盖,路径随机绕路、失败或不稳定。
  • 水边、岩浆附近或 Folia 区域随机选点:把危险液体或错误区域候选当成可行位置。
  • 高个/宽体实体经过墙角、台阶、窄门或悬空位置时平滑路径:删除必要节点后直线穿墙、穿过危险方块或失去地面支撑。

本 PR 修改

  • 使用版本兼容的世界最小/最大高度,并按目标 x/z 执行边界安全的纵向扫描。
  • 修复节点坐标哈希,避免不同位置碰撞。
  • 明确区分水和岩浆,修正可游泳状态、Folia 目标区域评分及随机陆地候选筛选。
  • 按实体完整碰撞箱扫掠验证平滑路径,检查障碍、液体、危险方块、支撑高度和通道宽度。
  • 增加现代世界高度、哈希、水域筛选、墙角/窄通道和平滑安全测试。

修改目的

确保寻路结果与真实世界边界和实体体积一致,平滑过程只能删除真正可安全直达的节点,避免路径穿墙、越界或进入危险地形。

兼容性与行为变化

  • 不修改公开导航 API。
  • 路径结果可能改变,但变化是从不安全或越界路线修正为可通行路线。

验证

  • ./gradlew :module:bukkit:bukkit-navigation:test --rerun-tasks --no-parallel
  • ./gradlew :module:bukkit:bukkit-navigation:build --rerun-tasks --no-parallel
  • git diff --check(仅 Windows LF→CRLF 提示)

Refs #703

@FxRayHughes

Copy link
Copy Markdown
Contributor

Code Review — #717 fix: 修复导航边界与路径平滑正确性

负高度世界边界、isWater() 误判、碰撞箱扫掠平滑这几处修得对,尤其"平滑只能删除真正可安全直达的节点"这个方向是对的——旧实现只按抽象连线采样,完全没考虑实体体积。

但有一处需要说清楚:PR 描述说"修复节点坐标哈希,避免不同位置碰撞",实际上哈希函数本身没有改,加的是开放寻址探测。我实测发现探测序列的周期是 2^28 而非 2^32,并且构造出了第二类碰撞(x 的第 8 位与 z<0 标志位重叠)。这条需要作者确认是有意为之还是遗漏。

另外与 #712 有一处需要协调#712callRegion 改成了不在拥有线程就抛异常,而本 PR 保留了全部 callRegion 调用。

审阅方式:读 patch + 对照源码 + 实测(哈希碰撞枚举、探测序列周期、Spigot 1.20.4 的 Material 枚举)。未实跑 gradle 测试


🟡 问题 1 — 哈希碰撞没有真修,只是加了开放寻址;探测序列周期为 2^28

Node.createHashNode.kt:132-134本 PR 未改动

fun createHash(x: Int, y: Int, z: Int): Int {
    return y and 0xFF or (x and 0x7FFF shl 8) or (z and 0x7FFF shl 24) or (if (x < 0) -0x80000000 else 0) or (if (z < 0) 0x8000 else 0)
}

y and 0xFF 只保留 8 位,所以 1.18+ 的 −64..320(385 格)必然碰撞。测试文件里也直接把这个事实固化了:

assertEquals(Node.createHash(4, -64, 8), Node.createHash(4, 192, 8))

我实测了单柱碰撞规模:

=== 单个 (x,z) 柱上 y=-64..320 (385 格) 的哈希碰撞 ===
唯一哈希数 = 256 / 385
参与碰撞的 y 数 = 258
  例: y=[0, 256]

第二类碰撞我构造出来了。 x and 0x7FFF shl 8 的第 8 位落在 0x8000,与 if (z < 0) 0x8000 标志位完全重叠:

createHash(128, 5, 1)      = 0x01008005
createHash(0,   5, -32767) = 0x01008005
碰撞 = true

x=128, z=1x=0, z=-32767 哈希相同。x=128 是很常见的坐标。

开放寻址本身是可行的补救,getOrCreateNavigationNode 会比对 existing.x/y/z 再决定复用还是继续探测,逻辑正确。但探测序列有个细节:

key = key * 31 + 1
check(key != initialKey) { "Unable to resolve navigation node hash collision" }

我实测了这个序列的周期:

seed=0:         回到 seed=true, 步数=268435456
seed=134218944: 回到 seed=true, 步数=268435456

周期是 2^28 = 268435456,不是完整的 2^32。也就是说这个探测序列只能覆盖 int 空间的 1/16。实践中 nodes 表远小于 2^28 所以不会真的走满,但:

  1. check 的失败条件是"绕回 initialKey",要走 2.68 亿次乘法才会触发——如果真的进入病态情况,这个"保护"实际上是 CPU 长时间空转而非快速失败
  2. 更常见的风险是探测链变长:同柱 385 个 y 挤进 256 个哈希槽,负载因子已经很高,链式探测会拖慢 getNode——而 getNode 在 A* 的邻居展开里是热点路径

建议:直接修 createHash。y 用 12 位(覆盖 −2048..2047,足够所有现代世界)、x/z 各用 10 位并做坐标偏移,或者干脆改用 Long 键((x.toLong() and 0x3FFFFF shl 42) or (z.toLong() and 0x3FFFFF shl 20) or (y.toLong() and 0xFFFFF))配合 HashMap<Long, Node>。这样能一次消除两类碰撞,也不需要开放寻址。

如果因为 Node.hash 是公开字段、担心破坏兼容而不改,建议在 PR 描述里把"修复节点坐标哈希"改成"缓解哈希碰撞的后果",并在 getOrCreateNavigationNode 上加注释说明 y 位宽限制。


🟡 问题 2 — 与 #712callRegion 改动冲突

#712Location.callRegion / Entity.callRegion 从"不在拥有线程就阻塞等待"改成 check(isOwnedByCurrentRegion()) { ... } 抛异常,同时把非 Folia 下的 isOwnedByCurrentRegion() 从恒 true 改成 Bukkit.isPrimaryThread()

而本 PR 保留了全部 callRegion 调用,并且还新增了一处:

 fun getWalkTargetValue(pos: Vector): Double {
-    return location.callRegion {
+    val world = location.world!!
+    return pos.toLocation(world).callRegion {

按目标坐标取区域是对的(旧代码用 location,在 Folia 上可能取到错误区域)。但两个 PR 合并后,navigation 模块的 15 处 callRegion 在异步线程调用时会抛异常——包括普通 Paper 服务器,因为 #712 改了非 Folia 分支的语义。

这条我在 #712 的审阅里也提了。建议两位作者或 #720 整合时确认:要么 #712 保持非 Folia 下 isOwnedByCurrentRegion() 返回 true,要么本 PR 把这些调用迁到 callRegionAsync。寻路放异步线程跑是常见用法,navigation 模块自身没有任何异步封送。


🟡 问题 3 — isStandableAtRegion 的支撑高度判定可能过严

val supportY = below.y + NMS.instance.getBlockHeight(below)
if (abs(supportY - y) > 1.0E-3) {
    return false
}

要求脚下方块的顶面精确等于(误差 1e-3)传入的 y。这比旧代码的 if (below.type.isAirLegacy()) return false 严格得多,方向是对的(旧判定只要不是空气就算有支撑,台阶/农田/雪层的高度差完全没考虑)。

但精确相等这个约束有两个隐患:

  1. 浮点等值比较。 y 来自 nodeCenter(node)node.y.toDouble(),是整数值;getBlockHeight 返回的可能是 0.5(台阶)、0.9375(雪层 15)等。整数 y 与 below.y + height 相等要求 height 恰好为 1.0。也就是说只有完整方块才能通过,半砖、农田(0.9375)、雪层全部会被判为不可站立。
  2. 这会让平滑几乎不删节点——只要路径经过任何非满方块,hasLineOfSightAtRegion 就返回 false。平滑退化为无操作虽然安全,但也失去了意义。

如果意图是"允许站立在任何顶面高度与实体脚部一致的方块上",那实体在半砖上时 node.y 本身应该是什么值?这取决于 A* 节点的 y 语义(getStartAtRegiony = NumberConversions.floor(entity.location.y + 0.5))。建议确认一下节点 y 与方块顶面的对应关系,或者把判定放宽为"顶面不低于 y 且不高于 y + 台阶容差"。

顺带一提,isSafeSmoothingFeetType 要求 malus == 0.0f,而 PathType.WATER 等类型的 malus 取决于实体(entity.getPathfindingMalus)。对会游泳的实体水面 malus 可能为 0,此时平滑允许穿水——这与 PR 描述"检查液体"的表述略有出入,但实际行为跟随实体能力,我认为是合理的。


🔵 次要

a. getStartAtRegion 的 else 分支在找不到支撑时返回 minHeight。

y = if (ground.type.isSolid) blockposition.up().blockY.coerceAtMost(maxHeight - 1) else minHeight

旧代码是 y = blockposition.up().blockY,循环条件 blockposition.y > 0 退出后无论是否找到实心方块都用当前位置。新代码在整柱都不是实心时回落到 minHeight——即世界底部。这比旧行为(可能是 y=1)更明确,但 minHeight 处通常是基岩/虚空,作为起点节点未必合理。考虑返回一个"无效节点"或保留实体当前 y。

b. addSweepBoundaries 只处理 x/z 边界,没有对角穿越的额外采样。 边界集合是"碰撞箱四条边跨越整数格线的 t 值"加上首尾,再对每对相邻 t 取中点。这个采样策略对轴向移动是完备的,但对角移动时如果实体正好从两个方块的公共顶点穿过(经典的"穿角"问题),x 边界和 z 边界在同一个 t 上重合,sortedSetOf 去重后只留一个采样点,中点采样落在格子内部而非顶点两侧。可能漏掉对角穿墙。建议对 x/z 边界重合的 t 额外取 t±ε 两个采样点。

c. check(key != initialKey) 的错误信息缺少坐标。 抛出时只说"Unable to resolve navigation node hash collision",没有 x/y/z。如果真的触发(虽然要 2.68 亿次探测),排查时没有任何线索。建议带上坐标。

d. Fluid.getFluid 的 waterlogged 分支只在 1.13+ 生效但没缓存。 每次调用都 blockData as? Waterlogged,而 blockData 在 Bukkit 里是每次 getBlockData() 新建对象(至少 Spigot 实现是 clone)。getFluidgetStartAtRegion 的循环里被反复调用,这会产生可观的临时对象。考虑在 getCachedBlockType 那一层缓存,或至少在循环内复用。

e. isWater() 的精确匹配我核实过是安全的。 我实测了 Spigot 1.20.4 的全部含 "WATER" 枚举:

WATER_BUCKET             旧contains=true  新精确=false   ← 行为改变
WATER                    旧contains=true  新精确=true
WATER_CAULDRON           旧contains=true  新精确=false   ← 行为改变
LEGACY_WATER             旧contains=true  新精确=false   ← 行为改变
LEGACY_STATIONARY_WATER  旧contains=true  新精确=false   ← 行为改变
LEGACY_WATER_LILY        旧contains=true  新精确=false   ← 行为改变
LEGACY_WATER_BUCKET      旧contains=true  新精确=false   ← 行为改变

旧实现会把 WATER_BUCKETWATER_CAULDRONLEGACY_WATER_LILY(睡莲) 全部误判为水,这是真 bug。LEGACY_* 那几个我确认过不影响:1.13+ 服务端 getType() 不会返回 LEGACY 枚举(isLegacy() == true 的枚举仅存在于兼容层),而 1.12 及更早的 API jar 里水方块名本就是 WATER / STATIONARY_WATER。所以精确匹配覆盖全版本,与 Fluid.getFluid 的既有约定也一致。这条修得对。


🟢 已核对无误

结论
navigationMinHeight() 版本兼容 if (MinecraftVersion.isHigherOrEqual(V1_17)) minHeight else 0World.getMinHeight() 是 1.17 加入的 API,1.16 及以下调用会 NoSuchMethodError,这个版本门槛设置正确
isWithinNavigationHeight 边界 y >= minHeight && y < maxHeight。Bukkit 的 maxHeight 是排他上界(1.20 为 320,最高可放方块的 y 是 319),用 < 正确
getBlockAtIfLoaded 前置越界检查 新增的高度检查在 callRegion 之前,避免了越界坐标进入区域调度。旧代码会把越界 y 传给 getBlockAt
纵向扫描的边界安全 getStartAtRegion 的三处 while 循环都加了 y < maxHeight - 1 上界与 > minHeight 下界。旧代码 while (true) + blockposition.y > 0 在负高度世界会漏掉 y<0 的可行节点,在接近上界时会越界读
循环结构从 while(true)+break 改为条件式 旧代码 while (true) { if (!cond) { --y; break }; ++y; ... } 与新代码 while (cond && y < max) { ++y; ... }; if (!cond) --y 在正常路径上等价,但新代码在撞到上界时不会多减一次 y。语义保持
PathType.OPEN 下坠检查的下界 if (air < world.navigationMinHeight()) 替代 if (air < 0)。负高度世界里旧代码会一直往下探到 y<minHeight,新代码正确截断
getWalkTargetValue 改用目标坐标取区域 pos.toLocation(world).callRegion { ... } 替代 location.callRegion。Folia 下评分的目标点可能不在实体所在区域,按目标坐标取区域是正确的
isInWater 区分水与岩浆 location.block.isLiquid 对岩浆也返回 true,导致岩浆里的实体被当作"在水中"从而走 canFloat 分支。改用 getFluid().isWater() 正确
waterlogged 识别 1.13+ 通过 (blockData as? Waterlogged)?.takeIf { it.isWaterlogged } 识别含水台阶/楼梯/栅栏等。旧代码完全不认这类方块。版本门槛 V1_13 正确(Waterlogged 接口是 1.13 引入)
getOrCreateNavigationNode 的正确性 探测时比对 existing.x/y/z 三者全等才复用,否则继续探测;空槽则新建。逻辑上不会把不同坐标当成同一节点,达到了"避免碰撞后果"的目的
RandomPositionGenerator 的 moveUp 上界 moveUp(result, 0, world.maxHeight) 替代硬编码 256。1.18+ 世界高度 320,旧代码在 y>256 的区域完全无法向上探测
acceptsNavigationSurface 的逻辑 allowWater || !isWater。旧代码 onWater || blockType?.isWater() == true 是"允许水当前是水"——即不允许水时反而接受水面候选,逻辑写反了。新版是"允许水,或当前不是水"。真 bug 修复
PathSmoothing.smooth 空路径保护 if (path.nodes.isEmpty()) return emptyList()。旧代码 smoothAtRegionnodes.size <= 2 会走 nodes.map { nodeCenter(it) },空列表返回空列表,本身不崩;但新增的前置检查避免了进入 callRegion(在 #712 语义下会抛异常)。有实际价值
碰撞箱扫掠替代固定步长 旧代码 SAMPLE_STEP = 0.5 固定采样,可能跳过窄障碍;新代码按实体碰撞箱四边跨格的精确 t 值采样 + 相邻中点。对轴向移动是完备的
maxBx / maxBz 的计算 ceil(x + halfWidth).toInt() - 1 替代 floor(x + halfWidth).toInt()。当 x + halfWidth 恰好落在整数边界时,旧代码会多算一格(实体边缘贴合格线但未进入下一格)。新版正确
身体空间检查改用 PathType 旧代码 if (block.type.isSolid) return false 只看实心;新代码走 PathTypeFactory + entity.getPathfindingMalus,能识别岩浆、仙人掌、火等危险方块(malus != 0)。这是 PR 描述"检查危险方块"的实现,方向正确
高度范围前置检查 `isWithinNavigationHeight(by, ...)
isWater() 精确匹配的版本安全性 实测确认(见 🔵 e):修掉了 WATER_BUCKET / WATER_CAULDRON / 睡莲的误判,且不影响 1.12 及更早版本
测试覆盖 新增 NavigationCorrectnessTest 覆盖高度边界(含 −65/−64/319/320 四个边界值)、哈希碰撞与开放寻址复用、acceptsNavigationSurface、平滑安全判定。都是纯逻辑测试,不依赖 Bukkit 运行时,能在 CI 跑

总结

负高度世界的边界处理(navigationMinHeight + 三处纵向扫描 + moveUp 上界)、acceptsNavigationSurface 的逻辑写反、isWater() 把水桶/炼药锅/睡莲误判为水、isInWater 把岩浆当水、碰撞箱扫掠替代固定步长采样,这些都是实打实的修复。

建议处理:

  1. 问题 1(哈希)——PR 描述说"修复节点坐标哈希"但哈希函数未改。我实测确认两类碰撞仍存在(y 只有 8 位;x 第 8 位与 z<0 标志位重叠,x=128,z=1x=0,z=-32767 相同),且探测序列周期是 2^28 而非 2^32。建议直接改 createHash(换 Long 键最干脆),或修正 PR 描述的表述。
  2. 问题 2(与 fix(bukkit): 收紧 Folia 调度与背包线程所有权 #712callRegion 冲突)——两个 PR 合并后 navigation 的 15 处 callRegion 在异步线程会抛异常,且影响普通 Paper。需要两边协调。
  3. 问题 3(支撑高度精确相等)——abs(supportY - y) > 1e-3 实际上要求脚下必须是满方块,半砖/农田/雪层全部判为不可站立,平滑可能退化为无操作。建议确认节点 y 与方块顶面的语义对应关系。
  4. 🔵 b(对角穿角采样)建议一并看一下。

说明:本次审阅未实跑 gradle 测试(含 PR 描述列出的两条命令)。以下为本机实测:createHash 的两类碰撞与单柱碰撞规模;key*31+1 探测序列周期 2^28;Spigot 1.20.4 全部含 "WATER" 的 Material 枚举及 isLegacy() 行为。其余结论基于 patch 与仓库源码推导,已逐条注明依据位置。

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