verpeteren / rust-simd-noise

SIMD noise library for Rust
https://crates.io/crates/simdnoise
251 stars 20 forks source link

Suspected regression in cc1930a #32

Closed Schuwi closed 4 years ago

Schuwi commented 4 years ago

There seems to be some kind of regression in cc1930a. This library is used in https://github.com/feather-rs/feather (server/worldgen/src/density_map/) and from revision cc1930a onwards one of the tests in feather (server/worldgen/src/lib.rs) fails to complete:

assertion failed: y < CHUNK_HEIGHT
thread 'tests::test_reproducability' panicked at 'assertion failed: y < CHUNK_HEIGHT', core/chunk/src/lib.rs:401:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:77
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1052
   5: std::io::Write::write_fmt
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/io/mod.rs:1426
   6: std::io::impls::<impl std::io::Write for alloc::boxed::Box<W>>::write_fmt
             at src/libstd/io/impls.rs:156
   7: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   8: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   9: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:204
  10: std::panicking::default_hook
             at src/libstd/panicking.rs:221
  11: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:472
  12: std::panicking::begin_panic
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/panicking.rs:399
  13: feather_chunk::Chunk::check_coords
             at core/chunk/src/lib.rs:401
  14: feather_chunk::Chunk::set_block_at
             at core/chunk/src/lib.rs:221
  15: <feather_server_worldgen::finishers::snow::SnowFinisher as feather_server_worldgen::FinishingGenerator>::generate_for_chunk
             at server/worldgen/src/finishers/snow.rs:24
  16: <feather_server_worldgen::ComposableGenerator as feather_server_worldgen::WorldGenerator>::generate_chunk
             at server/worldgen/src/lib.rs:180
  17: feather_server_worldgen::tests::test_reproducability
             at server/worldgen/src/lib.rs:422
  18: feather_server_worldgen::tests::test_reproducability::{{closure}}
             at server/worldgen/src/lib.rs:410
  19: core::ops::function::FnOnce::call_once
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libcore/ops/function.rs:232
  20: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/liballoc/boxed.rs:1015
  21: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:86
  22: std::panicking::try
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/panicking.rs:281
  23: std::panic::catch_unwind
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/panic.rs:394
  24: test::run_test_in_process
             at src/libtest/lib.rs:539
  25: test::run_test::run_test_inner::{{closure}}
             at src/libtest/lib.rs:452
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I can't really give you more insight into what exactly went wrong here though, as I have no knowledge of this topic whatsoever, it's just something I noticed when incorporating #31 in feather and I wanted to let you know.

Ralith commented 4 years ago

To be clear, it does not occur in b8c864f0b96ad21c259971b757620aacb357fba4? That commit (and its immediate ancestors) changed the output range of everything, and that looks like the sort of failure you'd expect from that. I made an effort to avoid changing the results in #29, but it's possible I missed something.

Schuwi commented 4 years ago

f642fa7 and previous => works 580d123 and following => doesn't work Also noticed a stack overflow in our test when running with opt-level=0 with f642fa7, which wasn't there in 3.1.5 and doesn't occur with opt-level=1. Guess I'll have to investigate that further.

Ralith commented 4 years ago

580d123 and following => doesn't work

Yeah, that's 100% expected then. Noise amplitudes were mapped into a consistent range, so you'll need to update any scaling factors you used. Output should be otherwise unchanged. See discussion in #27 for details.

Also noticed a stack overflow in our test when running with opt-level=0 with f642fa7

Maybe a dupe of #25?

Schuwi commented 4 years ago

Yeah, I don't understand a word of the comments in that PR ^^ I'll open an issue in feather and leave it to someone who knows this stuff to implement the new version. Those changes to amplitude will require a MAJOR version bump though, right?

Maybe a dupe of #25?

Thanks, added a comment.

jackmott commented 4 years ago

definitely going to be a major version bump in the next version

Schuwi commented 4 years ago

Okay, so everything working as intended after all. Thanks for the help!