stonar96 / RayTraceAntiXray

Paper plugin for server-side async multithreaded ray tracing to hide ores that are exposed to air using Paper Anti-Xray engine-mode 1.
MIT License
128 stars 26 forks source link

"chunk" in CachedSectionBlockOcclusionGetter could possibly throw NPE? #10

Closed TauCubed closed 1 year ago

TauCubed commented 1 year ago

Wouldn't it be possible for "chunk" to be null here after the null check? If so chunk.getMinSection() and chunk.getMaxSection() will throw an NPE (Assuming 2 or more worker threads)

Perhaps create a local chunk variable so if the "this.chunk" variable becomes null it won't affect it.

                    LevelChunk chunk = this.chunk;
                    if (chunk == null) {
                        // section = null;
                        return UNLOADED_OCCLUDING;
                    }

                    if (sectionY < chunk.getMinSection() || sectionY >= chunk.getMaxSection()) {
                        section = null;
                        return false;
                    }

There are also a few more instances of this in that class.

stonar96 commented 1 year ago

Each player has its own RayTraceCallable and a player is never ticked by two or multiple threads simultaneously. That's a contract of the RayTraceCallable class, which it is optimized for (at least everything in the constructor relies on this fact to reuse the BlockOcclusionGetter, the BlockIterator and even the MutableLongWrapper). Everything else is designed thread safe if provided with a stateless BlockOcclusionGetter and a new BlockIterator instance per call. That's why I have separated this logic from the otherwise thread safe BlockOcclusionCulling class.