feat: add Raytrace Entity Tracker (WIP, experimental from Leaf PR #309)#3
Merged
BadgersMC merged 16 commits intoJul 1, 2026
Merged
Conversation
Port experimental raytrace-based entity culling from Leaf PR #309. Uses async ray-tracing to untrack entities players cannot see, reducing bandwidth and providing Anti-ESP. Combines: - tr7zw/EntityCulling (custom license) - LogisticsCraft/OcclusionCulling (MIT) STATUS: Patch copied but NOT yet applied — blob hashes need regeneration via rebuildPatches workflow due to MC version difference (Leaf 1.21.8 → Fuji 1.21.11). Known issues documented in PR body and session 20260628_201302_c88900bb audit. Draft PR — DO NOT MERGE.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a raytrace-based entity culling system with occlusion contracts, cache and visibility logic, per-player task scheduling, Minecraft patch wiring, configuration, tests, and supporting docs/license/state files. ChangesRaytrace Entity Tracker (Occlusion Culling)
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
added 10 commits
June 29, 2026 22:04
…lization - docs/tech-stack.md: Fuji fork stack (Java 21, Paperweight, MC 1.21.11) - docs/requirements.md: 8 EARS-validated REQs from code audit - docs/implementation.md: architecture plan (dirty flag, AtomicReference, records) - docs/tasks.md: 10 tasks (7 TDD, 1 DOC, 2 INFRA) mapped to REQs
… dirty flag REQ-001: Unbounded CompletableFuture.runAsync() on every block change replaced with lock-free AtomicBoolean dirty flag + periodic CAS sweep. Changes: - CullTask.java: add AtomicBoolean dirty field, replace onBlockChange body (set flag per-nearby-player, zero thread spawning) - CullTask.java run(): check dirty.compareAndSet(true, false) alongside camera-move check, coalescing N bursts into 1 cache reset per tick - build.gradle.kts: add leaf-server/src/test/java as test source dir - DirtyFlagDesignTest.java: JUnit 5 design contract tests (concurrent safety, burst coalescing, idle behavior, periodic cycle) SPEAR: prove→engine→arch→refine ✓
REQ-002: Player.cullTask plain field replaced with AtomicReference<CullTask> for safe cross-thread visibility with Fuji's multithreaded tracker (patch 0293). Changes: - Patch 0350 Player.java: field type changed, lifecycle + remove() use .get()/.set() - Patch 0350 Entity.java: setCulled()/isCulled() read via .get() - CullTask.java onBlockChange: read via .get() SPEAR: prove→engine→arch→refine ✓
Removed `|| true` from `CullTask.cullEntities()` condition that made `isForcedVisible()` grace period always bypassed. The 1-second grace period now correctly prevents flickering when entities are unculled. - CullTask.java line 122: `!isForcedVisible() || true` → `!isForcedVisible()` - Added IsForcedVisibleProof.java design contract test - Proof confirms: buggy condition always true, fix respects grace period
Standard ray-AABB slab method: tmax < 0 means entire box is behind ray origin. The old `tmax > 0` condition was inverted — it rejected ALL boxes in front of the viewer (false negatives) while simultaneously allowing behind-box false positives when tmax was negative and the slab-overlap check passed. - OcclusionCullingInstance.rayIntersection(): tmax > 0 → tmax < 0 - Added RayAabbProof.java design contract (7 assertions) - Proven: front hit, behind miss, inside-box, off-axis, grazing, opposite
Clamp out-of-range config values with warning logs: - traceInterval < 0 → clamp to 0 - maxTraceDistance <= 0 → clamp to 1 - boundingBoxLimit < 0 → clamp to 0 - boundingBoxExpansion < 0 → clamp to 0 Without validation, negative/zero values pass through to runtime causing crashes (ScheduledExecutor), NaN risks, and false occlusions.
CullTask.isSkippableArmorstand() used isInvisible() (potion effect) instead of isMarker() (NBT tag). This caused: - Invisible non-marker armor stands (potion) wrongly skipped - Marker armor stands without invisibility potion NOT skipped Config key skipMarkerArmorStand already correctly named — no rename needed.
…Trace→LOGGER.warn Replaced unbounded catch(Throwable)+printStackTrace() in isAABBVisible() with catch(Exception)+rate-limited LOGGER.warn() using 5s cooldown per exception type via HashMap<String, Long>. Three fixes in one: 1. Throwable → Exception: Errors (OOM, StackOverflow) pass through 2. printStackTrace() → SLF4J Logger.warn: structured logging 3. Rate-limited: same exception type logged at most once per 5s
Fetched from upstream tr7zw/EntityCulling repository. Saved as leaf-server/LICENSE-EntityCulling.txt.
Replaced mutable class with `record Vec3d(double x, double y, double z)`. All mutation methods now return new instances: - div() — returns new Vec3d (was mutating this) - normalize() — returns new Vec3d (was mutating this) - set() → constructor call (immutable) - setAdd() → add() factory (immutable) Updated 3 files: - Vec3d.java: 86 lines → 23 lines (record + 3 methods) - OcclusionCullingInstance.java: 14 setAdd→add, 1 set→new, 24 field accesses→x()/y()/z() - CullTask.java: 3 set→new Vec3d, 3 field accesses→accessor methods Records eliminate shared-state corruption risk from div/normalize mutating targetPoints array elements in place across ray iterations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/implementation.md (1)
63-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick windocs/implementation.md:64 — Invalid Java record syntax and fictitious architecture claims.
Line 64:
record Vec3d(double x, y, z)is invalid Java; each component requires a type. The actual Vec3d.java usesrecord Vec3d(double x, double y, double z).Lines 65-66 claim
sealed interface EntityTraitandConcurrentHashMap for OcclusionCacheas completed modern Java upgrades. Neither exists in any provided cohort file, commit summary, or task list. Remove aspirational items or mark as future work; don't document them as implemented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/implementation.md` around lines 63 - 66, Update the implementation docs to match the actual code: fix the Vec3d record syntax reference in docs/implementation.md to use the real Vec3d declaration from Vec3d.java, and remove or clearly label as future work the unsupported claims about sealed interface EntityTrait and ConcurrentHashMap for OcclusionCache. Keep the bullet list aligned with what is actually implemented in the documented classes and avoid presenting aspirational architecture changes as completed.leaf-server/src/test/java/dev/tr7zw/entityculling/ArmorStandCheckProof.java (1)
11-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMirrors
CullTask.isSkippableArmorstandinstead of calling it.
buggy()/fixed()(lines 25-31) reimplement the guard locally rather than invoking the realisSkippableArmorstand(Entity)inCullTask.java:203-206. Reasonable for now since the MC patch isn't applied/compilable yet, but flag as tech debt: once 0350 lands, replace this with a real call againstCullTask/a mockArmorStandso the proof tracks the actual implementation, not a hand-copied mirror. Also not picked up by./gradlew test(no@Test).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leaf-server/src/test/java/dev/tr7zw/entityculling/ArmorStandCheckProof.java` around lines 11 - 65, The proof test in ArmorStandCheckProof is duplicating the skippable-armor-stand logic instead of exercising the real implementation, so update the helper methods to call CullTask.isSkippableArmorstand(Entity) once that code is available. Keep ArmorStandCheckProof aligned with the actual CullTask guard by replacing the local buggy/fixed mirror with a real invocation (or a test double/mock if needed), and make the test runnable through the normal test suite by turning the proof into a proper `@Test-based` check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tech-stack.md`:
- Around line 8-9: The tech stack docs list an impossible Gradle version, so
update the Gradle entry in docs/tech-stack.md to the actual version used by the
project. Verify the version from the build configuration or wrapper settings,
and replace “Gradle 9.4.1” with that real version while keeping the existing
GRADLE_OPTS and Paperweight Patcher notes unchanged.
In `@leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch`:
- Around line 60-69: The visibility state in the entity tracker is read from an
async culling path but written in setTimeout(), so the plain lastTime field in
this class can be stale across threads. Update the state in the same class that
defines setTimeout() and isForcedVisible() so lastTime is safely published,
either by making it volatile or by switching to an AtomicLong and using it
consistently in both methods.
- Around line 73-76: The task reference is being published too late, so
Entity.setCulled() can read null or an outdated CullTask while newTask.setup()
is already running. In the setup path that creates and starts the culling task,
assign player.cullTask to the new task before calling setup()/starting async
work, and make sure any stop/cleanup path on CullTask cannot write state back
into a replacement task after it has been superseded.
In
`@leaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.java`:
- Around line 74-76: The cache in OcclusionCullingInstance is reused across
different viewer block positions, which causes stale relative cell entries to be
interpreted against a new cameraPos. Update the viewer-position handling in the
method that assigns cameraPos to track the last floored block position and call
cache.resetCache() whenever the floored viewer block changes before continuing
with getCacheValue()/cacheResult() usage.
- Around line 30-32: The cooldown tracker in OcclusionCullingInstance is not
thread-safe because lastErrorLogged is a static HashMap updated from the async
failure path. Replace it with a ConcurrentHashMap and change the
error-throttling logic in the catch block around shouldLogError so the
check-and-update happens atomically. Make sure the cooldown behavior stays the
same while preventing races when multiple exceptions hit at once.
- Around line 1-15: The OcclusionCullingInstance source is still carrying diff
markers like N|N| before the actual Java tokens, which breaks parsing. Remove
the generated line prefixes from the top of the file so the class starts with
the normal package declaration and imports, and ensure the entire file content
is clean Java source before merge.
- Around line 392-398: The visibility memoization in OcclusionCullingInstance is
reusing the same cache for ray traversal/block opacity and target-visibility
results, so cached false/true from entity checks can corrupt later stepRay
decisions. Update cacheResult and the related call sites around stepRay, the
AABB scan, and the target-handling paths to use separate caches or distinct
state values for traversal versus target memoization. Make sure
OcclusionCullingInstance no longer treats a previously cached target result as
an occluding block, and vice versa, especially in the logic that checks
targets[0] and the surrounding visibility loops.
In
`@leaf-server/src/main/java/com/logisticscraft/occlusionculling/util/MathUtilities.java`:
- Around line 16-18: The fastFloor() implementation in MathUtilities is
incorrect for values below -1024 because the current offset-and-cast approach
truncates toward zero. Fix it by either removing fastFloor() and using the
existing floor() logic everywhere, or rewriting fastFloor() to correctly handle
all negative inputs without relying on the unsafe offset trick. Make sure any
callers of MathUtilities.fastFloor continue to get mathematically correct
flooring for world-coordinate indexing.
In
`@leaf-server/src/main/java/com/logisticscraft/occlusionculling/util/Vec3d.java`:
- Around line 19-21: `Vec3d.normalize()` currently divides by the vector
magnitude without handling the zero-length case, which can produce NaNs that
later affect `OcclusionCullingInstance.isVisible()` and `rayIntersection()`.
Update `normalize()` to guard against a zero or near-zero magnitude before
division, and return a defined zero vector or otherwise short-circuit safely so
the method never emits NaN values.
In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java`:
- Around line 122-155: The forced-visibility branch in CullTask is leaving stale
per-player cull state behind because the whole visibility update is skipped when
cullable.isForcedVisible() is true. Update the CullTask loop so forced-visible
entities are handled as an early branch that immediately calls
cullable.setCulled(false, player) and continues, rather than bypassing the
culling state refresh. Keep the change local to the visibility update logic
around cullable, setTimeout, and setCulled so the player-specific hidden state
is always cleared during the grace period.
In
`@leaf-server/src/main/java/dev/tr7zw/entityculling/DefaultChunkDataProvider.java`:
- Around line 31-37: The opacity test in DefaultChunkDataProvider.isOpaque is
inverted, causing full cubes to be treated as transparent and breaking
OcclusionCullingInstance visibility checks. Update the logic in the isOpaque
method so it returns true for blocks that should occlude sight (including normal
solid full cubes), and make the outside-build-height branch return false rather
than using VOID_AIR state checks. Keep the fix localized to
DefaultChunkDataProvider and preserve the existing access.getBlockState(pos)
path for in-bounds positions.
In
`@leaf-server/src/main/java/org/dreeam/leaf/config/modules/misc/RaytraceTracker.java`:
- Around line 124-132: The namespace handling in RaytraceTracker’s
skipped-entities loop is forcing every entry through DEFAULT_PREFIX, which
breaks already-qualified IDs like modid:entity. Update the typeId construction
so only bare names are prefixed with minecraft:, and preserve any value that
already contains a namespace separator before calling EntityType.byString and
setting skipRaytraceCheck.
- Around line 80-83: The trace-interval validation in RaytraceTracker should not
clamp negative values to 0, since zero is also invalid for the async culling
loop. Update the traceInterval handling in the config parsing logic to enforce a
minimum of 1 instead, matching the positive-bound validation already used for
maxTraceDistance, and keep the warning message consistent with the new lower
bound.
In
`@leaf-server/src/test/java/com/logisticscraft/occlusionculling/ErrorLoggingProof.java`:
- Around line 11-94: The ErrorLoggingProof class is not executed by the test
runner because it only defines main() and no JUnit test methods. Convert the
proof logic into a real `@Test-based` regression test (or otherwise hook it into
CI) so Gradle test discovery will run it, and ensure the assertions exercise the
OcclusionCullingInstance.isAABBVisible catch block rather than just documenting
it. Keep the existing rate-limited logging checks, but move them into a
discoverable test method with the relevant helper classes and symbols preserved
for easy location.
In
`@leaf-server/src/test/java/com/logisticscraft/occlusionculling/RayAabbProof.java`:
- Around line 11-101: The proof class relies on a standalone main() and a copied
ray-box intersection mirror, so it can pass without exercising production code.
Convert RayAabbProof into a real test that the test runner discovers, and
replace the local buggy()/fixed() copy with assertions against the actual
OcclusionCullingInstance.rayIntersection behavior (or a test-accessible
wrapper/helper), using the existing Vec3d and box scenarios to validate the real
implementation instead of the duplicated logic.
In
`@leaf-server/src/test/java/com/logisticscraft/occlusionculling/util/Vec3dRecordProof.java`:
- Around line 11-61: Replace the hand-rolled local Vec3d record in
Vec3dRecordProof with the real com.logisticscraft.occlusionculling.util.Vec3d so
the proof validates production behavior instead of a copied implementation. Keep
the existing checks around add, div, and normalize, but point them at the
imported Vec3d type and its methods; if needed, adapt the test to whatever
constructors or factory methods the real record exposes. Also make the proof
discoverable by the test runner by converting the current main-based harness
into a proper test method/class that the framework will execute automatically.
In
`@leaf-server/src/test/java/dev/tr7zw/entityculling/CullTaskReferenceSafetyTest.java`:
- Around line 74-99: The test plainFieldBugDemonstration does not actually
exercise a plain field, so it fails to demonstrate the intended stale-read bug.
Update this test to either use a real plain field path with no
volatile/AtomicReference and compare it against the AtomicReference behavior, or
remove/rename it if crossThreadVisibility already covers the fixed behavior.
Make the change in plainFieldBugDemonstration so the writer/reader logic
reflects the bug description instead of only using AtomicReference.
- Around line 18-72: This test only validates AtomicReference semantics and does
not cover the real Player.cullTask integration, so replace it with an
integration-style test that exercises the actual Player class and its cullTask
field. Update CullTaskReferenceSafetyTest to create/use a real Player instance,
verify the set/clear/recreate behavior through the production API or field
access, and remove the MockCullTask-only assertions once the patch is
regenerated. Keep the test focused on the concurrency behavior of the real
wiring rather than re-proving java.util.concurrent.atomic.AtomicReference
guarantees.
In `@leaf-server/src/test/java/dev/tr7zw/entityculling/DirtyFlagDesignTest.java`:
- Around line 19-110: The test class currently verifies AtomicBoolean behavior
in isolation instead of the actual CullTask flow, so it won’t catch regressions
in the wiring. Update DirtyFlagDesignTest to exercise CullTask’s onBlockChange()
and run() behavior directly, using the dirty flag transition that run()
consumes, so the assertions validate the real producer/consumer path rather than
re-implementing compareAndSet semantics.
In `@leaf-server/src/test/java/dev/tr7zw/entityculling/IsForcedVisibleProof.java`:
- Around line 12-96: Convert IsForcedVisibleProof from a main-only proof into a
real JUnit test so it is discovered by ./gradlew test; the current checks in
main(), including check(), buggyCondition(), correctCondition(), and
GracePeriodTracker, are never executed by the test runner. Move the assertions
into a `@Test` method (or equivalent JUnit structure) and keep the same
grace-period and dead-code verification logic so the file actually contributes
automated test coverage.
In
`@leaf-server/src/test/java/org/dreeam/leaf/config/modules/misc/ConfigBoundsProof.java`:
- Around line 12-99: This proof class is duplicating the clamp behavior instead
of testing the real logic, so update ConfigBoundsProof to exercise
RaytraceTracker.onLoaded through a fake config source rather than
ValidatedConfig.load. Use the RaytraceTracker symbols and its existing clamp
rules as the target under test, and remove the hand-copied validation path so
the test fails if production logic drifts. Also convert this into a real JUnit
test by adding the proper test annotation/structure so it runs under the test
task.
---
Outside diff comments:
In `@docs/implementation.md`:
- Around line 63-66: Update the implementation docs to match the actual code:
fix the Vec3d record syntax reference in docs/implementation.md to use the real
Vec3d declaration from Vec3d.java, and remove or clearly label as future work
the unsupported claims about sealed interface EntityTrait and ConcurrentHashMap
for OcclusionCache. Keep the bullet list aligned with what is actually
implemented in the documented classes and avoid presenting aspirational
architecture changes as completed.
In `@leaf-server/src/test/java/dev/tr7zw/entityculling/ArmorStandCheckProof.java`:
- Around line 11-65: The proof test in ArmorStandCheckProof is duplicating the
skippable-armor-stand logic instead of exercising the real implementation, so
update the helper methods to call CullTask.isSkippableArmorstand(Entity) once
that code is available. Keep ArmorStandCheckProof aligned with the actual
CullTask guard by replacing the local buggy/fixed mirror with a real invocation
(or a test double/mock if needed), and make the test runnable through the normal
test suite by turning the proof into a proper `@Test-based` check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99b35c16-4751-4b15-bcab-67e5b59c8513
📒 Files selected for processing (25)
.claude/spear-state.jsondocs/implementation.mddocs/requirements.mddocs/tasks.mddocs/tech-stack.mdleaf-server/LICENSE-EntityCulling.txtleaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patchleaf-server/src/main/java/com/logisticscraft/occlusionculling/DataProvider.javaleaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.javaleaf-server/src/main/java/com/logisticscraft/occlusionculling/cache/ArrayOcclusionCache.javaleaf-server/src/main/java/com/logisticscraft/occlusionculling/cache/OcclusionCache.javaleaf-server/src/main/java/com/logisticscraft/occlusionculling/util/MathUtilities.javaleaf-server/src/main/java/com/logisticscraft/occlusionculling/util/Vec3d.javaleaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.javaleaf-server/src/main/java/dev/tr7zw/entityculling/DefaultChunkDataProvider.javaleaf-server/src/main/java/dev/tr7zw/entityculling/versionless/access/Cullable.javaleaf-server/src/main/java/org/dreeam/leaf/config/modules/misc/RaytraceTracker.javaleaf-server/src/test/java/com/logisticscraft/occlusionculling/ErrorLoggingProof.javaleaf-server/src/test/java/com/logisticscraft/occlusionculling/RayAabbProof.javaleaf-server/src/test/java/com/logisticscraft/occlusionculling/util/Vec3dRecordProof.javaleaf-server/src/test/java/dev/tr7zw/entityculling/ArmorStandCheckProof.javaleaf-server/src/test/java/dev/tr7zw/entityculling/CullTaskReferenceSafetyTest.javaleaf-server/src/test/java/dev/tr7zw/entityculling/DirtyFlagDesignTest.javaleaf-server/src/test/java/dev/tr7zw/entityculling/IsForcedVisibleProof.javaleaf-server/src/test/java/org/dreeam/leaf/config/modules/misc/ConfigBoundsProof.java
Comment on lines
+8
to
+9
| - **Gradle 9.4.1** (`GRADLE_OPTS=-Xmx6g`) | ||
| - **Paperweight Patcher** (papermill v2) — applies patches to decompiled MC source |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
docs/tech-stack.md:8 — Gradle 9.4.1 does not exist.
Latest Gradle stable is 8.x (8.5 as of late 2023). There is no Gradle 9.4.1. Fix to actual version used (likely 8.4 or 8.5).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tech-stack.md` around lines 8 - 9, The tech stack docs list an
impossible Gradle version, so update the Gradle entry in docs/tech-stack.md to
the actual version used by the project. Verify the version from the build
configuration or wrapper settings, and replace “Gradle 9.4.1” with that real
version while keeping the existing GRADLE_OPTS and Paperweight Patcher notes
unchanged.
Comment on lines
+18
to
+72
| class CullTaskReferenceSafetyTest { | ||
|
|
||
| static class MockCullTask { | ||
| final int id; | ||
| MockCullTask(int id) { this.id = id; } | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("AtomicReference provides safe cross-thread null→value transition") | ||
| void crossThreadVisibility() throws Exception { | ||
| AtomicReference<MockCullTask> ref = new AtomicReference<>(null); | ||
|
|
||
| // Writer thread: sets the reference | ||
| Thread writer = new Thread(() -> { | ||
| ref.set(new MockCullTask(42)); | ||
| }); | ||
| writer.start(); | ||
| writer.join(); | ||
|
|
||
| // Reader: must see the write (happens-before via AtomicReference) | ||
| MockCullTask read = ref.get(); | ||
| assertNotNull(read, "REQ-002: Reader must see writer's set — no stale null"); | ||
| assertEquals(42, read.id, "REQ-002: Reader must see fully-constructed object"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("AtomicReference.compareAndSet prevents torn initialization") | ||
| void noTornInitialization() { | ||
| AtomicReference<MockCullTask> ref = new AtomicReference<>(null); | ||
|
|
||
| // Simulate set-once lifecycle (CullTask created once, cleared on remove) | ||
| boolean set = ref.compareAndSet(null, new MockCullTask(1)); | ||
| assertTrue(set, "REQ-002: CAS null→value must succeed on first creation"); | ||
|
|
||
| // Second CAS must fail — no double-init | ||
| boolean setAgain = ref.compareAndSet(null, new MockCullTask(2)); | ||
| assertFalse(setAgain, "REQ-002: CAS null→value must fail if already set"); | ||
|
|
||
| assertEquals(1, ref.get().id, "REQ-002: Original value preserved"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Clear-to-null allows re-creation (player relog)") | ||
| void clearAndRecreate() { | ||
| AtomicReference<MockCullTask> ref = new AtomicReference<>(new MockCullTask(1)); | ||
|
|
||
| // Player removes → clear reference | ||
| ref.set(null); | ||
| assertNull(ref.get(), "REQ-002: Clear must release reference"); | ||
|
|
||
| // Player relogs → new CullTask created | ||
| boolean set = ref.compareAndSet(null, new MockCullTask(2)); | ||
| assertTrue(set, "REQ-002: CAS null→value must work after clear"); | ||
| assertEquals(2, ref.get().id); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Tests JDK semantics, not the actual Player.cullTask integration.
MockCullTask/AtomicReference here just re-verify java.util.concurrent.atomic.AtomicReference's documented happens-before guarantees (lines 28-41, 46-57, 62-72) — guaranteed by the JLS/JDK, not something this PR needs to prove. It doesn't touch the real Player class or its cullTask field at all, so it gives no actual regression coverage for the patch wiring once applied. Understandable as a placeholder while the patch isn't compilable yet, but track this as a TODO to replace with an integration test against the real field post-patch-regeneration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@leaf-server/src/test/java/dev/tr7zw/entityculling/CullTaskReferenceSafetyTest.java`
around lines 18 - 72, This test only validates AtomicReference semantics and
does not cover the real Player.cullTask integration, so replace it with an
integration-style test that exercises the actual Player class and its cullTask
field. Update CullTaskReferenceSafetyTest to create/use a real Player instance,
verify the set/clear/recreate behavior through the production API or field
access, and remove the MockCullTask-only assertions once the patch is
regenerated. Keep the test focused on the concurrency behavior of the real
wiring rather than re-proving java.util.concurrent.atomic.AtomicReference
guarantees.
Comment on lines
+74
to
+99
| @Test | ||
| @DisplayName("Plain field reproduces the bug: stale read from other thread") | ||
| void plainFieldBugDemonstration() throws Exception { | ||
| // This simulates the current BUG: plain field, no memory barrier. | ||
| // A writer thread sets the field, but reader may see stale null. | ||
| // With AtomicReference, this test would pass reliably. | ||
| // With plain field, it CAN fail (though not guaranteed on all JVMs). | ||
|
|
||
| AtomicReference<MockCullTask> fixed = new AtomicReference<>(null); | ||
|
|
||
| Thread writer = new Thread(() -> { | ||
| fixed.set(new MockCullTask(99)); // happens-before for readers | ||
| }); | ||
| writer.start(); | ||
|
|
||
| // Busy-read until we see the value — AtomicReference guarantees visibility | ||
| MockCullTask read; | ||
| long start = System.currentTimeMillis(); | ||
| do { | ||
| read = fixed.get(); | ||
| if (System.currentTimeMillis() - start > 1000) break; | ||
| } while (read == null); | ||
|
|
||
| assertNotNull(read, "REQ-002: AtomicReference guarantees reader visibility within 1s"); | ||
| assertEquals(99, read.id); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
plainFieldBugDemonstration never uses a plain field — it doesn't demonstrate the bug.
Docstring (lines 77-80) says this "simulates the current BUG: plain field, no memory barrier," but line 82 declares fixed as an AtomicReference, and the writer/reader at lines 84-95 both operate on that same AtomicReference. There is no plain-field code path anywhere in this test. As written, this is a duplicate of crossThreadVisibility with extra polling — it proves nothing about the bug it claims to demonstrate, and would pass identically whether or not Player.cullTask is ever migrated to AtomicReference.
If you want an actual bug demonstration, add a real plain-field variant (with no volatile) alongside the AtomicReference one and compare outcomes, or just rename/remove this test since crossThreadVisibility already covers the fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@leaf-server/src/test/java/dev/tr7zw/entityculling/CullTaskReferenceSafetyTest.java`
around lines 74 - 99, The test plainFieldBugDemonstration does not actually
exercise a plain field, so it fails to demonstrate the intended stale-read bug.
Update this test to either use a real plain field path with no
volatile/AtomicReference and compare it against the AtomicReference behavior, or
remove/rename it if crossThreadVisibility already covers the fixed behavior.
Make the change in plainFieldBugDemonstration so the writer/reader logic
reflects the bug description instead of only using AtomicReference.
Comment on lines
+19
to
+110
| class DirtyFlagDesignTest { | ||
|
|
||
| @Test | ||
| @DisplayName("AtomicBoolean coalesces N rapid signals into 1 action") | ||
| void dirtyFlagCoalescesBursts() { | ||
| AtomicBoolean dirty = new AtomicBoolean(false); | ||
|
|
||
| // Simulate 100 rapid block changes — only one should trigger cache reset | ||
| int resetCount = 0; | ||
| for (int i = 0; i < 100; i++) { | ||
| dirty.set(true); | ||
| } | ||
| // Periodic tick: check and CAS-reset the flag atomically | ||
| if (dirty.compareAndSet(true, false)) { | ||
| resetCount++; | ||
| } | ||
|
|
||
| assertEquals(1, resetCount, | ||
| "REQ-001: 100 rapid dirty signals must coalesce into exactly 1 cache reset. " | ||
| + "Unbounded CompletableFuture.runAsync() would spawn 100 tasks."); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Dirty flag survives concurrent writes without lost updates") | ||
| void dirtyFlagConcurrentSafety() throws Exception { | ||
| AtomicBoolean dirty = new AtomicBoolean(false); | ||
| int threadCount = 10; | ||
| int signalsPerThread = 1000; | ||
| Thread[] threads = new Thread[threadCount]; | ||
|
|
||
| // Multiple threads hammer the dirty flag concurrently | ||
| for (int i = 0; i < threadCount; i++) { | ||
| threads[i] = new Thread(() -> { | ||
| for (int j = 0; j < signalsPerThread; j++) { | ||
| dirty.set(true); | ||
| } | ||
| }); | ||
| threads[i].start(); | ||
| } | ||
| for (Thread t : threads) { | ||
| t.join(); | ||
| } | ||
|
|
||
| // After all signals, flag must be true (no lost updates from races) | ||
| assertTrue(dirty.get(), | ||
| "REQ-001: Dirty flag must be true after concurrent writes — " | ||
| + "no lost updates allowed."); | ||
|
|
||
| // CAS reset works atomically | ||
| assertTrue(dirty.compareAndSet(true, false), | ||
| "REQ-001: CAS reset must succeed when flag is true."); | ||
| assertFalse(dirty.get(), | ||
| "REQ-001: Flag must be false after CAS reset."); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Idle state: no spurious wakeups when nothing changed") | ||
| void dirtyFlagIdleWhenNoChanges() { | ||
| AtomicBoolean dirty = new AtomicBoolean(false); | ||
|
|
||
| // Periodic tick fires, but nothing was dirtied — no action taken | ||
| int idleResets = 0; | ||
| if (dirty.compareAndSet(true, false)) { | ||
| idleResets++; | ||
| } | ||
|
|
||
| assertEquals(0, idleResets, | ||
| "REQ-001: When no block changes occur, periodic tick must NOT trigger " | ||
| + "cache reset. CachedThreadPool spawns 0 tasks in idle state."); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Periodic reset pattern: set→sweep→set→sweep cycle") | ||
| void dirtyFlagPeriodicCycle() { | ||
| AtomicBoolean dirty = new AtomicBoolean(false); | ||
| int sweeps = 0; | ||
|
|
||
| // Tick 1: block change happens | ||
| dirty.set(true); | ||
| if (dirty.compareAndSet(true, false)) sweeps++; | ||
| assertEquals(1, sweeps, "Sweep 1: dirty flag was set, should sweep"); | ||
|
|
||
| // Tick 2: no changes | ||
| if (dirty.compareAndSet(true, false)) sweeps++; | ||
| assertEquals(1, sweeps, "Sweep 2: no changes, no additional sweep"); | ||
|
|
||
| // Tick 3: another block change | ||
| dirty.set(true); | ||
| if (dirty.compareAndSet(true, false)) sweeps++; | ||
| assertEquals(2, sweeps, "Sweep 3: dirty flag was set, should sweep again"); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Same gap: tests AtomicBoolean CAS semantics, not CullTask.run()/onBlockChange.
The context snippet from CullTask.java:82-109 shows the real consumer (dirty.compareAndSet(true, false) inside run()), but this test never calls into CullTask — it just re-derives that AtomicBoolean.compareAndSet works as documented (lines 23-39, 43-72, 76-88, 92-109). Useful as design documentation, but it won't catch a regression if onBlockChange()/run() wiring is broken once the patch is applied (e.g., if dirty.set(true) is accidentally dropped from the block-change hook). Flag as a follow-up once patch 0350 is regenerated against Fuji's tree.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@leaf-server/src/test/java/dev/tr7zw/entityculling/DirtyFlagDesignTest.java`
around lines 19 - 110, The test class currently verifies AtomicBoolean behavior
in isolation instead of the actual CullTask flow, so it won’t catch regressions
in the wiring. Update DirtyFlagDesignTest to exercise CullTask’s onBlockChange()
and run() behavior directly, using the dirty flag transition that run()
consumes, so the assertions validate the real producer/consumer path rather than
re-implementing compareAndSet semantics.
Comment on lines
+12
to
+96
| public class IsForcedVisibleProof { | ||
| static int failures = 0; | ||
|
|
||
| static void check(boolean cond, String msg) { | ||
| if (!cond) { | ||
| System.err.println("FAIL: " + msg); | ||
| failures++; | ||
| } | ||
| } | ||
|
|
||
| static class GracePeriodTracker { | ||
| private long lastTime = 0; | ||
|
|
||
| void setTimeout() { | ||
| this.lastTime = System.currentTimeMillis() + 1000; | ||
| } | ||
|
|
||
| boolean isForcedVisible() { | ||
| return this.lastTime > System.currentTimeMillis(); | ||
| } | ||
| } | ||
|
|
||
| static boolean buggyCondition(GracePeriodTracker tracker) { | ||
| return !tracker.isForcedVisible() || true; | ||
| } | ||
|
|
||
| static boolean correctCondition(GracePeriodTracker tracker) { | ||
| return !tracker.isForcedVisible(); | ||
| } | ||
|
|
||
| public static void main(String[] args) throws Exception { | ||
| GracePeriodTracker tracker = new GracePeriodTracker(); | ||
|
|
||
| // Test 1: Buggy condition is ALWAYS true | ||
| check(buggyCondition(tracker), | ||
| "Buggy: no setTimeout, condition must be true (|| true always wins)"); | ||
|
|
||
| tracker.setTimeout(); | ||
| Thread.sleep(1100); | ||
| check(!tracker.isForcedVisible(), | ||
| "Grace period expired after 1.1s"); | ||
| check(buggyCondition(tracker), | ||
| "Buggy: after grace expiry, condition STILL true (|| true) — BUG CONFIRMED"); | ||
|
|
||
| // Test 2: Correct condition respects grace period | ||
| GracePeriodTracker t2 = new GracePeriodTracker(); | ||
| check(correctCondition(t2), | ||
| "Correct: no grace → enters visibility check block"); | ||
| t2.setTimeout(); | ||
| check(t2.isForcedVisible(), | ||
| "After setTimeout: isForcedVisible=true within 1s"); | ||
| check(!correctCondition(t2), | ||
| "Correct: during grace → SKIPS visibility block (prevents flickering)"); | ||
| Thread.sleep(1100); | ||
| check(!t2.isForcedVisible(), "After 1.1s: isForcedVisible=false"); | ||
| check(correctCondition(t2), | ||
| "Correct: after grace expiry → enters block again"); | ||
|
|
||
| // Test 3: Grace period prevents flickering | ||
| GracePeriodTracker t3 = new GracePeriodTracker(); | ||
| t3.setTimeout(); | ||
| Thread.sleep(100); | ||
| check(t3.isForcedVisible(), "Within grace (100ms): isForcedVisible=true"); | ||
| check(!correctCondition(t3), | ||
| "Correct: within grace → skip culling, no flickering"); | ||
| check(buggyCondition(t3), | ||
| "Buggy: within grace → enters block anyway (|| true) — flickering bug"); | ||
|
|
||
| // Test 4: Multiple setTimeouts coalesce | ||
| GracePeriodTracker t4 = new GracePeriodTracker(); | ||
| t4.setTimeout(); | ||
| Thread.sleep(500); | ||
| t4.setTimeout(); | ||
| Thread.sleep(600); | ||
| check(t4.isForcedVisible(), "Extended grace still active"); | ||
| check(!correctCondition(t4), "Correct: extended grace → still skip"); | ||
|
|
||
| if (failures == 0) { | ||
| System.out.println("ALL TESTS PASSED: isForcedVisible dead code fix verified."); | ||
| } else { | ||
| System.err.println(failures + " TEST(S) FAILED"); | ||
| System.exit(1); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Same main()-only pattern — never auto-discovered as a test.
Logic itself checks out (correctly distinguishes the || true dead-code bug from the fixed grace-period condition), but like the other "Proof" files in this PR, there's no @Test annotation, so ./gradlew test runs zero assertions from this file. Consider converting to JUnit once Entity.isForcedVisible() is compilable against the applied patch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@leaf-server/src/test/java/dev/tr7zw/entityculling/IsForcedVisibleProof.java`
around lines 12 - 96, Convert IsForcedVisibleProof from a main-only proof into a
real JUnit test so it is discovered by ./gradlew test; the current checks in
main(), including check(), buggyCondition(), correctCondition(), and
GracePeriodTracker, are never executed by the test runner. Move the assertions
into a `@Test` method (or equivalent JUnit structure) and keep the same
grace-period and dead-code verification logic so the file actually contributes
automated test coverage.
Comment on lines
+12
to
+99
| public class ConfigBoundsProof { | ||
| static int failures = 0; | ||
| static final StringBuilder log = new StringBuilder(); | ||
|
|
||
| static void check(boolean cond, String msg) { | ||
| if (!cond) { System.err.println("FAIL: " + msg); failures++; } | ||
| } | ||
|
|
||
| static void warn(String msg) { | ||
| log.append("WARN: ").append(msg).append("\n"); | ||
| } | ||
|
|
||
| static class UnvalidatedConfig { | ||
| int traceInterval, maxTraceDistance, boundingBoxLimit; | ||
| double boundingBoxExpansion; | ||
|
|
||
| void load(int ti, int mtd, int bbl, double bbe) { | ||
| this.traceInterval = ti; | ||
| this.maxTraceDistance = mtd; | ||
| this.boundingBoxLimit = bbl; | ||
| this.boundingBoxExpansion = bbe; | ||
| } | ||
| } | ||
|
|
||
| static class ValidatedConfig { | ||
| int traceInterval, maxTraceDistance, boundingBoxLimit; | ||
| double boundingBoxExpansion; | ||
|
|
||
| void load(int ti, int mtd, int bbl, double bbe) { | ||
| if (ti < 0) { warn("traceInterval " + ti + " < 0, clamping to 0"); this.traceInterval = 0; } | ||
| else { this.traceInterval = ti; } | ||
| if (mtd <= 0) { warn("maxTraceDistance " + mtd + " <= 0, clamping to 1"); this.maxTraceDistance = 1; } | ||
| else { this.maxTraceDistance = mtd; } | ||
| if (bbl < 0) { warn("boundingBoxLimit " + bbl + " < 0, clamping to 0"); this.boundingBoxLimit = 0; } | ||
| else { this.boundingBoxLimit = bbl; } | ||
| if (bbe < 0) { warn("boundingBoxExpansion " + bbe + " < 0, clamping to 0"); this.boundingBoxExpansion = 0; } | ||
| else { this.boundingBoxExpansion = bbe; } | ||
| } | ||
| } | ||
|
|
||
| public static void main(String[] args) { | ||
| UnvalidatedConfig bad = new UnvalidatedConfig(); | ||
| bad.load(-50, 0, -10, -1.5); | ||
| check(bad.traceInterval == -50, | ||
| "BUG: traceInterval=-50 passed through"); | ||
| check(bad.maxTraceDistance == 0, | ||
| "BUG: maxTraceDistance=0 passed through"); | ||
| check(bad.boundingBoxLimit == -10, | ||
| "BUG: boundingBoxLimit=-10 passed through"); | ||
| check(bad.boundingBoxExpansion == -1.5, | ||
| "BUG: boundingBoxExpansion=-1.5 passed through"); | ||
|
|
||
| log.setLength(0); | ||
| ValidatedConfig good = new ValidatedConfig(); | ||
| good.load(-50, 0, -10, -1.5); | ||
| check(good.traceInterval == 0, "FIX: traceInterval clamped to 0"); | ||
| check(good.maxTraceDistance == 1, "FIX: maxTraceDistance clamped to 1"); | ||
| check(good.boundingBoxLimit == 0, "FIX: boundingBoxLimit clamped to 0"); | ||
| check(good.boundingBoxExpansion == 0.0, "FIX: boundingBoxExpansion clamped to 0.0"); | ||
| check(log.toString().contains("traceInterval"), "FIX: warning logged"); | ||
| check(log.toString().contains("maxTraceDistance"), "FIX: warning logged"); | ||
|
|
||
| log.setLength(0); | ||
| ValidatedConfig normal = new ValidatedConfig(); | ||
| normal.load(50, 64, 20, 0.5); | ||
| check(normal.traceInterval == 50, "Normal traceInterval=50 preserved"); | ||
| check(normal.maxTraceDistance == 64, "Normal maxTraceDistance=64 preserved"); | ||
| check(normal.boundingBoxLimit == 20, "Normal boundingBoxLimit=20 preserved"); | ||
| check(normal.boundingBoxExpansion == 0.5, "Normal boundingBoxExpansion=0.5 preserved"); | ||
| check(log.toString().isEmpty(), "No warnings for in-range values"); | ||
|
|
||
| log.setLength(0); | ||
| ValidatedConfig edge = new ValidatedConfig(); | ||
| edge.load(0, 1, 0, 0.0); | ||
| check(edge.traceInterval == 0, "Edge: traceInterval=0 preserved"); | ||
| check(edge.maxTraceDistance == 1, "Edge: maxTraceDistance=1 preserved"); | ||
| check(edge.boundingBoxLimit == 0, "Edge: boundingBoxLimit=0 preserved"); | ||
| check(edge.boundingBoxExpansion == 0.0, "Edge: boundingBoxExpansion=0.0 preserved"); | ||
| check(log.toString().isEmpty(), "No warnings for edge values"); | ||
|
|
||
| if (failures == 0) { | ||
| System.out.println("ALL TESTS PASSED: config bounds validation design verified."); | ||
| } else { | ||
| System.err.println(failures + " TEST(S) FAILED"); | ||
| System.exit(1); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Mirrors RaytraceTracker.onLoaded clamping logic instead of exercising it.
ValidatedConfig.load (lines 36-50) hand-copies the clamp rules from RaytraceTracker.java (max-trace-distance ≤0 → 1, bounding-box-limit <0 → 0, trace-interval <0 → 0, bounding-box-expansion <0 → 0) rather than calling RaytraceTracker.onLoaded() against a fake config source. The mirrored rules currently match the real implementation, but there's nothing forcing them to stay in sync — and again, no @Test annotation means this never runs under ./gradlew test. Same recommendation as the other proof files: convert to a real test against RaytraceTracker once feasible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@leaf-server/src/test/java/org/dreeam/leaf/config/modules/misc/ConfigBoundsProof.java`
around lines 12 - 99, This proof class is duplicating the clamp behavior instead
of testing the real logic, so update ConfigBoundsProof to exercise
RaytraceTracker.onLoaded through a fake config source rather than
ValidatedConfig.load. Use the RaytraceTracker symbols and its existing clamp
rules as the target under test, and remove the hand-copied validation path so
the test fails if production logic drifts. Also convert this into a real JUnit
test by adding the proper test annotation/structure so it runs under the test
task.
- Stale blob hashes from Leaf 1.21.8 caused 'corrupt patch at line 141' - Regenerated via diff of src/minecraft/java/ (0001-0349) vs manual 0350 apply - Also fixes b/ path in patch diff headers - Added partial series file tracking (previously untracked)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch`:
- Line 96: The cullTask field on Entity should remain a non-null, final
AtomicReference, with only its contained CullTask value being nullable. Update
the Entity-related patch so that Entity.setCulled(), Entity.isCulled(),
teardown, and CullTask.onBlockChange() can continue safely calling
player.cullTask.get() without null checks on the reference itself, and remove
the public/non-final/@Nullable contract from cullTask.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f33bea50-6e51-4367-9c61-dce27291a3f4
📒 Files selected for processing (2)
leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patchleaf-server/minecraft-patches/features/series
| private long lastSprintKnockback = -1; // Sakura - configure entity knockback | ||
|
|
||
| + // Leaf start - Raytrace Entity Tracker | ||
| + @javax.annotation.Nullable public java.util.concurrent.atomic.AtomicReference<dev.tr7zw.entityculling.CullTask> cullTask = new java.util.concurrent.atomic.AtomicReference<>(null); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch:96 — keep the AtomicReference itself non-null and final.
Entity.setCulled(), Entity.isCulled(), teardown, and CullTask.onBlockChange() all call player.cullTask.get() directly. Making the field public, non-final, and @Nullable makes that contract unsafe; only the referenced value should be nullable.
Proposed fix
- `@javax.annotation.Nullable` public java.util.concurrent.atomic.AtomicReference<dev.tr7zw.entityculling.CullTask> cullTask = new java.util.concurrent.atomic.AtomicReference<>(null);
+ public final java.util.concurrent.atomic.AtomicReference<dev.tr7zw.entityculling.CullTask> cullTask = new java.util.concurrent.atomic.AtomicReference<>(null);📝 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
| + @javax.annotation.Nullable public java.util.concurrent.atomic.AtomicReference<dev.tr7zw.entityculling.CullTask> cullTask = new java.util.concurrent.atomic.AtomicReference<>(null); | |
| public final java.util.concurrent.atomic.AtomicReference<dev.tr7zw.entityculling.CullTask> cullTask = new java.util.concurrent.atomic.AtomicReference<>(null); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch` at
line 96, The cullTask field on Entity should remain a non-null, final
AtomicReference, with only its contained CullTask value being nullable. Update
the Entity-related patch so that Entity.setCulled(), Entity.isCulled(),
teardown, and CullTask.onBlockChange() can continue safely calling
player.cullTask.get() without null checks on the reference itself, and remove
the public/non-final/@Nullable contract from cullTask.
added 2 commits
July 1, 2026 00:43
Critical:
- cr:3502224888 strip N|N| line-number prefixes from OcclusionCullingInstance
- cr:3502224921 fix inverted opacity check in DefaultChunkDataProvider
Major:
- cr:3502224877 make Entity.lastTime volatile for thread safety
- cr:3502224882 reorder: cullTask.set() before newTask.setup()
- cr:3502224890 HashMap->ConcurrentHashMap for error cooldown map
- cr:3502224910 fix forced-visible branch: early continue clears stale culled state
- cr:3502224928 clamp traceInterval to 1 (not 0)
Minor:
- cr:3502224902 fastFloor delegates to floor() (fix negative inputs)
- cr:3502224906 normalize() zero-magnitude guard
- cr:3502224933 namespace handling: indexOf(':') not startsWith('minecraft:')
Patch 0350 regenerated with volatile + setup ordering fixes.
…tifier API - OcclusionCullingInstance: fix 14 corrupted targetPoint lines - OcclusionCullingInstance: remove 'final' from targetPos (record reassign) - OcclusionCullingInstance: .getX()/.getY()/.getZ() → .x()/.y()/.z() - RaytraceTracker: ResourceLocation → Identifier (MC 1.21.11) - RaytraceTracker: EntityType.byString() → BuiltInRegistries.ENTITY_TYPE.getOptional(Identifier.parse())
Owner
|
@coderabbitai look better now? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "=== CullTask.java ==="
fd CullTask.java --exec cat -n {}
echo "=== OcclusionCullingInstance.java rayIntersection/tmax ==="
fd OcclusionCullingInstance.java --exec grep -n "tmax\|rayIntersection" -A5 -B5 {}
echo "=== RaytraceTracker.java ==="
fd RaytraceTracker.java --exec cat -n {}
echo "=== Vec3d.java ==="
fd Vec3d.java -e java --exec cat -n {}
echo "=== patch isForcedVisible/armorstand ==="
fd '0350-Raytrace-Entity-Tracker.patch' --exec grep -n "isForcedVisible\|isMarker\|isInvisible\|Throwable\|catch" -A3 -B3 {}Length of output: 20894 🏁 Script executed: #!/bin/bash
set -e
echo "=== catch blocks in OcclusionCullingInstance ==="
fd OcclusionCullingInstance.java --exec grep -n "catch\|lastErrorLogged\|ERROR_LOG_COOLDOWN" -B3 -A5 {}
echo "=== catch Throwable anywhere in leaf-server/src ==="
rg -n "catch\s*\(\s*Throwable" leaf-server/src || echo "none found"
echo "=== setTimeout/lastTime in patch ==="
fd '0350-Raytrace-Entity-Tracker.patch' --exec grep -n "setTimeout\|lastTime" -B3 -A5 {}Length of output: 4484
Fixed and confirmed:
This is a solid pass — actual production code matches what the docs/tasks.md claims, not just aspirational test proofs. Still worth double-checking before merge:
Otherwise this looks in good shape. Still needs the ✅ Action performedReview finished.
|
- CullTask.run(): add rate-limited catch (Exception) with 5s cooldown, consistent with OcclusionCullingInstance.isAABBVisible pattern. Prevents unhandled exception per tick if cullEntities() throws. - onBlockChange: add PERF comment documenting O(players) cost with distance-bounded mitigation. Reported by @BadgersMC in analysis chain review.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java (2)
174-187: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftleaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java Lines 174-187 — block updates now pay an O(realPlayers) scan on the world hot path.
Every block change walks
playerList.realPlayers, then filters by level and distance. Redstone/explosion bursts will scale with total online players on the main thread. This needs a level-local/nearby-player lookup before release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java` around lines 174 - 187, The block-update path in CullTask currently scans playerList.realPlayers for every change, which makes world hot-path work scale with total online players. Update the logic around the player iteration in CullTask so it first narrows to players in the same level and preferably uses a level-local or nearby-player lookup instead of a full realPlayers scan, while preserving the existing distance check and dirty flag behavior.
82-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winleaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java Lines 82-108 — one culling exception turns into an infinite async failure loop.
If
resetCache()orcullEntities()throws once, this pass aborts and Line 106 immediately reschedules the same task again. That leaves this player’s culled state stale and can spam the log every interval forever. CatchExceptioninsiderun()and rate-limit the warning before rescheduling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java` around lines 82 - 108, The CullTask.run() method currently lets exceptions from culling.resetCache() or cullEntities() escape, which causes the task to abort and then immediately reschedule itself forever. Update run() to catch Exception around the culling work, log a warning once per failure window, and still allow the scheduleNext resubmission path to continue without creating an infinite failure loop.
♻️ Duplicate comments (3)
leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch (2)
59-65: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftleaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch Lines 59-65, 121-137 — stale tasks can still write into the replacement task.
signalStop()only stops future scheduling. An old in-flightCullTaskcan still reachEntity.setCulled(), and Line 60 writes throughplayer.cullTask.get()at that moment. After Line 135 publishesnewTask, stale results from the old world/tick can be recorded into the replacement task. Route writes through the owningCullTaskinstance, or reject writes when the task identity no longer matches.Also applies to: 121-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch` around lines 59 - 65, Stale in-flight culling work can still write into a newly published task because Entity.setCulled() fetches player.cullTask.get() at write time instead of using the owning CullTask instance. Update the flow around signalStop(), newTask publication, and setCulled() so writes are tied to the original CullTask identity, or are ignored when the current task no longer matches the one that produced the result. Ensure the guard is applied in the write path where task.setCulled(this, value) is called.
60-72: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winleaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch Lines 95-97 — the
AtomicReferenceitself should stay non-null and final.Current callers assume
player.cullTaskis always a usable reference and call.get()directly. Leaving the field public, mutable, and@Nullablemakes that cross-file contract fragile and opens an async NPE/replacement hazard. Keep only the containedCullTasknullable.Also applies to: 95-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch` around lines 60 - 72, The `player.cullTask` reference should remain a non-null, final `AtomicReference`; only the contained `CullTask` may be nullable. Update the `CullTask` holder declaration accordingly and remove any ability to replace the reference itself, while keeping `setCulled` and `isCulled` using `.get()` on the stable reference.leaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.java (1)
93-102: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftleaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.java Lines 93-102, 391-398, 414-445, 497-505 — target visibility is still poisoning the traversal cache.
State
1/2is doing double duty for both ray-traversed block opacity and prior entity visibility results. That makes Line 95 treat a previously cached target hit as “visible now”, and Line 416 treat a previously cached hidden target cell as a solid wall for later rays. Split target memoization from block-opacity cache state, or reserve disjoint state values and branch on them separately.Also applies to: 391-398, 414-445, 497-505
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.java` around lines 93 - 102, The cache state in OcclusionCullingInstance is conflating block opacity with target visibility, so a prior entity hit can later be interpreted as a visible block or a solid wall. Update the caching in the ray traversal and visibility lookup paths to keep target memoization separate from block-opacity cache state, or use clearly disjoint state values with distinct branches. Adjust the logic in getCacheValue/setCacheValue usage and the related traversal checks so cached target visibility never poisons later occlusion tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@leaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.java`:
- Around line 153-158: The cooldown check in OcclusionCullingInstance is still
racy because the separate get/put on lastErrorLogged lets multiple threads log
the same exception at once. Update the isAABBVisible exception-handling path to
use an atomic ConcurrentHashMap operation such as compute/computeIfPresent (or
an equivalent CAS-style timestamp update) keyed by e.getClass().getSimpleName(),
and only emit LOGGER.warn when the current thread successfully wins the cooldown
window.
In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java`:
- Around line 132-136: The force-visible shortcut in CullTask does not honor
RaytraceTracker.forceVisibleRadius values of 0 or less because the check squares
the value, so negative values still enable a small radius. Update the distance
shortcut in CullTask so it only applies when RaytraceTracker.forceVisibleRadius
> 0, or normalize/clamp the value to 0 when it is loaded, keeping the existing
cullable.setCulled(false, player) path unchanged for valid positive radii.
In
`@leaf-server/src/main/java/org/dreeam/leaf/config/modules/misc/RaytraceTracker.java`:
- Around line 124-135: The skipped-entities parsing in RaytraceTracker can still
abort config loading because Identifier.parse(typeId) throws before the
unknown-entity warning path is reached. Update the loop in RaytraceTracker to
use Identifier.tryParse(typeId) instead of parse, handle null results by logging
a warning and continuing, and keep the existing BuiltInRegistries.ENTITY_TYPE
lookup and skipRaytraceCheck assignment for valid IDs.
---
Outside diff comments:
In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java`:
- Around line 174-187: The block-update path in CullTask currently scans
playerList.realPlayers for every change, which makes world hot-path work scale
with total online players. Update the logic around the player iteration in
CullTask so it first narrows to players in the same level and preferably uses a
level-local or nearby-player lookup instead of a full realPlayers scan, while
preserving the existing distance check and dirty flag behavior.
- Around line 82-108: The CullTask.run() method currently lets exceptions from
culling.resetCache() or cullEntities() escape, which causes the task to abort
and then immediately reschedule itself forever. Update run() to catch Exception
around the culling work, log a warning once per failure window, and still allow
the scheduleNext resubmission path to continue without creating an infinite
failure loop.
---
Duplicate comments:
In `@leaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patch`:
- Around line 59-65: Stale in-flight culling work can still write into a newly
published task because Entity.setCulled() fetches player.cullTask.get() at write
time instead of using the owning CullTask instance. Update the flow around
signalStop(), newTask publication, and setCulled() so writes are tied to the
original CullTask identity, or are ignored when the current task no longer
matches the one that produced the result. Ensure the guard is applied in the
write path where task.setCulled(this, value) is called.
- Around line 60-72: The `player.cullTask` reference should remain a non-null,
final `AtomicReference`; only the contained `CullTask` may be nullable. Update
the `CullTask` holder declaration accordingly and remove any ability to replace
the reference itself, while keeping `setCulled` and `isCulled` using `.get()` on
the stable reference.
In
`@leaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.java`:
- Around line 93-102: The cache state in OcclusionCullingInstance is conflating
block opacity with target visibility, so a prior entity hit can later be
interpreted as a visible block or a solid wall. Update the caching in the ray
traversal and visibility lookup paths to keep target memoization separate from
block-opacity cache state, or use clearly disjoint state values with distinct
branches. Adjust the logic in getCacheValue/setCacheValue usage and the related
traversal checks so cached target visibility never poisons later occlusion
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd3f4c24-e838-419a-bda4-39df416f5e22
📒 Files selected for processing (8)
build-data/leaf.atleaf-server/minecraft-patches/features/0350-Raytrace-Entity-Tracker.patchleaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.javaleaf-server/src/main/java/com/logisticscraft/occlusionculling/util/MathUtilities.javaleaf-server/src/main/java/com/logisticscraft/occlusionculling/util/Vec3d.javaleaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.javaleaf-server/src/main/java/dev/tr7zw/entityculling/DefaultChunkDataProvider.javaleaf-server/src/main/java/org/dreeam/leaf/config/modules/misc/RaytraceTracker.java
Comment on lines
+153
to
+158
| String key = e.getClass().getSimpleName(); | ||
| long now = System.currentTimeMillis(); | ||
| Long last = lastErrorLogged.get(key); | ||
| if (last == null || now - last >= ERROR_LOG_COOLDOWN_MS) { | ||
| lastErrorLogged.put(key, now); | ||
| LOGGER.warn("[OcclusionCulling] {} in isAABBVisible: {}", key, e.getMessage()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
leaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.java Lines 153-158 — the error cooldown check is still non-atomic.
ConcurrentHashMap avoids map corruption, but this get()/put() pair still lets concurrent failures all observe the same old timestamp and all log. Use compute/CAS-style update so only the winner emits within the cooldown window.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@leaf-server/src/main/java/com/logisticscraft/occlusionculling/OcclusionCullingInstance.java`
around lines 153 - 158, The cooldown check in OcclusionCullingInstance is still
racy because the separate get/put on lastErrorLogged lets multiple threads log
the same exception at once. Update the isAABBVisible exception-handling path to
use an atomic ConcurrentHashMap operation such as compute/computeIfPresent (or
an equivalent CAS-style timestamp update) keyed by e.getClass().getSimpleName(),
and only emit LOGGER.warn when the current thread successfully wins the cooldown
window.
Comment on lines
+132
to
+136
| final double distanceSqr = entity.position().distanceToSqr(cameraMC); | ||
| if (distanceSqr < RaytraceTracker.forceVisibleRadius * RaytraceTracker.forceVisibleRadius) { | ||
| cullable.setCulled(false, player); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java Lines 132-136 — negative forceVisibleRadius does not disable the shortcut.
RaytraceTracker documents <= 0 as disabled, but this branch squares the value. -1 becomes a 1-block forced-visible radius instead of disabling it. Guard this path with RaytraceTracker.forceVisibleRadius > 0 or clamp negatives to 0 on load.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java` around lines
132 - 136, The force-visible shortcut in CullTask does not honor
RaytraceTracker.forceVisibleRadius values of 0 or less because the check squares
the value, so negative values still enable a small radius. Update the distance
shortcut in CullTask so it only applies when RaytraceTracker.forceVisibleRadius
> 0, or normalize/clamp the value to 0 when it is loaded, keeping the existing
cullable.setCulled(false, player) path unchanged for valid positive radii.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java (2)
31-32: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCullTask.java:79 — make setup idempotence atomic before this creates duplicate scheduler chains.
volatiledoes not make the check/set atomic. Ifsetup()is called concurrently, multiple delayed executions can be started, and each one self-reschedules infinally. UseAtomicBoolean.compareAndSet(false, true)and register before scheduling.Proposed fix
- private volatile boolean isInit = false; + private final AtomicBoolean isInit = new AtomicBoolean(false); ... - if (!this.isInit) { - this.isInit = true; - } else { + if (!this.isInit.compareAndSet(false, true)) { return; } - this.worker.execute(this); tasks.add(this); + this.worker.execute(this);Also applies to: 79-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java` around lines 31 - 32, The setup idempotence in CullTask.setup() is not atomic, so concurrent calls can create duplicate scheduler chains. Replace the volatile isInit guard with an AtomicBoolean and use compareAndSet(false, true) before creating the delayed task, ensuring only one scheduler chain is registered. Keep the existing scheduling and self-rescheduling flow in CullTask, but make the registration step happen only after the atomic claim succeeds.
177-206: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftCullTask.java:191 — dirty coalescing does not protect the block-event hot path.
This still scans every real player for every block change before coalescing. Explosions, fluids, redstone, or chunk updates can turn into
changedBlocks × playersmain-thread work. Move block changes into a bounded per-level queue/dirty-region set and let each task consume it on its scheduled interval, or use a spatial player lookup instead ofplayerList.realPlayers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java` around lines 177 - 206, The onBlockChange method in CullTask still does an O(players) scan on every block event, so the dirty flag only coalesces task work, not the hot path. Change onBlockChange to enqueue block changes into a bounded per-level structure or use a spatial player lookup, and have each CullTask consume and clear that state during its scheduled tick instead of iterating playerList.realPlayers per event. Keep the distance filtering and dirty signaling inside CullTask, but remove the main-thread per-block player loop from onBlockChange.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java`:
- Around line 111-118: The cooldown check in CullTask’s exception handler is
racy because the separate get and put on lastErrorLogged can let concurrent
tasks all log at once. Update the catch block in CullTask.run to reserve the
cooldown atomically using compute on lastErrorLogged, and make the warning
include the exception object so the first logged failure has a stack trace. Keep
the existing ERROR_LOG_COOLDOWN_MS behavior and use the key derived from
e.getClass().getSimpleName() to preserve per-exception throttling.
---
Outside diff comments:
In `@leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java`:
- Around line 31-32: The setup idempotence in CullTask.setup() is not atomic, so
concurrent calls can create duplicate scheduler chains. Replace the volatile
isInit guard with an AtomicBoolean and use compareAndSet(false, true) before
creating the delayed task, ensuring only one scheduler chain is registered. Keep
the existing scheduling and self-rescheduling flow in CullTask, but make the
registration step happen only after the atomic claim succeeds.
- Around line 177-206: The onBlockChange method in CullTask still does an
O(players) scan on every block event, so the dirty flag only coalesces task
work, not the hot path. Change onBlockChange to enqueue block changes into a
bounded per-level structure or use a spatial player lookup, and have each
CullTask consume and clear that state during its scheduled tick instead of
iterating playerList.realPlayers per event. Keep the distance filtering and
dirty signaling inside CullTask, but remove the main-thread per-block player
loop from onBlockChange.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd5acdfd-e016-4b26-85be-10b9c1aed586
📒 Files selected for processing (1)
leaf-server/src/main/java/dev/tr7zw/entityculling/CullTask.java
- CullTask.run(): use ConcurrentHashMap.compute() for atomic error-log cooldown instead of racy get/put pair. Pass exception object for stack trace in warn log. (#3502775302) - RaytraceTracker.onPostLoaded(): use Identifier.tryParse() instead of parse() to avoid InvalidIdentifierException aborting config load on malformed skipped-entities entries. Add explicit null check with 'Invalid entity ID' warning. Rename 'Skip unknown' to 'Unknown entity' for clarity. (#3502736965)
Owner
|
@coderabbitai ow about now. ready to merge? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "=== run() exception handling in CullTask ==="
fd CullTask.java --exec grep -n "cullEntities\|catch\|void run" -A5 -B2 {}
echo "=== onBlockChange loop ==="
fd CullTask.java --exec grep -n "onBlockChange" -A20 {}
echo "=== check patch regeneration / blob hashes note ==="
fd '0350-Raytrace-Entity-Tracker.patch' --exec grep -n "index \|@@" {} | head -5
echo "=== tasks.md status ==="
fd tasks.md --exec cat {}Length of output: 12550
Not ready to merge yet though — TDD-010 is still unchecked in Once TDD-010 is done and you've confirmed the Sakura patches (0342-0346) still apply cleanly on top, ping me again and I'll do a final pass. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "=== onBlockChange current ==="
fd CullTask.java --exec grep -n "onBlockChange" -A20 {}
echo "=== run() exception handling ==="
fd CullTask.java --exec grep -n "public synchronized void run" -A40 {}
echo "=== patch application status / blob hashes ==="
fd '0350-Raytrace-Entity-Tracker.patch' --exec head -30 {}
echo "=== check for rebuildPatches evidence / applied patch dir ==="
fd -t d "leaf-server" -d 1
git log --oneline -5
git status
</bash>Length of output: 6848 🏁 Script executed: #!/bin/bash
set -e
gh pr view 3 --repo BadgersMC/Fuji --json title,body,isDraft,mergeable,mergeStateStatus,labels
echo "=== check hash validation script ==="
fd -e sh -e py "hash"
rg -n "blob hash\|hashCheck\|sha1" leaf-server -i | head -30
</bash>Length of output: 378 🏁 Script executed: #!/bin/bash
timeout 60 gh pr view 3 --repo BadgersMC/Fuji --json title,isDraft,mergeable,mergeStateStatus 2>&1 || echo "gh failed"
</bash>Length of output: 580 🏁 Script executed: #!/bin/bash
gh pr view 3 --repo BadgersMC/Fuji --json title,isDraft,mergeable,mergeStateStatusLength of output: 387 🏁 Script executed: #!/bin/bash
gh pr checks 3 --repo BadgersMC/Fuji 2>&1 | head -50Length of output: 331
Not ready to merge yet though, for two reasons that aren't code-quality issues:
Ping me once the build check passes and I'll take another pass — happy to sign off at that point given the code itself checks out. ✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Port experimental raytrace-based entity culling from Leaf PR #309. Uses async ray-tracing to untrack entities players cannot see, reducing bandwidth usage and providing Anti-ESP.
Combines two libraries:
Status: WIP — Patch NOT Yet Applied
The
.patchfile and Java sources are committed as-is. The patch has NOT been applied to the decompiled MC source yet — blob hashes mismatch because the original was generated against Leaf 1.21.8 (Fuji is 1.21.11). Next step:rebuildPatchesworkflow per the paper-fork-patch-development skill.Known Issues (from code audit)
🔴 CRITICAL
onBlockChangespawns unbounded async tasks — every block change triggersCompletableFuture.runAsync()on a cached thread pool, iterating all online players. Cannon TPS will tank.player.cullTasknot volatile — race condition with Fuji's multithreaded tracker (patch 0293). Stale reads can causeisCulled()to return wrong value.🔴 HIGH
isForcedVisible() || true— debug remnant defeats forced-visible optimization. Entities flicker at corners.rayIntersectionAABB test appears inverted —tmax > 0returns false when it should return true. Redundant ray marches.🟠 MEDIUM
Config values completely unvalidated — traceInterval=0 would cause infinite loop, negative maxTraceDistance = undefined behavior.
skipMarkerArmorStandchecksisInvisible(), notisMarker()— config name lies about what it does.Silent
Throwableswallowing inisAABBVisible— catches Throwable, prints stacktrace with no rate limiting. Log spam at 20 traces/sec/player.🟡 LOW
License compliance — EntityCulling "Custom License" not included.
Mutable
Vec3dreuse — works for single-threaded CullTask but is a footgun.Planning: Stabilization with Modern Java
See PR discussion — primary goals:
CachedThreadPoolfirehose with bounded work queue + rate limitingvolatile/AtomicReferencefor thread safety with MT tracker|| true, inverted ray check)@Rangeor bounds checkThrowablecatch with Logger + rate-limited warningRecordfor immutable vectors,sealedinterfaces for entity filteringVirtualThreadsvs dedicated scheduler for async tracesTest Plan
applyAllPatchessucceeds with regenerated blob hashesjarbuilds successfullyDraft PR — DO NOT MERGE. Work in progress. See session audit for detailed per-file analysis.
Fixes
RaytraceTrackerconfig bounds and apply skip/marker rules to targetEntityTypes.onBlockChangework with a bounded dirty-flag cache reset (instead of spawning one async task per block event).CullTaskin anAtomicReference, with safe stop/clear and concurrent entity-removal queueing.ArmorStand.isMarker().Exception(notThrowable) and make occlusion vectors immutable (Vec3drecord).Features
RaytraceTrackerconfig module to enable raytrace entity tracking with interval/distance limits, forced-visible radius, and skip-entity rules.OcclusionCullingInstanceusingOcclusionCache/ArrayOcclusionCacheandDataProvider/DefaultChunkDataProvider.CullTask+Cullablewith forced-visible/timeout and per-playerisCulledqueries to suppress culled entity tracking.ChunkMaptracking onentity.isCulled(player).tr7zw/EntityCullinglicense.