search

rvandoosselaer / Blocks

A block (voxel) engine for jMonkeyEngine
BSD 3-Clause "New" or "Revised" License
41 stars 14 forks source link

Missing Getter + Fixed version of Caffeine #63

Closed vxel closed 2 years ago

vxel commented 3 years ago

Regarding issue #24, many NPE are still occurring (despite previous fix) because parallel threads continue processing after a chunk is removed (e.g. the thread running FacesMeshGenerator.createAndSetNodeAndCollisionMesh). This PR adds checks so that those threads can terminate gracefully. This PR also adds a missing Getter on PhysicsChunkPagerState.

rvandoosselaer commented 3 years ago

Thanks, I’ll have a look at it. 👍

vxel commented 3 years ago

BTW, the build is failing because of caffeine 3.0.1 now requiring JDK 11... (?!) Downgrading version to caffeineVersion = "2.8.8" will solve the issue.

rvandoosselaer commented 3 years ago

Sorry for my late reply on this PR.

I have no issues with the added @Getter for the PhysicsChunkPager.

I'm not convinced that the other 'fixes' are correct. The fact that the FacesMeshGenerator is throwing an error, is in my opinion correct behaviour in this case. The chunk was removed from the cache, so the mesh generation should actually stop as soon as possible, otherwise you are waisting resources. Why do you want to continue processing a mesh for a chunk that will never be retrievable? I do agree that a NPE is probably not the correct exception to throw though.

vxel commented 3 years ago

Thanks for your review. Now that I better understand your frawework, I do... totally agree with you ;-). Do you want I prepare another PR for the missing Getter and the caffeine 3.0.1 dependency now requiring JDK 11 ?

rvandoosselaer commented 3 years ago

Thanks for your review. Now that I better understand your frawework, I do... totally agree with you ;-). Do you want I prepare another PR for the missing Getter and the caffeine 3.0.1 dependency now requiring JDK 11 ?

Yep, I would appreciate this.

vxel commented 2 years ago

I left in the PR only the missing Getter, and fixed version of Caffeine to restore the build.