search

Gnurfos / transvoxel_rs

Rust implementation of the Transvoxel algorithm
Apache License 2.0
24 stars 6 forks source link

Added tests for rendering the same Noise on different Level of detail #2

Closed davids91 closed 1 year ago

davids91 commented 1 year ago

Hi! I thought there was some problem with rendering noisy data on different levels, because seemingly the crashes reacted to setting different resolution; So I wrote a test for that. It turns out this isn't the case, as the crashes are because of incorrectly setting the transition sides somehow, so I'll work on it in another PR.

I hope you'll find this useful!

I'll be finalizing this in the coming days, meanwhile if you'll need any changes I'll be happy to discuss, otherwise I'm okay with it not being merged.

Gnurfos commented 1 year ago

Hi,

Thank you for the report and sorry if the bug is blocking you!

I don't agree 100% with your diagnostic, but this whole caching and density getting is a bit messy, it's not obvious (I'd like to refactor it but am low on time): getting density for transition cells tries to be smart and instead gets a regular point density, when this is possible. But regular points densities outside of the block are only loaded lazily, and too lazily (they should be loaded whenever we are about to need them, but it was done only in the context of a regular cell extraction). So here is a a case where a transition cell extraction needs a density from a regular grid point outside the block.

I pushed a rushed fix, and published 0.5.1

I'm sorry the PR got accidentally rejected when I renamed the main branch. I would welcome them if you can submit tests again illustrating the issue. However, when I ran them, they were very slow.

davids91 commented 1 year ago

Thank you this is really appreciated! I'll fix the tests to be less brute-force-y and update. Unfortunately it's really hard for me to pin-point the problem case, due to the lack of basic understanding of the algorithm, but I'd like to help even in refactoring if it's welcome!

Gnurfos commented 1 year ago

I just added a test, which I think is the minimal case to reproduce the issue (but it doesn't do much in terms of making things less confusing).

davids91 commented 1 year ago

I don't think I can add anything here :) Thank you for the fix!